Hi,
Design-wise I think the 8 triangles and readback grid make the test fairly difficult to read. You could set up a structure that contains the d3d parameters you're changing between the tests and the expected color and then loop over the table. Something like this:
const struct { BOOL d3drs_lighting; DWORD dp_flags; <- for things like D3DP_DONOTLIGHT DWORD fvf; void *vertices; DWORD expected_color; } test_data[] = { ... };
You can also think about sharing this control table between d3d2, d3d3 and d3d7 by having different color fields for that, but I don't know how well this will work.
On Monday 07 November 2011 23:27:13 Octavian Voicu wrote:
- DWORD expected[] = { GRAY, WHITE, YELLOW2, YELLOW, GREEN, GREEN, BLUE,
BLUE }; Please make data const where you can.
/* Note: IDirect3DDevice3::DrawPrimitive calls with D3DVT_ vertextypes should fail. */ +
/* Triangle 0 -- D3DFVF_VERTEX (vertices with normals, but withoutcolors). */ + hr = IDirect3DDevice3_DrawPrimitive(Direct3DDevice3, D3DPT_TRIANGLELIST, D3DVT_VERTEX,
vertices, 3, D3DDP_WAIT);ok(hr != DDERR_INVALIDPARAMS, "DrawPrimitive returned: %08x\n",hr);
I don't think there's a point in testing this, both D3DVT_* and D3DFVF_* are defines / enums that map to integers, and it is obvious that you get wrong behavior if you mix those.
The ok condition looks strange. If you're trying to avoid a todo_wine, don't do that, just mark tests that show a difference between Wine and Windows as todo_wine. (I guess Wine currently returns DD_OK, and Windows returns D3DERR_INVALIDVERTEXFORMAT)
- /* Make sure getPixelColor works correctly. */
- color = getPixelColor(device, 320, 240);
- ok(color == RED, "getPixelColor returned: %08x\n", color);
We have some sanity checks for that in the main function
Most of this applies to the d3d2 and d3d3 tests as well. I haven't looked over the cleanup patches yet.
[resent to include wine-devel]
On Tue, Nov 8, 2011 at 1:23 AM, Stefan Dösinger stefandoesinger@gmx.at wrote:
Design-wise I think the 8 triangles and readback grid make the test fairly difficult to read. You could set up a structure that contains the d3d parameters you're changing between the tests and the expected color and then loop over the table. Something like this:
const struct { BOOL d3drs_lighting; DWORD dp_flags; <- for things like D3DP_DONOTLIGHT DWORD fvf; void *vertices; DWORD expected_color; } test_data[] = { ... };
You can also think about sharing this control table between d3d2, d3d3 and d3d7 by having different color fields for that, but I don't know how well this will work.
Henry wasn't very happy about this either. Your fix sounds interesting, I'm going to give it a try.
The idea behind using triangles was to fit everything in one screen so I could debug it visually very easily. I'm thinking I can still have that if I draw full-screen quads in a loop, like you suggest, then blit one small rectangle to the primary surface, in different positions for each test.
On Monday 07 November 2011 23:27:13 Octavian Voicu wrote:
- DWORD expected[] = { GRAY, WHITE, YELLOW2, YELLOW, GREEN, GREEN, BLUE,
BLUE }; Please make data const where you can.
Point taken.
- /* Note: IDirect3DDevice3::DrawPrimitive calls with D3DVT_ vertex
types should fail. */ +
- /* Triangle 0 -- D3DFVF_VERTEX (vertices with normals, but without
colors). */ + hr = IDirect3DDevice3_DrawPrimitive(Direct3DDevice3, D3DPT_TRIANGLELIST, D3DVT_VERTEX,
- vertices, 3, D3DDP_WAIT);
- ok(hr != DDERR_INVALIDPARAMS, "DrawPrimitive returned: %08x\n",
hr);
I don't think there's a point in testing this, both D3DVT_* and D3DFVF_* are defines / enums that map to integers, and it is obvious that you get wrong behavior if you mix those.
The ok condition looks strange. If you're trying to avoid a todo_wine, don't do that, just mark tests that show a difference between Wine and Windows as todo_wine. (I guess Wine currently returns DD_OK, and Windows returns D3DERR_INVALIDVERTEXFORMAT)
DirectX SDK 6.1 docs says that "If you attempt to use one of the members of D3DVERTEXTYPE, the method fails, returning DDERR_INVALIDPARAMS", so I wanted to test exactly that. I accidentally wrote "!=" instead of "==" and forgot about it since Windows did not complain (SDK is probably inaccurate), nor did Wine. Will look into it and probably write some separate tests, outside the test loop. I think we should cover special error cases if there are any (that is, if D3DERR_INVALIDVERTEXTYPE is not returned for D3DVT_ constants).
- /* Make sure getPixelColor works correctly. */
- color = getPixelColor(device, 320, 240);
- ok(color == RED, "getPixelColor returned: %08x\n", color);
We have some sanity checks for that in the main function
I know that, but I figured it didn't hurt to have an extra check there, since each D3D2, D3D3, and D3D7 test uses a different version of the getPixelColor function.
Most of this applies to the d3d2 and d3d3 tests as well. I haven't looked over the cleanup patches yet.
Thanks for the quick and thorough feedback, it was really helpful!
About the ddraw/visual tests: I think we need one structure for each of d3d, d3d2, d3d3, d3d7, where to have most of the things we store globally or in local vars now, and make the create/release functions work with that. With my cleanup patches I think it will be easier to refactor code to use such a structure. However, my patch stack was getting uncomfortably large and I wanted to commit it, so I can get to the good part -- fixing the game I'm debugging. I might look into such a refactoring later.
Octavian
-
Octavian Voicu -
Stefan Dösinger