On Tue, May 31, 2011 at 2:30 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell michael@mcdonnell.dk wrote:
On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
2011/5/30 Michael Mc Donnell michael@mcdonnell.dk:
Thanks for the comments Matteo! I have a few questions below.
On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
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:
That sounds great :-)
- testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
We usually use something like HeapAlloc(GetProcessHeap(), 0, sizeof(*testContext)); instead.
Ok, I guess that's better in case the type of the variable changes at some point?
Yes, that's a reason. The other one is simply consistency.
Ok, I've changed that.
I think you can remove some obvious comments (like /* Create the test mesh */). Also, check your whitespaces.
Is it too much or too little whitespace? Could you give me an example?
todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "
"got %#x expected D3D_OK\n", hr);
Here you put 4 spaces on the line continuation, which is neither the "align to be below the '(' + 1 space" used in most of the file, nor the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a nice and consistent coding style (I'm sure you noticed that), so that's mostly trying to not mix even more style variants...
Ok I've changed it to the "align to be below the '(' + 1 space" convention.
There was also something wrong with spaces in a comment that caught my eye, I think.
The only thing that stood out to me was that I had extra newlines in my multi-line comments. I've removed those.
I told you I was nitpicking... :)
That's ok, I prefer consistency too :-)
Oops the patches I sent earlier had a tab character and some superfluous whitespace.
Henri, do you have any comments about my UpdateSemantics patches before I send them off to wine-patches?
Thanks