2011/5/30 Michael Mc Donnell <mich...@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.myst...@gmail.com> > wrote: >> 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: > > 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. >> 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... There was also something wrong with spaces in a comment that caught my eye, I think. I told you I was nitpicking... :) > > Thanks! >