Re: [PATCH 02/10] d3d10: Implement scalar effect variable set methods.

[Matteo Bruni]

On Sat, Dec 7, 2019 at 7:23 PM Connor McAdams  wrote:
> Implement SetFloat/SetFloatArray, SetInt/SetIntArray, and
> SetBool/SetBoolArray methods for the scalar effect variable interface.
> Signed-off-by: Connor McAdams
>
> dlls/d3d10/effect.c | 83 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 71 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> index f0932409b1..4ecc0753de 100644
> --- a/dlls/d3d10/effect.c
> +++ b/dlls/d3d10/effect.c
> @@ -4212,6 +4212,43 @@ static const struct ID3D10EffectConstantBufferVtbl d3d10_effect_constant_buffer_
>      d3d10_effect_constant_buffer_GetTextureBuffer,
>  };
>
> +static inline void write_variable_to_cbuffer(struct d3d10_effect_variable *variable, void *data)

I'm not too happy about the name of this function but I'm not sure
about a better alternative. Maybe just _buffer (no c)?

Also I'd drop the inline.
> +{
> - memcpy(variable->buffer->u.buffer.local_buffer + variable->buffer_offset, data, variable->type->size_packed);
> -
> - variable->buffer->u.buffer.changed = 1;
>
> +}
> -
>
> +static void write_variable_array_to_cbuffer(struct d3d10_effect_variable *variable, void *data, UINT count)
> +{
> - char *cbuf = variable->buffer->u.buffer.local_buffer + variable->buffer_offset;
> - char *cur_element = data;
> - DWORD element_size;
> - UINT i;

No reason to use UINT or DWORD here and in the function arguments.
> -
> - /*
> -
> * If for some reason we try to use an array write on a variable that
> -
> * isn't an array, just default back to the normal variable write.
> -
> */
> - if (!variable->type->element_count)
> - {
> -
>    write_variable_to_cbuffer(variable, data);
> -
>    return;
> - }

I don't think this comment should be there, there isn't anything
special about using array methods to write to non-array variables.
> - element_size = variable->type->elementtype->size_packed;
> -
> - for (i = 0; i < count; i++)
> - {
> -
>    memcpy(cbuf, cur_element, element_size);
> -
> -
>    cur_element += element_size;
> -
>    cbuf += variable->type->stride;
> - }
> -
> - variable->buffer->u.buffer.changed = 1;
>
> +}
> -
>
> /* ID3D10EffectVariable methods */
>
> static BOOL STDMETHODCALLTYPE d3d10_effect_scalar_variable_IsValid(ID3D10EffectScalarVariable *iface)
> @@ -4370,9 +4407,12 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetRawValue(ID3D10
>  static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetFloat(ID3D10EffectScalarVariable *iface,
>          float value)
>  {
> - FIXME("iface %p, value %.8e stub!\n", iface, value);
> - struct d3d10_effect_variable *effect_var = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface);

There are already a few instances of this (i.e. upcasting to
ID3D10EffectVariable) and I'm not sure that the alternative is
necessarily better, but another option would be to introduce separate
impl_from_ID3D10Effect*Variable() for each variable type.
> - return E_NOTIMPL;
> - TRACE("iface %p, value %.8e.\n", iface, value);
> - write_variable_to_cbuffer(effect_var, &value);
> -
> - return S_OK;
>
> }
>
> static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetFloat(ID3D10EffectScalarVariable *iface,
> @@ -4383,12 +4423,19 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetFloat(ID3D10Eff
>      return E_NOTIMPL;
>  }
>
> +/*
> -
> - According to MSDN, array writing functions for Scalar/Vector effect
> -
> - variables have offset go unused.
> - */

We want a test for that specifically (and for all of those additions
in general).