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.