From: Henri Verbeet <hverbeet@gmail.com>
Subject: Re: [PATCH v3 6/6] d3d9/tests: Add Fetch4 tests
Message-Id: <CAOsNvwxq8NmX=GiZHkjcBamxW=B2B_S11PoXy+18ChmggDpuMw@mail.gmail.com>
Date: Tue, 27 Nov 2018 23:02:02 +0330
In-Reply-To: <20181124201337.20330-6-mailszeros@gmail.com>
References: <20181122012823.15966-1-mailszeros@gmail.com> <20181124201337.20330-1-mailszeros@gmail.com> <20181124201337.20330-6-mailszeros@gmail.com>

On Sat, 24 Nov 2018 at 23:51, Daniel Ansorregui <mailszeros@gmail.com> wrote:
> - Implemented for texld/texldp/texldd/texldb/texldl
> - In all cases tested on Windows10 + Intel
>   Fetch4 enabled always produced same result (like texld)
>
> Signed-off-by: Daniel Ansorregui <mailszeros@gmail.com>
> ---
>  dlls/d3d9/tests/visual.c | 313 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 313 insertions(+)
>
Since the CheckDeviceFormat() check will prevent this from doing
anything on implementations that don't support DF24, you may as well
send this test as the first patch in the series.

> +    struct
> +    {
> +        float x, y, z;
> +        float tu, tv;
> +    }
Not a big deal, but we tend to do these as "struct vec3 position" and
"struct vec2 texcoord".

> +    struct struct_expected_color
> +    {
> +        UINT x, y;
> +        D3DCOLOR color;
> +    };
Why the "struct_" prefix on the structure name? In fact, you don't
really need a structure name at all.

> +    static const DWORD texture_data[4] = {0x10111213,
Again minor, but no need for the "4".

> +    IDirect3DTexture9 *texture_L8, *texture_A8, *texture_A8R8G8B8;
You'll probably want an array of texture formats you want to test, and
loop over it.

> +    /* Create our texture for FETCH4 */
> +    hr = IDirect3DDevice9_CreateTexture(device, 4, 4, 1, 0, D3DFMT_L8, D3DPOOL_MANAGED, &texture_L8, NULL);
> +    ok(hr == D3D_OK, "Failed to create texture, hr %#x.\n", hr);
> +    memset(&lr, 0, sizeof(lr));
The memset() is redundant.

> +    hr = IDirect3DTexture9_LockRect(texture_L8, 0, &lr, NULL, 0);
> +    ok(hr == D3D_OK, "Failed to lock texture, hr %#x.\n", hr);
> +    memcpy(lr.pBits, texture_data, sizeof(texture_data));
> +    hr = IDirect3DTexture9_UnlockRect(texture_L8, 0);
> +    ok(hr == D3D_OK, "Failed to unlock texture, hr %#x.\n", hr);
This probably works in practice, but you should be using the pitch.

> +    /* Render with fetch4 and test if we obtain proper results */
> +    for (i=0; i<5; i++)
Formatting.

> +    {
> +        if (i==0)
> +            hr = IDirect3DDevice9_SetPixelShader(device, ps_texld);
> +        else if(i==1)
> +            hr = IDirect3DDevice9_SetPixelShader(device, ps_texldp);
> +        else if(i==2)
> +            hr = IDirect3DDevice9_SetPixelShader(device, ps_texldd);
> +        else if(i==3)
> +            hr = IDirect3DDevice9_SetPixelShader(device, ps_texldb);
> +        else
> +            hr = IDirect3DDevice9_SetPixelShader(device, ps_texldl);
> +        ok(SUCCEEDED(hr), "SetPixelShader failed, hr %#x.\n", hr);
If you create an array like this:

    static const DWORD *ps_code[] =
    {
        ps_code_texld,
        ps_code_texldp,
        ...
    };

You can loop over it, and avoid the fairly awkward construction above.