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

[Matteo Bruni]

2016-04-12 11:34 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> @@ -369,12 +410,17 @@ static HRESULT get_constants_desc(unsigned int *byte_code, struct d3dx_const_tab
>              goto err_out;
>          }
>          count = 1;
> -        hr = ID3DXConstantTable_GetConstantDesc(ctab, hc, &cdesc[i], &count);
> -        if (FAILED(hr))
> +        if (FAILED(hr = ID3DXConstantTable_GetConstantDesc(ctab, hc, &cdesc[i], &count)))
>          {
>              FIXME("Could not get constant desc, hr %#x.\n", hr);
>              goto err_out;
>          }
> +        if (count != 1)
> +        {
> +            FIXME("Unexpected constant descriptors count %u.\n", count);
> +            hr = D3DERR_INVALIDCALL;
> +            goto err_out;
> +        }

Notice that, since you set count to 1 before calling
GetConstantDesc(), the only possible results are 0 or 1 and the 0 case
should be already handled by the "if (FAILED())" branch.
You might use a temporary array of 3 elements and pass 3 as count,
then only copy the first element to the destination so the rest of the
code is unchanged while still giving a chance for the FIXME to
trigger. Not sure it's worth it though, maybe just adding support for
multiple descriptors (even only in struct d3dx_const_tab and the
immediately related code) might be a better plan.
> +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;
> +    unsigned int desc_count;
> +
> +    desc_count = 1;
> +    ID3DXConstantTable_GetConstantDesc(ctab, hc, &desc, &desc_count);
> +    if (desc_count != 1)
> +    {
> +        FIXME("Unexpected constant descriptors count %u.\n", desc_count);
> +        return D3DERR_INVALIDCALL;
> +    }

Same here, in this case the descriptors are only used locally so both
options above should be easier to do.
> +    if (const_count != param_count)
> +    {
> +        FIXME("Number of elements or struct members differs between parameter and constant.\n");
> +        return D3DERR_INVALIDCALL;
> +    }

You could print the two counts here. Not a big deal though.

Looks good otherwise.

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

[Paul Gofman]

On 04/13/2016 10:59 PM, Matteo Bruni wrote:
> 2016-04-12 11:34 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> > @@ -369,12 +410,17 @@ static HRESULT get_constants_desc(unsigned int *byte_code, struct d3dx_const_tab
> >              goto err_out;
> >          }
> >          count = 1;
> > -        hr = ID3DXConstantTable_GetConstantDesc(ctab, hc, &cdesc[i], &count);
> > -        if (FAILED(hr))
> > +        if (FAILED(hr = ID3DXConstantTable_GetConstantDesc(ctab, hc, &cdesc[i], &count)))
> >          {
> >              FIXME("Could not get constant desc, hr %#x.\n", hr);
> >              goto err_out;
> >          }
> > +        if (count != 1)
> > +        {
> > +            FIXME("Unexpected constant descriptors count %u.\n", count);
> > +            hr = D3DERR_INVALIDCALL;
> > +            goto err_out;
> > +        }
> > Notice that, since you set count to 1 before calling
> > GetConstantDesc(), the only possible results are 0 or 1 and the 0 case
> > should be already handled by the "if (FAILED())" branch.
> > You might use a temporary array of 3 elements and pass 3 as count,
> > then only copy the first element to the destination so the rest of the
> > code is unchanged while still giving a chance for the FIXME to
> > trigger. Not sure it's worth it though, maybe just adding support for
> > multiple descriptors (even only in struct d3dx_const_tab and the
> > immediately related code) might be a better plan.

I would prefer to keep FIXME (adding a temporary buffer for proper
check) for now if possible, as adding a support to d3dx_const_tab looks
a bit tricky without any working example of this (which in turn requires
the support in ID3DXConstantTable to be tested).

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

[Matteo Bruni]

> I would prefer to keep FIXME (adding a temporary buffer for proper
> check) for now if possible, as adding a support to d3dx_const_tab looks
> a bit tricky without any working example of this (which in turn requires
> the support in ID3DXConstantTable to be tested).

Sure, that's okay. Thinking about it, it might even turn out that in
effect files this doesn't happen in practice and the FIXME is just
fine.

Essentially, depending on its usage in the shader, compiler options
and such, a single shader constant can be "instantiated" into more
than one register set and d3dx9, when uploading constants, needs to
set the value for all the copies.
You can try to write a shader using the same constant in some generic
math expression and as a loop count / if condition, that might
generate one of those "multiple descriptors" constants. You might need
to disable loop unrolling or other optimizations though. I'm sure I
have some shader like that somewhere but I can't find it right now...

With effects a similar end result can be obtained by means of a
preshader. I guess it depends on what construction the compiler
prefers in such a situation.