Thanks for the lengthy feedback!
On Tue, May 24, 2011 at 3:36 PM, Stefan Dösinger stefandoesinger@gmx.at wrote:
Hi, I'm at school right now, so I had only a very quick look. I'll take a closer look later in the evening.
On Tuesday 24 May 2011 14:38:19 Michael Mc Donnell wrote:
This is my first try at implementing the UpdateSemantics mesh method. It works fine here on my local machine, passes the new tests, and works with my little interactive demo(not included). I still have a few questions though.
This implementation allocates new heap memory and I suspect that it would be faster to re-use the already allocated memory.
As far as I can see this happens only in the tests, where it doesn't really matter. Creating and destroying the device is more expensive than the heap allocation, so if you care about performance you should reuse your test context structure
Sorry I phrased that in a wrong way. In the UpdateSemantics function I call IDirect3DDevice9_CreateVertexDeclaration which allocates a new Vertex Declaration on the heap. I think it might slow things down if UpdateSemantics is in a tight loop where it is used for animation.
I also noticed that I didn't call IDirect3DDevice9_SetVertexDeclaration. I've added it to stick with the defined locking scheme.
Btw should I roll all or some of the patches up into a single patch?
I think you can merge the 3 test patches into one
Ok, I've rolled the three test patches into one.
Any other comments about the patches in general?
*)
- Copyright (C) 2011 Google (Michael Mc Donnell)
Afaik the gsoc conditions say that you keep the copyright to your code :-)
Yes you are right. First paragraph in the faq about code :-)
*) In your first test you forgot to check the HeapAlloc result.
Ok, I'll return E_OUTOFMEMORY in that case.
*) You could write a test that passes an invalid D3DVERTEXELEMENT9 array to UpdateSemantics to see how it handles an invalid declaration.
I've already got one test with an invalid D3DVERTEXELEMENT9 array. It happily accepts too big declarations even though msdn says "The call is valid only if the old and new declaration formats have the same vertex size". I've added test for null pointers, which it didn't handle correctly. Should I try to make some more invalid D3DVERTEXELEMENT9 arrays to see if I can provoke an error?
A few style suggestions:
*) memset(mem, 0, size) is preferred over ZeroMemory(mem, size);
Ok
*) In the wined3d code(and its client libs) Henri and I avoid structure typedefs. The existing d3dx9 code has a few of them. I'm ok with either way, but maybe you and the other d3dx9 devs want to go the wined3d way.
Sure I can change them. So just normal structs? Is it to keep the namespace clean?
*) In the TestContext structure you can use gotos for error handling, for example:
void *ptr1 = NULL, *ptr2 = NULL, *ptr3 = NULL;
ptr1 = HeapAlloc(...); if(!ptr1) goto err; ptr2 = HeapAlloc(...); if(!ptr2) goto err; ptr3 = HeapAlloc(...); if(!ptr3) goto err;
return success;
err: HeapFree(ptr3); HeapFree(ptr2); HeapFree(ptr1); return error;
That avoids ever-growing if (FAILED(hr)) blocks with repeated code.
Ok I've changed NewTestContext to use gotos instead. I've also changed it so that it returns an HRESULT instead of a pointer to the new structure.
I've attached the new versions of the patches.