[PATCH v5 1/5] d3d10: Implement scalar effect variable set methods.

[Matteo Bruni]

On Fri, Feb 28, 2020 at 7:52 PM Connor McAdams <conmanx360 at gmail.com> wrote:>
> Implement SetFloat/SetFloatArray, SetInt/SetIntArray, and
> SetBool/SetBoolArray methods for the scalar effect variable interface.
>
> Signed-off-by: Connor McAdams <conmanx360 at gmail.com>
> ---
> v5: Added correct behavior when Get/Set methods are called for types
> different than the variable type, i.e SetInt on a Float variable and
> vice versa.
>
>  dlls/d3d10/effect.c | 187 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 175 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> index 3ee9bf7a35..42311bd1fe 100644
> --- a/dlls/d3d10/effect.c
> +++ b/dlls/d3d10/effect.c
> @@ -4212,8 +4212,146 @@ static const struct ID3D10EffectConstantBufferVtbl d3d10_effect_constant_buffer_
>      d3d10_effect_constant_buffer_GetTextureBuffer,
>  };
>
> +
> +static BOOL get_variable_as_bool(void *in_data, D3D10_SHADER_VARIABLE_TYPE in_type)

This is a bit wrong in principle. As you noticed, d3d10 effect /
shader booleans canonically have a value of ~0u (or -1, unsigned vs
signed) for true and 0 for false. In a sense this makes using the
Windows type BOOL for them not quite correct. DWORD, for example, is
probably okay instead, here and in the other places using BOOL for
this purpose.
Same thing with TRUE or FALSE. I don't think there are types or
constants for these kind of booleans in MS headers but we could still
define some if useful.

> +{
> +    switch (in_type)
> +    {
> +        case D3D10_SVT_FLOAT:
> +        case D3D10_SVT_INT:
> +        case D3D10_SVT_BOOL:
> +            if (*(DWORD *)in_data)
> +                return -1;
> +            break;
> +
> +        default:
> +            break;
> +    }
> +
> +    return FALSE;
> +}
> +
> +static int get_variable_as_int(void *in_data, D3D10_SHADER_VARIABLE_TYPE in_type)
> +{
> +    switch (in_type)
> +    {
> +        case D3D10_SVT_FLOAT:
> +            return (int)(*(float *)in_data);
> +
> +        case D3D10_SVT_INT:
> +            return *(int *)in_data;
> +
> +        case D3D10_SVT_BOOL:
> +            return get_variable_as_bool(in_data, in_type);
> +
> +        default:
> +            return 0;
> +    }
> +}
> +
> +static float get_variable_as_float(void *in_data, D3D10_SHADER_VARIABLE_TYPE in_type)
> +{
> +    switch (in_type)
> +    {
> +        case D3D10_SVT_FLOAT:
> +            return *(float *)in_data;
> +
> +        case D3D10_SVT_INT:
> +            return (float)(*(int *)in_data);
> +
> +        case D3D10_SVT_BOOL:
> +            return (float)get_variable_as_bool(in_data, in_type);
> +
> +        default:
> +            return 0.0f;
> +    }
> +}
> +
> +static void write_variable(BYTE *out_data, D3D_SHADER_VARIABLE_TYPE out_type, BYTE *in_data,
> +        D3D_SHADER_VARIABLE_TYPE in_type, unsigned int col_count)

Nitpicks: this function doesn't really care about variables, it's just
a conversion function. Same with its helpers above actually.
Maybe get_value_as_*() for the helpers and get_vector_as_type() for
this one? Just an idea.
Related, this function doesn't really handle columns, but components.
I'd just call the last parameter "count".

> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < col_count; i++, out_data += 4, in_data += 4)

I guess sizeof(DWORD) might be better than a somewhat magic 4.

