[PATCH 8/8] d3d10/effect: Use shader variable directly from pass structure on Apply().

[Matteo Bruni]

On Thu, Oct 7, 2021 at 9:00 PM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>
>
>
> On 10/7/21 9:45 PM, Matteo Bruni wrote:
> > On Wed, Oct 6, 2021 at 9:54 AM Nikolay Sivov <nsivov at codeweavers.com> wrote:
> >> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
> >> ---
> >>  dlls/d3d10/d3d10_private.h |  15 ---
> >>  dlls/d3d10/effect.c        | 185 +++++++++++++------------------------
> >>  2 files changed, 64 insertions(+), 136 deletions(-)
> > Love to see that! Very nice patch overall.
> >
> > There's only a couple of small things...
> >
> >> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> >> index 0ba906375d7..d4e44a7f419 100644
> >> --- a/dlls/d3d10/effect.c
> >> +++ b/dlls/d3d10/effect.c
> >> @@ -4109,12 +4029,35 @@ static void apply_shader_resources(ID3D10Device *device, struct d3d10_effect_var
> >>      }
> >>  }
> >>
> >> +static void d3d10_effect_pass_set_shader(struct d3d10_effect_pass *pass,
> >> +        const struct d3d10_effect_pass_shader_desc *shader_desc)
> >> +{
> >> +    ID3D10Device *device = pass->technique->effect->device;
> >> +    struct d3d10_effect_variable *v = shader_desc->shader;
> >> +
> >> +    if (v->type->element_count)
> >> +        v = &v->elements[shader_desc->index];
> >> +
> >> +    switch (v->type->basetype)
> >> +    {
> >> +        case D3D10_SVT_VERTEXSHADER:
> >> +            ID3D10Device_VSSetShader(device, v->u.shader.shader.vs);
> >> +            break;
> >> +        case D3D10_SVT_PIXELSHADER:
> >> +            ID3D10Device_PSSetShader(device, v->u.shader.shader.ps);
> >> +            break;
> >> +        case D3D10_SVT_GEOMETRYSHADER:
> >> +            ID3D10Device_GSSetShader(device, v->u.shader.shader.gs);
> >> +            break;
> >> +        default:
> >> +            WARN("Unexpected shader type %u.\n", v->type->basetype);
> >> +    }
> >> +}
> >> +
> > Like in some of the previous patches, this is actually a fix to
> > shaders arrays / D3D10_EOO_CONST_INDEX, which would be nice to
> > mention. FWIW this is more D3D10_EOO_VAR_INDEX-ready than the others.
> Existing code already picked correct shader object, using
> GetVertexShader(index, &vs) methods on shader variable interface.
> So arrays were already working I believe, but yes, that didn't make it
> easier to support variable index, let alone switching between variables.
>
> >
> >>  static HRESULT STDMETHODCALLTYPE d3d10_effect_pass_Apply(ID3D10EffectPass *iface, UINT flags)
> >>  {
> >>      struct d3d10_effect_pass *pass = impl_from_ID3D10EffectPass(iface);
> >>      ID3D10Device *device = pass->technique->effect->device;
> >> -    HRESULT hr = S_OK;
> >> -    unsigned int i;
> >>
> >>      TRACE("iface %p, flags %#x\n", iface, flags);
> >>
> >> @@ -4134,14 +4077,14 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_pass_Apply(ID3D10EffectPass *iface
> >>      if (pass->blend)
> >>          ID3D10Device_OMSetBlendState(device, pass->blend->u.state.object.blend,
> >>                  pass->blend_factor, pass->sample_mask);
> >> +    if (pass->vs.shader != &null_shader_variable)
> >> +        d3d10_effect_pass_set_shader(pass, &pass->vs);
> >> +    if (pass->ps.shader != &null_shader_variable)
> >> +        d3d10_effect_pass_set_shader(pass, &pass->ps);
> >> +    if (pass->gs.shader != &null_shader_variable)
> >> +        d3d10_effect_pass_set_shader(pass, &pass->gs);
> >>
> >> -    for (i = 0; i < pass->object_count; ++i)
> >> -    {
> >> -        hr = d3d10_effect_object_apply(&pass->objects[i]);
> >> -        if (FAILED(hr)) break;
> >> -    }
> >> -
> >> -    return hr;
> >> +    return S_OK;
> >>  }
> > I suspect you still want to set the shaders unconditionally (e.g. what
> > if pass 0 sets a non-NULL geometry shader and pass 1 sets NULL?)
> That's why I sent some state block mask tests. There appears to be a
> difference between SetPixelShader(NULL) and no SetPixelShader()
> instruction at all. Currently, and that's what existing code did, we use
> null_shader_variable as initial value, and anonymous_{vs|gs|ps} for
> assigned NULL values. I believe that is also visible with
> SHADER_PASS_DESC, that never returns NULL vars.

Ah, interesting. I'll have another look to make sure but it sounds
like the patch is fine.