Re: [PATCH 1/5] d3d8/tests: Add cnd instruction test.

[Henri Verbeet]

On 2 September 2013 23:04, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> +    float quad1[] = {
> +        -1.0,   -1.0,   0.1,     0.0,    0.0,    1.0,
> +         0.0,   -1.0,   0.1,     1.0,    0.0,    1.0,
> +        -1.0,    0.0,   0.1,     0.0,    1.0,    0.0,
> +         0.0,    0.0,   0.1,     1.0,    1.0,    0.0
> +    };
> I think that should be static const, and could do with an 'f' suffix
> on the literals.
> +    ok(hr == D3D_OK, "IDirect3DDevice8_BeginScene returned %08x\n", hr);
> +    if(SUCCEEDED(hr))
> +    {
> I think the if block is pointless.
> +    /* 1.1 shader. All 3 components get set, based on the .w comparison */
> +    color = getPixelColor(device, 158, 358);
> +    ok(color == 0x00ffffff, "pixel 158, 358 has color %08x, expected 0x00ffffff\n", color);
> +    color = getPixelColor(device, 162, 358);
> +    ok( (((color & 0x00ff0000) >> 16) <= 0x01) && (((color & 0x0000ff00) >> 8) <= 0x01) && ((color & 0x000000ff) <= 0x01),
> +        "pixel 162, 358 has color %08x, expected 0x00000000\n", color);
> I think that should use color_match().

I realize those are all present in the original test, but that's more
of a reason to fix the original instead of copying it.

[PATCH 1/5] d3d8/tests: Add cnd instruction test.

[Matteo Bruni]

2013/9/3 Henri Verbeet <hverbeet(a)gmail.com>:
> On 2 September 2013 23:04, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> > +    float quad1[] = {
> > +        -1.0,   -1.0,   0.1,     0.0,    0.0,    1.0,
> > +         0.0,   -1.0,   0.1,     1.0,    0.0,    1.0,
> > +        -1.0,    0.0,   0.1,     0.0,    1.0,    0.0,
> > +         0.0,    0.0,   0.1,     1.0,    1.0,    0.0
> > +    };
> > I think that should be static const, and could do with an 'f' suffix
> > on the literals.
> > +    ok(hr == D3D_OK, "IDirect3DDevice8_BeginScene returned %08x\n", hr);
> > +    if(SUCCEEDED(hr))
> > +    {
> > I think the if block is pointless.
> > +    /* 1.1 shader. All 3 components get set, based on the .w comparison */
> > +    color = getPixelColor(device, 158, 358);
> > +    ok(color == 0x00ffffff, "pixel 158, 358 has color %08x, expected 0x00ffffff\n", color);
> > +    color = getPixelColor(device, 162, 358);
> > +    ok( (((color & 0x00ff0000) >> 16) <= 0x01) && (((color & 0x0000ff00) >> 8) <= 0x01) && ((color & 0x000000ff) <= 0x01),
> > +        "pixel 162, 358 has color %08x, expected 0x00000000\n", color);
> > I think that should use color_match().
>
> I realize those are all present in the original test, but that's more
> of a reason to fix the original instead of copying it.

Totally fair points. I trusted my cleanup of the d3d9 test way too
much (also rereading the patch with some more attention would have
probably helped).

Resent, hopefully I caught all the issues. I'll cleanup the d3d9 test
as a followup.