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 <gofmanp(a)gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>
> ---
>  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.