Re: [PATCH 3/4] d3d10: Implement ShaderResource effect variable set method.

[Matteo Bruni]

On Wed, Mar 25, 2020 at 7:17 PM Connor McAdams  wrote:
> Signed-off-by: Connor McAdams
>
> dlls/d3d10/d3d10_private.h |   7 +++
>  dlls/d3d10/effect.c        | 114 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 114 insertions(+), 7 deletions(-)
>
> diff --git a/dlls/d3d10/d3d10_private.h b/dlls/d3d10/d3d10_private.h
> index 21d7f964e8..5d7c4e128b 100644
> --- a/dlls/d3d10/d3d10_private.h
> +++ b/dlls/d3d10/d3d10_private.h
> @@ -131,6 +131,12 @@ struct d3d10_effect_state_object_variable
>      } object;
>  };
>
> +struct d3d10_effect_resource_variable
> +{
> - ID3D10ShaderResourceView **resource_view;
> - BOOL parent;
>
> +};

Just to give a couple more naming options: that could also be srv or view.
> -
>
> struct d3d10_effect_buffer_variable
>  {
>      ID3D10Buffer *buffer;
> @@ -196,6 +202,7 @@ struct d3d10_effect_variable
>          struct d3d10_effect_state_object_variable state;
>          struct d3d10_effect_shader_variable shader;
>          struct d3d10_effect_buffer_variable buffer;
> -
>    struct d3d10_effect_resource_variable resource;
> } u;
>
> };
>
> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> index 3d397a38eb..91cb189362 100644
> --- a/dlls/d3d10/effect.c
> +++ b/dlls/d3d10/effect.c
> @@ -2079,6 +2079,28 @@ static HRESULT parse_fx10_local_variable(const char *data, size_t data_size,
>          case D3D10_SVT_TEXTURE2DMSARRAY:
>          case D3D10_SVT_TEXTURE3D:
>          case D3D10_SVT_TEXTURECUBE:
> -
>        if (!v->type->element_count)
> -
>            i = 1;
> -
>        else
> -
>            i = v->type->element_count;
> -
> -
>        if (!(v->u.resource.resource_view = heap_calloc(i, sizeof(ID3D10ShaderResourceView *))))

sizeof(*v->u.resource.resource_view)
> @@ -5924,8 +5977,27 @@ static const struct ID3D10EffectStringVariableVtbl d3d10_effect_string_variable_
>      d3d10_effect_string_variable_GetStringArray,
>  };
>
> +static void set_shader_resource_variable(ID3D10ShaderResourceView **src, ID3D10ShaderResourceView **dst)
> +{
> - if (*dst == *src)
> -
>    return;
> -
> - if (*dst)
> -
>    ID3D10ShaderResourceView_Release(*dst);
> - if (*src)
> -
>    ID3D10ShaderResourceView_AddRef(*src);
> -
> - *dst = *src;
>
> +}

Nitpick: can you swap the order of the if (*src) / if (*dst)? I know
it doesn't matter since there is the equality check just above, but
with the AddRef() before the Release() it becomes obviously correct
and a bit easier to read.
> @@ -6082,9 +6154,13 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_shader_resource_variable_GetRawVal
>  static HRESULT STDMETHODCALLTYPE d3d10_effect_shader_resource_variable_SetResource(
>          ID3D10EffectShaderResourceVariable *iface, ID3D10ShaderResourceView *resource)
>  {
> - FIXME("iface %p, resource %p stub!\n", iface, resource);
> - struct d3d10_effect_variable *effect_var = impl_from_ID3D10EffectShaderResourceVariable(iface);

Just var is fine.
> - return E_NOTIMPL;
> - TRACE("iface %p, resource %p.\n", iface, resource);
> -
> - set_shader_resource_variable(&resource, effect_var->u.resource.resource_view);
> -
> - return S_OK;
>
> }
>
> static HRESULT STDMETHODCALLTYPE d3d10_effect_shader_resource_variable_GetResource(
> @@ -6098,9 +6174,33 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_shader_resource_variable_GetResour
>  static HRESULT STDMETHODCALLTYPE d3d10_effect_shader_resource_variable_SetResourceArray(
>          ID3D10EffectShaderResourceVariable *iface, ID3D10ShaderResourceView **resources, UINT offset, UINT count)
>  {
> - FIXME("iface %p, resources %p, offset %u, count %u stub!\n", iface, resources, offset, count);
> - struct d3d10_effect_variable *effect_var = impl_from_ID3D10EffectShaderResourceVariable(iface);
> - ID3D10ShaderResourceView **rsrc_view;
> - UINT i;

unsigned int i;