Re: [PATCH 2/4] d3d10: Apply shader resources for shaders used in pass.

[Matteo Bruni]

On Sat, Mar 21, 2020 at 7:30 PM Connor McAdams  wrote:
> Signed-off-by: Connor McAdams
>
> dlls/d3d10/effect.c | 105 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>
> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> index 580b6079cf..f86ec5ce9f 100644
> --- a/dlls/d3d10/effect.c
> +++ b/dlls/d3d10/effect.c
> @@ -3617,9 +3617,107 @@ static struct ID3D10EffectVariable * STDMETHODCALLTYPE d3d10_effect_pass_GetAnno
>      return &null_variable.ID3D10EffectVariable_iface;
>  }
>
> +static void update_buffer(ID3D10Device *device, struct d3d10_effect_variable *v)
> +{
> - struct d3d10_effect_buffer_variable *b = &v->u.buffer;
> -
> - if (!b->changed)
> -
>    return;
> -
> - ID3D10Device_UpdateSubresource(device, (ID3D10Resource *)b->buffer, 0, NULL,
> -
>        (const void *)b->local_buffer, v->data_size, 0);

The (const void *) cast is unnecessary.
> -
> - b->changed = FALSE;
>
> +}
> -
>
> +static void apply_shader_resources(ID3D10Device *device, struct ID3D10EffectShaderVariable *variable)
> +{
> - struct d3d10_effect_variable *v = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)variable);

I think we should introduce a impl_from_ID3D10EffectShaderVariable(),
like we recently did for ID3D10EffectScalarVariable and such. Probably
as a separate patch, replacing the existing cases at the same time.
> - struct d3d10_effect_shader_variable *sv = &v->u.shader;
> - struct d3d10_effect_shader_resource *sr;
> - ID3D10ShaderResourceView *const *srv;

I see no reason for the const there, it's not really protecting much.
> - struct d3d10_effect_variable *rsrc_v;
> -
> - unsigned int i;

Blank line?
> -
> - for (i = 0; i < sv->resource_count; ++i)
> - {
> -
>    sr = &sv->shader_resource[i];
> -
>    rsrc_v = sr->resource_variable;
> -
> -
>    switch (sr->in_type)
> -
>    {
> -
>        case D3D10_SIT_CBUFFER:
> -
>            update_buffer(device, rsrc_v);
> -
>            switch (v->type->basetype)
> -
>            {
> -
>                case D3D10_SVT_VERTEXSHADER:
> -
>                    ID3D10Device_VSSetConstantBuffers(device, sr->bind_point, sr->bind_count,
> -
>                            (ID3D10Buffer *const *)&rsrc_v->u.buffer.buffer);

Those casts are unnecessary AFAICS.
> -
>                    break;
> -
>                case D3D10_SVT_PIXELSHADER:
> -
>                    ID3D10Device_PSSetConstantBuffers(device, sr->bind_point, sr->bind_count,
> -
>                            (ID3D10Buffer *const *)&rsrc_v->u.buffer.buffer);
> -
>                    break;
> -
>                case D3D10_SVT_GEOMETRYSHADER:
> -
>                    ID3D10Device_GSSetConstantBuffers(device, sr->bind_point, sr->bind_count,
> -
>                            (ID3D10Buffer *const *)&rsrc_v->u.buffer.buffer);
> -
>                    break;
> -
>                default:
> -
>                    break;

It might be worthwhile to add a WARN() or something for the default case.
> @@ -3627,6 +3725,13 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_pass_Apply(ID3D10EffectPass *iface
>  if (flags) FIXME("Ignoring flags (%#x)\n", flags);
>
> - if (This->vs.pShaderVariable != (ID3D10EffectShaderVariable *)&null_shader_variable.ID3D10EffectVariable_iface)
> -
>    apply_shader_resources(device, This->vs.pShaderVariable);

These casts also look ugly but, as I understand it, they're caused by
the idiotic d3d10 effect variable object model and are basically
unavoidable :/

While touching the function, though, you could rename "This" to "pass".