[v4 1/6] d3dx9: add test for preshader in effect.

[Matteo Bruni]

2016-03-17 12:59 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:

Nice job. I have a few nitpicks still...

> +static const D3DXVECTOR4 test_effect_preshader_fconstsv[] =
> +{
> +    {11.0f, 12.0f, 13.0f, 0.0f},
> +    {21.0f, 22.0f, 23.0f, 0.0f},
> +    {31.0f, 32.0f, 33.0f, 0.0f},
> +    {41.0f, 42.0f, 43.0f, 0.0f},
> +    {11.0f, 21.0f, 31.0f, 0.0f},
> +    {12.0f, 22.0f, 32.0f, 0.0f},
> +    {13.0f, 23.0f, 33.0f, 0.0f},
> +    {14.0f, 24.0f, 34.0f, 0.0f},
> +    {11.0f, 12.0f, 13.0f, 14.0f},
> +    {21.0f, 22.0f, 23.0f, 24.0f},
> +    {31.0f, 32.0f, 33.0f, 34.0f},
> +    {11.0f, 21.0f, 31.0f, 41.0f},
> +    {12.0f, 22.0f, 32.0f, 42.0f},
> +    {13.0f, 23.0f, 33.0f, 43.0f},
> +    {4.0f,   5.0f,  6.0f, 7.0f}
> +};
> +
> +static const D3DXVECTOR4 test_effect_preshader_fconstsp[] =
> +{
> +    {11.0f, 21.0f,  0.0f, 0.0f},
> +    {12.0f, 22.0f,  0.0f, 0.0f},
> +    {13.0f, 23.0f,  0.0f, 0.0f},
> +    {11.0f, 12.0f,  0.0f, 0.0f},
> +    {21.0f, 22.0f,  0.0f, 0.0f},
> +    {31.0f, 32.0f,  0.0f, 0.0f},
> +    {11.0f, 12.0f,  0.0f, 0.0f},
> +    {21.0f, 22.0f,  0.0f, 0.0f},
> +    {11.0f, 21.0f,  0.0f, 0.0f},
> +    {12.0f, 22.0f,  0.0f, 0.0f},
> +    {11.0f, 12.0f, 13.0f, 0.0f},
> +    {21.0f, 22.0f, 23.0f, 0.0f},
> +    {11.0f, 21.0f, 31.0f, 0.0f},
> +    {12.0f, 22.0f, 32.0f, 0.0f}
> +};
> +
> +static const BOOL test_effect_preshader_bconsts[] =
> +{
> +    TRUE, FALSE, TRUE, FALSE, TRUE, FALSE
> +};
> +
> +static const struct
> +{
> +    const char *comment;
> +    unsigned int todo[4];
> +    unsigned int result[4];
> +}
> +test_effect_preshader_op_results[] =
> +{
> +    {"1 / op", {1, 1, 1, 1}, {0x7f800000, 0xff800000, 0xbee8ba2e, 0x00200000}},
> +    {"rsq",    {1, 1, 3, 3}, {0x7f800000, 0x7f800000, 0x3f2c985d, 0x1f800000}},
> +    {"mul",    {1, 1, 1, 1}, {0x00000000, 0x80000000, 0x40d33334, 0x7f800000}},
> +    {"add",    {0, 1, 1, 1}, {0x3f800000, 0x40000000, 0xc0a66666, 0x7f7fffff}},
> +    {"lt",     {0, 0, 1, 0}, {0x3f800000, 0x3f800000, 0x00000000, 0x00000000}},
> +    {"ge",     {1, 1, 0, 1}, {0x00000000, 0x00000000, 0x3f800000, 0x3f800000}},
> +    {"neg",    {1, 1, 1, 1}, {0x80000000, 0x00000000, 0x400ccccd, 0xff7fffff}},
> +    {"rcp",    {1, 1, 1, 1}, {0x7f800000, 0xff800000, 0xbee8ba2e, 0x00200000}},
> +    {"frac",   {0, 0, 1, 0}, {0x00000000, 0x00000000, 0x3f4ccccc, 0x00000000}},
> +    {"min",    {0, 1, 1, 1}, {0x00000000, 0x80000000, 0xc0400000, 0x40800000}},
> +    {"max",    {1, 1, 1, 1}, {0x3f800000, 0x40000000, 0xc00ccccd, 0x7f7fffff}},
> +    {"sin",    {0, 1, 1, 3}, {0x00000000, 0x80000000, 0xbf4ef99e, 0xbf0599b3}},
> +    {"cos",    {1, 1, 1, 3}, {0x3f800000, 0x3f800000, 0xbf16a803, 0x3f5a5f96}},
> +    {"den mul",{1, 1, 1, 1}, {0x7f800000, 0xff800000, 0xbb94f209, 0x000051ec}},
> +    {"dot",    {0, 1, 1, 0}, {0x00000000, 0x7f800000, 0x41f00000, 0x00000000}},
> +    {"prec",   {1, 1, 2, 0}, {0x2b8cbccc, 0x2c0cbccc, 0x00000000, 0x00000000}}
> +};
> +
> +#define TEST_EFFECT_PRES_NFLOATV ARRAY_SIZE(test_effect_preshader_fconstsv)
> +#define TEST_EFFECT_PRES_NFLOATP ARRAY_SIZE(test_effect_preshader_fconstsp)
> +#define TEST_EFFECT_PRES_NFLOATMAX (TEST_EFFECT_PRES_NFLOATV > TEST_EFFECT_PRES_NFLOATP ? \
> +        TEST_EFFECT_PRES_NFLOATV : TEST_EFFECT_PRES_NFLOATP)
> +#define TEST_EFFECT_PRES_NBOOL ARRAY_SIZE(test_effect_preshader_bconsts)
> +#define TEST_EFFECT_PRES_NOPTESTS ARRAY_SIZE(test_effect_preshader_op_results)

