Re: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon

[Matteo Bruni]

2011/11/7 David Adam :
> Hello,
>
> any problem with this patch
> http://source.winehq.org/patches/data/80433
>
> and this one
> http://source.winehq.org/patches/data/80434
>
> Thanks in advance
>
> David
>
> ---------- Forwarded message ----------
> From: David Adam 
> Date: 2011/10/30
> Subject: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon
> To: wine-patches
>
> Fix a possible crash when calling D3DXCreateMeshFVF.
>
> A+
>
> David

Hello David,

I see Rico preceded me (his points are very much valid too, and I'm
going to overlap a bit), but as I have already written a reply
anyway...

I think your patches generally look somewhat ugly. Many of the
comments I gave in
http://www.winehq.org/pipermail/wine-devel/2011-March/089173.html for
another patch of yours apply here all the same.

About specific issues I have noticed here:

Patch 1:

+    hr = D3DXCreateMeshFVF((DWORD)sides, (DWORD)(sides + 1),
D3DXMESH_MANAGED, D3DFVF_XYZ | D3DFVF_NORMAL, device, &polygon);

Why those casts?

+    vertice = HeapAlloc(GetProcessHeap(), 0, 2 * (sides + 1) *
sizeof(D3DXVECTOR3));
...
+    memcpy(data, vertice, 2 * (sides + 1) * sizeof(D3DXVECTOR3));

What's the point in allocating and filling this additional array? You
should just write into the vertex buffer. Same with the index buffer.

+    (polygon)->lpVtbl->UnlockVertexBuffer(polygon);

those parentheses around polygon are useless.

Patch 2:

+    hr = D3DXCreateBuffer(11 * sizeof(DWORD), &ppBuffer);

Why are you creating a D3DXBuffer here? It is probably created and
returned by D3DXCreatePolygon (and indeed your own implementation in
patch 1 does exactly that).
If you want to have the cleanup work even when the tests fail, you
have to protect the Release() calls somehow (e.g. initializing the
object pointers to NULL and checking for != NULL on release).

+                index = *( (WORD*)( data + ( 3 * i + j ) * sizeof(WORD) ) );

You could make that much simpler by introducing a WORD pointer.
Similarly for the vertex data.

I guess the objection about expecting a specific vertex/index ordering
is moot as it was in D3DXCreateBox case (i.e. there are applications
blindly modifying the mesh), right?