[v10 1/4] d3dx9: Implement fxlc constants (expressions) in effect.

[Matteo Bruni]

2016-04-07 17:41 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:

>      switch (state->type)
>      {
>          case ST_CONSTANT:
> +            *param_value = param->data;
> +            *out_param = param;
> +            return D3D_OK;
>          case ST_PARAMETER:
> +            param = param->referenced_param;
>              *param_value = param->data;
>              *out_param = param;
>              return D3D_OK;

It doesn't matter (this is perfectly fine) but you could have kept the
fallthrough by switching these two cases around.

> +static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab,
> +        D3DXHANDLE hc, struct d3dx_parameter *param)
> +{
> +    ID3DXConstantTable *ctab = const_tab->ctab;
> +    D3DXCONSTANT_DESC desc;
> +    unsigned int const_count, param_count, i, j, n, table, start_offset;
> +    unsigned int minor, major, major_stride, param_offset;
> +    BOOL transpose, get_element;
> +
> +    ID3DXConstantTable_GetConstantDesc(ctab, hc, &desc, &const_count);

This isn't correct, although it currently works. const_count is
supposed to be an inout parameter for GetConstantDesc() (you could
potentially have multiple instances of the same constant in different
register sets). The current implementation of GetConstantDesc()
doesn't check the parameter and blindly returns only the first
descriptor, also setting const_count to 1 (actually the following
descriptors aren't even parsed currently by the constant table code
IIRC) so this ends up working, although that's a bug and at some point
it might get fixed which would in turn break this.
Also it's a bit of an "overload" to use the same variable here and as
the count of constant elements / members in the following lines, it's
probably nicer to use two separate variables.

BTW you're using GetConstantDesc() correctly in get_constants_desc().

> +    table = const_tab->regset2table[desc.RegisterSet];
> +    if (table >= PRES_REGTAB_COUNT)
> +    {
> +        FIXME("Unexpected register set %u.\n", desc.RegisterSet);
> +        return D3DERR_INVALIDCALL;
> +    }

Since regset2table is defined by us ERR() is maybe more appropriate
here. It might make sense to also check desc.RegisterSet before
accessing the array.

> +        hc = ID3DXConstantTable_GetConstant(const_tab->ctab, NULL, i);
> +        if (!hc)
> +        {
> +            FIXME("Could not get constant.\n");
> +            hr = D3DERR_INVALIDCALL;

You could probably also print the value of 'i' in the FIXME.
Up to you, but you could switch the then and else branches and drop
the '!' in the if condition.

> +        if (oi->func_all_comps)
> +        {
> +            if (oi->input_count * ins->component_count > ARGS_ARRAY_SIZE)
> +            {
> +                FIXME("Too many arguments (%u) for one instruction.\n", oi->input_count * ins->component_count);
> +                return E_FAIL;
> +            }

I think this check should be outside of the if (oi->func_all_comps) block.

> +    hr = set_constants(&peval->pres.regs, &peval->pres.inputs);
> +    if (FAILED(hr))
> +        return hr;
> +
> +    hr = execute_preshader(&peval->pres);
> +    if (FAILED(hr))
> +        return hr;

Not a big deal but since you're there... You could merge the function
calls with the respective if, like you're doing in some other patches
of the series.