Those arrays above should probably be inside test_effect_preshader(),
unless you plan to add other functions needing them.

> +    for (     ; i < 16; ++i)

Those spaces in place of the first "for" expression look weird. Just
leave that empty i.e.

    for (; i < 16; ++i)

> +    for (i = 0; i < TEST_EFFECT_PRES_NOPTESTS; ++i)
> +    {
> +        unsigned int *v;
> +
> +        hr = IDirect3DDevice9_GetLight(device, i % 8, &light);
> +        v = i < 8 ? (unsigned int *)&light.Diffuse : (unsigned int *)&light.Ambient;
> +        ok(hr == D3D_OK, "Got result %#x.\n", hr);
> +        for (j = 0; j < 4; ++j)
> +            todo_wine_if(test_effect_preshader_op_results[i].todo[j] & 1)
> +            ok(v[j] == test_effect_preshader_op_results[i].result[j]
> +                    || ((test_effect_preshader_op_results[i].todo[j] & 2)
> +                    && broken(v[j] != test_effect_preshader_op_results[i].result[j])),
> +                    "Operation %s, component %u, expected %#x, got %#x (%g).\n",
> +                    test_effect_preshader_op_results[i].comment, j,
> +                    test_effect_preshader_op_results[i].result[j], v[j], ((float *)v)[j]);

I don't like that broken(), it isn't checking much (i.e. the condition
is true for almost any value) and overloading the "todo" field for
that doesn't seem too clean.

Maybe something like this for the test results table would help:

static const struct
{
    const char *name;
    unsigned int expected[4];
    BOOL todo[4];
    unsigned int expected_broken[4];
}

I think all the broken results are != 0 so expected_broken also works
as a flag. If that's not the case you can add explicit "BOOL
broken[4]" flags.
You can then leave out the todo and expected_broken fields for the
table entries which don't require them, since they end up
zero-initialized.

Maybe also add a small comment for each broken value, if it makes sense?

> +    todo_wine ok(!memcmp(byte_code,
> +            &test_effect_preshader_effect_blob[TEST_EFFECT_PRESHADER_VSHADER_POS +
> +            1 * TEST_EFFECT_PRESHADER_VSHADER_LEN], byte_code_size),

You can leave out the "1 *", it's pretty clear either way.