> +    {
> +        if (out_type == in_type)
> +            *(DWORD *)out_data = *(DWORD *)in_data;
> +        else
> +        {
> +            switch (out_type)
> +            {
> +                case D3D10_SVT_FLOAT:
> +                    *(float *)out_data = get_variable_as_float(in_data, in_type);
> +                    break;
> +
> +                case D3D10_SVT_INT:
> +                    *(int *)out_data = get_variable_as_int(in_data, in_type);
> +                    break;
> +
> +                case D3D10_SVT_BOOL:
> +                    *(BOOL *)out_data = get_variable_as_bool(in_data, in_type);
> +                    break;
> +
> +                default:
> +                    *(DWORD *)out_data = 0;
> +                    break;
> +            }
> +        }
> +    }
> +}
> +
> +static void write_variable_to_buffer(struct d3d10_effect_variable *variable, void *data,
> +        D3D_SHADER_VARIABLE_TYPE in_type)
> +{
> +    BYTE *buf = variable->buffer->u.buffer.local_buffer + variable->buffer_offset;
> +
> +    write_variable(buf, variable->type->basetype, data, in_type, variable->type->column_count);
> +
> +    variable->buffer->u.buffer.changed = TRUE;
> +}
> +
> +static void write_variable_array_to_buffer(struct d3d10_effect_variable *variable, void *data,
> +        D3D_SHADER_VARIABLE_TYPE in_type, unsigned int offset, unsigned int count)
> +{
> +    BYTE *buf = variable->buffer->u.buffer.local_buffer + variable->buffer_offset;
> +    BYTE *cur_element = data;
> +    unsigned int element_size, i;
> +
> +    if (!variable->type->element_count)
> +    {
> +        write_variable_to_buffer(variable, data, in_type);
> +        return;
> +    }
> +
> +    if (offset >= variable->type->element_count)
> +        return;
> +
> +    if (count > variable->type->element_count - offset)
> +        count = variable->type->element_count - offset;

I don't think this is tested in patch 5/5 at the moment. We probably
should, unless Windows doesn't check for out of bounds access (but in
that case we should write a comment somewhere).

> +
> +    element_size = variable->type->elementtype->size_packed;
> +    if (offset)
> +        buf += variable->type->stride * offset;
> +
> +    for (i = 0; i < count; i++)
> +    {
> +        write_variable(buf, variable->type->basetype, cur_element, in_type,
> +                variable->type->column_count);
> +
> +        cur_element += element_size;
> +        buf += variable->type->stride;
> +    }
> +
> +    variable->buffer->u.buffer.changed = TRUE;
> +}
> +
>  /* ID3D10EffectVariable methods */
>
> +static inline struct d3d10_effect_variable *impl_from_ID3D10EffectScalarVariable(ID3D10EffectScalarVariable *iface)
> +{
> +    return CONTAINING_RECORD(iface, struct d3d10_effect_variable, ID3D10EffectVariable_iface);
> +}
> +
>  static BOOL STDMETHODCALLTYPE d3d10_effect_scalar_variable_IsValid(ID3D10EffectScalarVariable *iface)
>  {
>      TRACE("iface %p\n", iface);
> @@ -4370,9 +4508,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_ID3D10EffectScalarVariable(iface);
>
> -    return E_NOTIMPL;
> +    TRACE("iface %p, value %.8e.\n", iface, value);
> +    write_variable_to_buffer(effect_var, &value, D3D10_SVT_FLOAT);
> +
> +    return S_OK;
>  }
>
>  static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetFloat(ID3D10EffectScalarVariable *iface,
> @@ -4383,12 +4524,22 @@ 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. Testing has revealed that this is
> + * true for Scalar variables, but not for vectors. So, in the case of
> + * Scalar variables, just pass 0 as the offset argument of
> + * write_variable_array_to_buffer.
> + */

Let's leave the reference to MSDN out of the comment, it isn't
interesting. The testing part can certainly stay. No need to
explicitly say that you're passing 0 as offset. Perhaps something
like:

/* Tests show that offset is ignored for scalar variables. */