Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

[Michael Mc Donnell]

Thanks for the lengthy feedback!

On Tue, May 24, 2011 at 3:36 PM, Stefan Dösinger <stefandoesinger at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-test-Added-UpdateSemanticsTest.patch
Type: text/x-patch
Size: 7837 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110524/19e127b0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-d3dx9-Implemented-UpdateSemantics-mesh-method.patch
Type: text/x-patch
Size: 3732 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110524/19e127b0/attachment-0001.bin>