On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Saturday 28 May 2011 19:55:55 you wrote:
It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)?
Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly.
- if (FAILED(hr))
- new_vertex_declaration = NULL;
- /* Free old vertex declaration */
- if (This->vertex_declaration)
- IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
- /* Set the position type to color instead of float3 (invalid
declaration) */
- hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, "
- "got %#x expected D3D_OK\n", hr);
Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
2011/5/29 Michael Mc Donnell michael@mcdonnell.dk:
On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
On Saturday 28 May 2011 19:55:55 you wrote:
It was implemented by setting the vertex declaration to null when the declaration is invalid. That seems to match what happens on Windows, because if a program has set an invalid declaration and then calls GetDeclaration or DrawSubset it will fail with an access violation. Should Wine also fail with an access violation or should it be slightly more graceful and return E_FAIL instead (which my new patch does)?
Yes, I think returning E_FAIL is a good idea, but write a WARN if CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not be shown by default(unlike a FIXME), it is used when the app does something fishy but you think you handled it correctly.
- if (FAILED(hr))
- new_vertex_declaration = NULL;
- /* Free old vertex declaration */
- if (This->vertex_declaration)
- IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
In this situation you can just release the old decl before calling CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. That simplified the code a bit. Don't forget to set This->vertex_declaration to NULL in case of an error though
Ok I've simplified the code. I've also inserted three WARNs. I've skipped a WARN when freeing the mesh because that works fine on Windows with an invalid declaration.
- /* Set the position type to color instead of float3 (invalid
declaration) */
- hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color);
- todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, "
- "got %#x expected D3D_OK\n", hr);
Is this really invalid? I don't know it on top of my head, but in general types and usage are independent, especially with shaders. I don't see a check in d3d9 or wined3d for this case.
You're right it isn't invalid. It doesn't crash, it just doesn't draw anything. I've change that comment and re-ordered the tests.
Beyond that I think the patches look OK. I'd recommend to send them to wine- devel one more time and then next week to wine-patches. Henri is on Vacation this week, and he's usually more picky than I am. I also hope some of the people who regularly contribute to d3dx9 will have a look and comment.
Ok, I've attached the newest version. I'll let the patches sit here for a week, and start working on the other functions in the mean time.
The patches are OK with me, just some nitpicking:
+ testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
-
Matteo Bruni -
Michael Mc Donnell