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

[Nikolay Sivov]

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.