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

[Matteo Bruni]

Hi Paul,

I have a few more comments, in addition to Henri's (BTW thank you for
taking a look).

2016-03-09 14:38 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> dlls/d3dx9_36/tests/effect.c | 755 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 755 insertions(+)
>
> diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> index a785f4d..8c5b5bb 100644
> --- a/dlls/d3dx9_36/tests/effect.c
> +++ b/dlls/d3dx9_36/tests/effect.c
> @@ -3009,6 +3009,760 @@ static void test_effect_states(IDirect3DDevice9 *device)
>      ok(value == 3, "Got result %u, expected %u.\n", value, 1);
>  }
>
> +/*
> -
> - fxc.exe /Tfx_2_0
> - */
>
> +#if 0
> +float4 g_Pos1;
> +float4 g_Pos2;
> +float4 g_Selector[2] = {{0, 0, 0, 0}, {10, 10, 10, 10}};
> -
>
> +float4 opvect1 = {0.0, -0.0, -2.2, 3.402823466e+38F};
> +float4 opvect2 = {1.0, 2.0, -3.0, 4.0};
> -
>
> +int4 g_iVect = {4, 3, 2, 1};
> -
>
> +vertexshader vs_arr[3] =
> +{
> - asm
> - {
> - vs_1_0
> - def c0, 1, 1, 1, 1
> - mov oPos, c0
> - },

Indentation.
> - asm
> - {
> - vs_1_1
> - def c0, 2, 2, 2, 2
> - mov oPos, c0
> - },
> - asm
> - {
> - vs_2_0
> - def c0, 3, 3, 3, 3
> - mov oPos, c0
> - }
>
> +};
> -
>
> +row_major float4x3 m4x3row={{11, 12, 13}, {21, 22, 23}, {31, 32, 33}, {41, 42, 43}};
> +row_major float3x4 m3x4row={{11, 12, 13, 14}, {21, 22, 23, 24}, {31, 32, 33, 34}};
> +column_major float4x3 m4x3column={{11, 12, 13},{21, 22, 23},{31, 32, 33},{41, 42, 43}};
> +column_major float3x4 m3x4column={{11, 12, 13, 14}, {21, 22, 23, 24}, {31, 32, 33, 34}};
> +row_major float2x2 m2x2row={{11, 12}, {21, 22}};
> +column_major float2x2 m2x2column={{11, 12}, {21, 22}};
> +row_major float2x3 m2x3row={{11, 12, 13}, {21, 22, 23}};
> +column_major float2x3 m2x3column={{11, 12, 13}, {21, 22, 23}};
> +row_major float3x2 m3x2row={{11, 12}, {21, 22}, {31, 32}};
> +column_major float3x2 m3x2column={{11, 12}, {21, 22}, {31, 32}};

Formatting.
> -
>
> +row_major bool2x3 mb2x3row = { {true, false, true}, {false, true, true}};
> +column_major bool2x3 mb2x3column = { {true, false, true}, {false, true, true}};
> -
>
> +struct VS_OUTPUT
> +{
> - float4 Position   : POSITION;   // vertex position
> - float2 TextureUV  : TEXCOORD0;  // vertex texture coords
> - float4 Diffuse    : COLOR0;     // vertex diffuse color
>
> +};

Those comments don't seem particularly useful.
> +static float test_effect_preshader_fconstsp[][4] =
> +{
> - {11.0, 21.0, 0.0 , 0.0 },

Numbers are usually right-aligned. Also those need the 'f' suffix.
The whitespace before the '}' seems wrong.
> +static BOOL test_effect_preshader_bconsts[] =
> +{
> - 1, 0, 1, 0, 1, 0
>
> +};

If you're using BOOL, you should also use TRUE and FALSE.
> +static void test_effect_preshader(IDirect3DDevice9 *device)
> +{
> - const static D3DXVECTOR4 fvect1 = {28.0f, 29.0f, 30.0f, 31.0f};
> - const static D3DXVECTOR4 fvect2 = {0.0f, 0.0f, 1.0f, 0.0f};
> - const static float fvect_empty[] = {-9999.0f, -9999.0f, -9999.0f, -9999.0f};
> - HRESULT hr;
> - ID3DXEffect *effect;
> - D3DXHANDLE par;
> - UINT npasses;
> - BOOL bval;
> - D3DLIGHT9 light;
> - DWORD value;
> - float fdata[TEST_EFFECT_PRES_NFLOATMAX * 4];
> - BOOL bdata[TEST_EFFECT_PRES_NBOOL];
> - IDirect3DVertexShader9 *vshader;
> - void *byte_code;
> - UINT byte_code_size;

Just use unsigned int instead of UINT or DWORD IMO.
> -
> - unsigned int i, j;
> -
> - hr = D3DXCreateEffect(device, test_effect_preshader_effect_blob, sizeof(test_effect_preshader_effect_blob),
> -
>        NULL, NULL, 0, NULL, &effect, NULL);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> -
> - for (i = 0; i < TEST_EFFECT_PRES_NFLOATV; i++)
> - {
> -
>    hr = IDirect3DDevice9_SetVertexShaderConstantF(device, 0, fvect_empty, 1);
> -
>    ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - }

That is repeatedly setting c0, right? I don't think that's what you
meant to do here.
Regardless, you probably want to initialize all 256 float constant registers.
> - hr = effect->lpVtbl->Begin(effect, &npasses, 0);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\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);

Typo. More of the same below.
> - for (i = 0; i < TEST_EFFECT_PRES_NOPTESTS; i++)
> - {
> -
>    DWORD *v;
> -
> -
>    hr = IDirect3DDevice9_GetLight(device, i % 8, &light);
> -
>    v = i < 8 ? (DWORD *)&light.Diffuse : (DWORD *)&light.Ambient;
> -
>    ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> -
>    if (i < TEST_EFFECT_PRES_NOPTESTS)

This seems either wrong or unnecessary.