2011/5/29 Michael Mc Donnell <mich...@mcdonnell.dk>: > On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger > <stefandoesin...@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.
