[v9 3/6] d3dx9: Implement fxlc constants (expressions) in effect.

[Matteo Bruni]

2016-04-01 13:21 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:

> +static double regstore_get_double(struct d3dx_regstore *rs, unsigned int table, unsigned int offset)
> +{
> +    BYTE *p;
> +
> +    p = (BYTE *)rs->tables[table] + table_info[table].component_size * offset;
> +    switch (table_info[table].type)
> +    {
> +        case PRES_VT_FLOAT : return *(float *)p;
> +        case PRES_VT_DOUBLE: return *(double *)p;
> +        case PRES_VT_INT   : return *(int *)p;
> +        case PRES_VT_BOOL  : return *(BOOL *)p ? 1.0f : 0.0f;

The 'f' suffix is now wrong since you're returning double. Something
like "return !!*(BOOL *)p;" would be probably preferable, although I
don't think you're ever supposed to read from PRES_VT_INT or
PRES_VT_BOOL tables so maybe replacing the last two cases with a FIXME
or similar is even better.

> +static void regstore_set_double(struct d3dx_regstore *rs, unsigned int table, unsigned int offset, double v)
> +{
> +    BYTE *p;
> +    unsigned int reg_idx;
> +
> +    p = (BYTE *)rs->tables[table] + table_info[table].component_size * offset;
> +    switch (table_info[table].type)
> +    {
> +        case PRES_VT_FLOAT : *(float *)p = v; break;
> +        case PRES_VT_DOUBLE: *(double *)p = v; break;
> +        case PRES_VT_INT   : *(int *)p = roundf(v); break;

Same here. Also it's not clear that rounding away from 0 is correct,
this would need tests (although IIRC preshaders generated by the MS
compiler explicitly compute integral values for integer constants,
that would make this hard to test and mostly irrelevant in practice).
So please try to test for rounding behavior with 0.5 / -0.5 / 1.5, if
nothing else to confirm that my memory serves me right. BTW unless
testing proves otherwise lrint() is probably the best option here
since it generates much nicer code (at least when building with gcc
for 32 bits).

> +        case PRES_VT_BOOL  : *(BOOL *)p = !!v; break;

The !! here might also be unnecessary strictly speaking but this isn't
quite as significant and it's okay to me either way.

> +    transpose = (desc.Class == D3DXPC_MATRIX_COLUMNS && param->class == D3DXPC_MATRIX_ROWS) ||
> +            (param->class == D3DXPC_MATRIX_COLUMNS && desc.Class == D3DXPC_MATRIX_ROWS);

It looks like you forgot to move the operator at the start of the next
line here (while you did that e.g. in patch 6/6).