[PATCH 5/7] d3d10/effect: Add support for constant index and anonymous shader values in assignment parsing helper.

[Matteo Bruni]

On Fri, Oct 8, 2021 at 7:14 AM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>
> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
> ---
>  dlls/d3d10/effect.c       | 142 +++++++++++++++++++++++++++++----
>  dlls/d3d10/tests/effect.c | 161 +++++++++++++++++++++++++-------------
>  2 files changed, 233 insertions(+), 70 deletions(-)

I have a couple of general comments, not really directed at this patch
(in fact here you're syncing up two functions so it's kind of the
special case where you don't want to follow them) but more as a heads
up.

>
> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> index 1e144bb1c56..6f3ece24e79 100644
> --- a/dlls/d3d10/effect.c
> +++ b/dlls/d3d10/effect.c
> @@ -1710,17 +1710,19 @@ static BOOL is_object_property_type_matching(const struct d3d10_effect_state_pro
>      }
>  }
>
> -static BOOL parse_fx10_state_group(const char *data, size_t data_size,
> +static HRESULT parse_fx10_property_assignment(const char *data, size_t data_size,
>          const char **ptr, enum d3d10_effect_container_type container_type,
>          struct d3d10_effect *effect, void *container)
>  {
>      const struct d3d10_effect_state_property_info *property_info;
> +    UINT value_offset, sodecl_offset, operation;
>      struct d3d10_effect_variable *variable;
> -    UINT value_offset, operation;
> +    unsigned int i, variable_idx;
> +    const char *data_ptr;
>      const char *name;
>      size_t name_len;
> -    unsigned int i;
>      DWORD count;
> +    HRESULT hr;
>      void *dst;
>      UINT idx;
>      UINT id;

As far as I'm concerned, you can just use "unsigned int" (or uint32_t,
where it makes sense) instead of UINT / DWORD for new variables.

> @@ -1807,18 +1809,128 @@ static BOOL parse_fx10_state_group(const char *data, size_t data_size,
>                  else
>                  {
>                      FIXME("Assigning variables to numeric fields is not supported.\n");
> -                    return FALSE;
> +                    return E_FAIL;
>                  }
>
>                  break;
>
> +            case D3D10_EOO_CONST_INDEX:
> +
> +                /* Array variable, constant index. */
> +                if (value_offset >= data_size || !require_space(value_offset, 2, sizeof(DWORD), data_size))
> +                {
> +                    WARN("Invalid offset %#x (data size %#lx).\n", value_offset, (long)data_size);
> +                    return E_FAIL;
> +                }
> +                data_ptr = data + value_offset;
> +                read_dword(&data_ptr, &value_offset);
> +                read_dword(&data_ptr, &variable_idx);
> +
> +                if (!fx10_get_string(data, data_size, value_offset, &name, &name_len))
> +                {
> +                    WARN("Failed to get variable name.\n");
> +                    return E_FAIL;
> +                }
> +
> +                TRACE("Variable name %s[%u].\n", debugstr_a(name), variable_idx);
> +
> +                if (!(variable = d3d10_effect_get_variable_by_name(effect, name)))
> +                {
> +                    WARN("Couldn't find variable %s.\n", debugstr_a(name));
> +                    return E_FAIL;
> +                }
> +
> +                /* Has to be an array */
> +                if (!variable->type->element_count || variable_idx >= variable->type->element_count)
> +                {
> +                    WARN("Invalid array size %u.\n", variable->type->element_count);
> +                    return E_FAIL;
> +                }
> +
> +                if (is_object_property(property_info))
> +                {
> +                    if (!is_object_property_type_matching(property_info, variable))
> +                    {
> +                        WARN("Object type mismatch. Variable type %#x, property type %#x.\n",
> +                                variable->type->basetype, property_info->type);
> +                        return E_FAIL;
> +                    }
> +
> +                    *(void **)dst = &variable->elements[variable_idx];
> +                }
> +                else
> +                {
> +                    FIXME("Assigning indexed variables to numeric fields is not supported.\n");
> +                    return E_FAIL;
> +                }
> +
> +                break;
> +
> +            case D3D10_EOO_ANONYMOUS_SHADER:
> +
> +                /* Anonymous shader */
> +                if (effect->anonymous_shader_current >= effect->anonymous_shader_count)
> +                {
> +                    ERR("Anonymous shader count is wrong!\n");
> +                    return E_FAIL;
> +                }
> +
> +                if (value_offset >= data_size || !require_space(value_offset, 2, sizeof(DWORD), data_size))
> +                {
> +                    WARN("Invalid offset %#x (data size %#lx).\n", value_offset, (long)data_size);
> +                    return E_FAIL;
> +                }
> +                data_ptr = data + value_offset;
> +                read_dword(&data_ptr, &value_offset);
> +                read_dword(&data_ptr, &sodecl_offset);
> +
> +                TRACE("Effect object starts at offset %#x.\n", value_offset);
> +
> +                if (FAILED(hr = parse_fx10_anonymous_shader(effect,
> +                        &effect->anonymous_shaders[effect->anonymous_shader_current], property_info->id)))
> +                    return hr;
> +
> +                variable = &effect->anonymous_shaders[effect->anonymous_shader_current].shader;
> +                ++effect->anonymous_shader_current;

I see nothing wrong with simply using the postincrement operator in
the previous assignment instead.