Re: [v4 3/6] d3dx9: implement fxlc constants (expressions) in effect.

[Matteo Bruni]

2016-03-17 12:59 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
> @@ -2616,7 +2630,7 @@ static void d3dx9_set_material_parameter(enum MATERIAL_TYPE op, D3DMATERIAL9 *ma
>              D3DCOLORVALUE c = *(D3DCOLORVALUE *)value;
>
> TRACE("%s, value (%f %f %f %f).\n", material_tbl[op].name, c.r, c.g, c.b, c.a);
> -            *(D3DCOLORVALUE *)((char *)material + material_tbl[op].offset) = c;
> +            *(D3DCOLORVALUE *)((BYTE *)material + material_tbl[op].offset) = c;
>              break;
>          }
>          default:

While you're at it, you could change the format strings to use %.8e
for the float values.
> +static void regstore_reset_table(struct d3dx_regstore *rs, unsigned int table)
> +{
> +    unsigned int sz;
> +
> +    sz = rs->table_sizes[table] * table_info[table].reg_component_count * table_info[table].component_size;
> +
> +    memset(rs->tables[table], 0, sz);
> +    memset(rs->table_value_set[table], 0,
> +            sizeof(*rs->table_value_set[table]) *
> +            ((rs->table_sizes[table] + PRES_VS_BITS_PER_WORD - 1) / PRES_VS_BITS_PER_WORD));
> +}

This function is unused and the compiler should throw a warning about it.
> +    rs->table_value_set[table][reg_idx / PRES_VS_BITS_PER_WORD] |=
> +                1 << (reg_idx % PRES_VS_BITS_PER_WORD);

That should be "1u", otherwise here shifting by 31 is technically
undefined behavior and there are some compilers that complain about
it.
The line continuation is also misindented.
> +    while (param->member_count && param->element_count)
> +    {
> +        if (param->element_count > 1)
> +        {
> +            FIXME("Unexpected param having both elements and members.\n");
> +            return D3DERR_INVALIDCALL;
> +        }
> +        param = ¶m->members[0];
> +    }

I don't quite understand what you're trying to do here but it doesn't
look right.
> +    TRACE("%s rows %u, par columns %u, par class %u, par flags %u, par bytes %u, c rows %u," \
> +            "c columns %u, c class %u, c bytes %u, transpose %u.\n",
> +            desc.Name, param->rows, param->columns, param->class, param->flags, param->bytes,
> +            desc.Rows, desc.Columns, desc.Class, desc.Bytes, transpose);

I would make two separate TRACEs for parameter and constant info. Also
the string comes from the application so you should use debugstr_a().
Something like:

TRACE("Constant %s rows %u, columns %u, class %u, bytes %u.\n",
        debugstr_a(desc.Name), desc.Rows, desc.Columns, desc.Class, desc.Bytes);
TRACE("Parameter rows %u, columns %u, class %u, flags %#x, bytes %u,
transpose %#x.\n",
        param->rows, param->columns, param->class, param->flags,
param->bytes, transpose);

Gmail will probably mangle the code but you get the point.

[v4 3/6] d3dx9: implement fxlc constants (expressions) in effect.

[Paul Gofman]

On 03/19/2016 12:56 AM, Matteo Bruni wrote:
> > +    while (param->member_count && param->element_count)
> > +    {
> > +        if (param->element_count > 1)
> > +        {
> > +            FIXME("Unexpected param having both elements and members.\n");
> > +            return D3DERR_INVALIDCALL;
> > +        }
> > +        param = ¶m->members[0];
> > +    }
> > I don't quite understand what you're trying to do here but it doesn't
> > look right.

That is related to the strange case I met in the real world effect data
when the parameter has structure class, element_count 1, member_count >
1. As I understood from the pre-existing effect parsing code,
element_count 1 together with member_count > 0 (and > 1) is legal. When
element_count is 1, and member_count > 1, members contain an "array" of
1 element, which is actual struct parameter with element_count = 0 and
member_count is the same as in parent weird parameter. It is what
existing d3dx9_parse_effect_typedef understands (element_count is taking
precedence over member_count). I could not reproduce this struct
representation compiling effect from fxc code yet, but seen it in real
world effect. CTAB structure in the shader which correspond to used that
strange struct parameter does not have this weirdness and has element
count 0, so I cannot match CTAB parameter to effect parameter without
going a level deeper in effect parameter.

The existing effect parsing code clearly assumes that if both
element_count > 0 and member_count > 0, element_count is taking
precedence (that is, members array has element_count elements). I am not
sure if it considers illegal element_count > 1 and member_count > 0, but
I do not know currently how to treat this (if it is ever possible) as
did not see such an example, so currently this is a FIXME in the code
(which will hopefully never happen).

[v4 3/6] d3dx9: implement fxlc constants (expressions) in effect.

[Paul Gofman]

On 03/19/2016 02:28 AM, Paul Gofman wrote:
> On 03/19/2016 12:56 AM, Matteo Bruni wrote:
> > > +    while (param->member_count && param->element_count)
> > > +    {
> > > +        if (param->element_count > 1)
> > > +        {
> > > +            FIXME("Unexpected param having both elements and members.\n");
> > > +            return D3DERR_INVALIDCALL;
> > > +        }
> > > +        param = ¶m->members[0];
> > > +    }
> > > I don't quite understand what you're trying to do here but it doesn't
> > > look right.
>
> That is related to the strange case I met in the real world effect data
> when the parameter has structure class, element_count 1, member_count >
> 1. As I understood from the pre-existing effect parsing code,
> element_count 1 together with member_count > 0 (and > 1) is legal. When
> element_count is 1, and member_count > 1, members contain an "array" of
> 1 element, which is actual struct parameter with element_count = 0 and
> member_count is the same as in parent weird parameter. It is what
> existing d3dx9_parse_effect_typedef understands (element_count is taking
> precedence over member_count). I could not reproduce this struct
> representation compiling effect from fxc code yet, but seen it in real
> world effect. CTAB structure in the shader which correspond to used that
> strange struct parameter does not have this weirdness and has element
> count 0, so I cannot match CTAB parameter to effect parameter without
> going a level deeper in effect parameter.
>
> The existing effect parsing code clearly assumes that if both
> element_count > 0 and member_count > 0, element_count is taking
> precedence (that is, members array has element_count elements). I am not
> sure if it considers illegal element_count > 1 and member_count > 0, but
> I do not know currently how to treat this (if it is ever possible) as
> did not see such an example, so currently this is a FIXME in the code
> (which will hopefully never happen).

I reproduced this uneven case in a test, this corresponds to an array of
structs having the size of 1. Unlike this case, the case when array of
structs is greater in size does not have any "weirdness" but result in
member_count > 0, and element_count > 0, and so far causes the error in
the current code. I am adding array tests to the test scenario (struct
array of size 1 and 2, and "usual" array of size 1 and 2), and changing
this place to match all the cases correctly.