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

[Matteo Bruni]

2016-03-10 15:23 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman

Looks much better, thanks. I still have a few comments though.
> - for (i = 0; i < 256; i++)
> - {
> -
>    hr = IDirect3DDevice9_SetVertexShaderConstantF(device, i,
> -
>            (const float *)&fvect_empty, 1);
> -
>    ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - }
> - for (i = 0; i < TEST_EFFECT_PRES_NFLOATP; i++)
> - {
> -
>    hr = IDirect3DDevice9_SetPixelShaderConstantF(device, i,
> -
>            (const float *)&fvect_empty, 1);
> -
>    ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - }

I know it was me who suggested to clear all the 256 float shader
constants but it looks a bit weird if the same doesn't happen for the
other constant types. It's also not very useful if those additional
constants aren't checked later on. I would change the initialization
of the other constant types in the same way (maybe with a define or
something instead of the plain number?) and then check that those are
not modified by BeginPass().
> - bval = FALSE;
> - for (i = 0; i < TEST_EFFECT_PRES_NBOOL; i++)
> - {
> -
>    hr = IDirect3DDevice9_SetPixelShaderConstantB(device, i, &bval, 1);
> -
>    ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - }
> -
> - hr = effect->lpVtbl->Begin(effect, &npasses, 0);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> -
> - par = effect->lpVtbl->GetParameterByName(effect, NULL, "g_Pos2");
> - ok(par != NULL, "GetParameterByName failed.\n");
> -
> - hr = effect->lpVtbl->SetVector(effect, par, &fvect1);
> - ok(hr == D3D_OK, "SetFloatArray failed, hr %#x.\n", hr);

This message is still wrong.
> -
> - hr = effect->lpVtbl->BeginPass(effect, 0);
> - todo_wine ok(hr == D3D_OK, "Got result %#x.\n", hr);
> -
> - hr = IDirect3DDevice9_GetVertexShaderConstantF(device, 0, (float *)fdata, TEST_EFFECT_PRES_NFLOATV);

We usually avoid the cast in these cases with something like "&fdata->x".
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - todo_wine ok(!memcmp(fdata, test_effect_preshader_fconstsv, sizeof(test_effect_preshader_fconstsv)),
> -
>        "Vertex shader float constants do not match.\n");
> - hr = IDirect3DDevice9_GetPixelShaderConstantF(device, 0, (float *)fdata, TEST_EFFECT_PRES_NFLOATP);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - todo_wine ok(!memcmp(fdata, test_effect_preshader_fconstsp, sizeof(test_effect_preshader_fconstsp)),
> -
>        "Pixel shader float constants do not match.\n");
> -
> - hr = IDirect3DDevice9_GetPixelShaderConstantB(device, 0, bdata, TEST_EFFECT_PRES_NBOOL);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - for (i = 0; i < TEST_EFFECT_PRES_NBOOL; i++)
> -
>    todo_wine_if(!bdata[i] != !test_effect_preshader_bconsts[i])
> -
>    ok(!bdata[i] == !test_effect_preshader_bconsts[i],
> -
>            "Pixel shader boolean constants do not match.\n");
> -
> - 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.\n",
> -
>                test_effect_preshader_op_results[i].comment, j,
> -
>                test_effect_preshader_op_results[i].result[j], v[j]);

The indentation is pretty weird here. Even if you keep the ok on the
same column of the todo_wine_if (to make its removal a few patches
later nicer) the continuations of the ok condition should be indented,
matching the other continuation lines.
FWIW in newer d3d code we usually put the operators at the start of
the next line (instead of at the end of the previous line) in line
continuations. Also we prefer preincrement to postincrement of typical
'for' induction variables.
> - ok(!!vshader, "Got NULL vshader.\n");

This double negation is unnecessary.
> -
> - hr = IDirect3DVertexShader9_GetFunction(vshader, NULL, &byte_code_size);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - byte_code = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, byte_code_size);
> - hr = IDirect3DVertexShader9_GetFunction(vshader, byte_code, &byte_code_size);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - ok(byte_code_size > 1, "Got unexpected byte code size %u.\n", byte_code_size);
> - 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),
> -
>        "Incorrect shader selected.\n");
> - HeapFree(GetProcessHeap(), 0, byte_code);
> - IDirect3DVertexShader9_Release(vshader);
> -
> - hr = IDirect3DDevice9_SetVertexShader(device, NULL);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> -
> - hr = effect->lpVtbl->SetVector(effect, par, &fvect1);
> - ok(hr == D3D_OK, "SetFloatArray failed, hr %#x.\n", hr);
> - hr = effect->lpVtbl->CommitChanges(effect);
> - todo_wine ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - hr = IDirect3DDevice9_GetVertexShader(device, &vshader);
> - ok(hr == D3D_OK, "Got result %#x.\n", hr);
> - ok(!vshader, "Incorrect shader selected.\n");
> - if (vshader)
> -
>    IDirect3DVertexShader9_Release(vshader);

I think you can remove this release too.

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

[Paul Gofman]

Thanks, I will follow up and resend the patch series on Thursday, 17th.
I have a few questions below, could you please look?

On 03/15/2016 05:50 AM, Matteo Bruni wrote:
> > - for (i = 0; i < TEST_EFFECT_PRES_NFLOATP; i++)
> > - {
> > -
> >    hr = IDirect3DDevice9_SetPixelShaderConstantF(device, i,
> > -
> >            (const float *)&fvect_empty, 1);
> > -
> >    ok(hr == D3D_OK, "Got result %#x.\n", hr);
> > - }
>
> I know it was me who suggested to clear all the 256 float shader
> constants but it looks a bit weird if the same doesn't happen for the
> other constant types. It's also not very useful if those additional
> constants aren't checked later on. I would change the initialization
> of the other constant types in the same way (maybe with a define or
> something instead of the plain number?) and then check that those are
> not modified by BeginPass().

I can't just set 256 pixel shader constants, that fails on Marvin
testbot (that was my unlucky v2 patch series). So I probably need to get
the actual number of constants supported through GetDeviceCaps. Do we
really need this in this test, or maybe I can just check the number of
constants used + 1 for all types of constants?
> > - ok(!!vshader, "Got NULL vshader.\n");
>
> This double negation is unnecessary.

There was a compiler warning if just "ok(vshader)". Or should I get it
back as ok(vshader == NULL)?

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

[Matteo Bruni]

2016-03-15 8:18 GMT+01:00 Paul Gofman :
> Thanks, I will follow up and resend the patch series on Thursday, 17th.
> I have a few questions below, could you please look?
>
> On 03/15/2016 05:50 AM, Matteo Bruni wrote:
> > > - for (i = 0; i < TEST_EFFECT_PRES_NFLOATP; i++)
> > > - {
> > > -
> > >    hr = IDirect3DDevice9_SetPixelShaderConstantF(device, i,
> > > -
> > >            (const float *)&fvect_empty, 1);
> > > -
> > >    ok(hr == D3D_OK, "Got result %#x.\n", hr);
> > > - }
> >
> > I know it was me who suggested to clear all the 256 float shader
> > constants but it looks a bit weird if the same doesn't happen for the
> > other constant types. It's also not very useful if those additional
> > constants aren't checked later on. I would change the initialization
> > of the other constant types in the same way (maybe with a define or
> > something instead of the plain number?) and then check that those are
> > not modified by BeginPass().
>
> I can't just set 256 pixel shader constants, that fails on Marvin
> testbot (that was my unlucky v2 patch series). So I probably need to get
> the actual number of constants supported through GetDeviceCaps. Do we
> really need this in this test, or maybe I can just check the number of
> constants used + 1 for all types of constants?

Oh, I see, that's because SM3 pixel shaders provide 224 float
constants. But yes, this needs some more care. You should probably
skip the test entirely if vertex or pixel shader 3 aren't supported,
then you can initialize and check all the constants.
> > > - ok(!!vshader, "Got NULL vshader.\n");
> >
> > This double negation is unnecessary.
>
> There was a compiler warning if just "ok(vshader)". Or should I get it
> back as ok(vshader == NULL)?

Uh, you're right. Just ignore my comment then.