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

[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 <david.adam.cnrs(a)gmail.com>
Date: 2011/10/30
Subject: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon
To: wine-patches <wine-patches(a)winehq.org>


Fix a possible crash when calling D3DXCreateMeshFVF.

A+

David

[Rico Schüller]

Hi David,

Impl:
- Probably you may unlock and free the polygon when allocating the 
vertice/index fail, why do you need the vertice at all? Couldn't you 
just write directly to "data", since you're holding the lock anyway?
- if locking fails, you're leaking the polygon memory, this happens for 
nearly all error checks
- the TRACE shouldn't contain "stub"
- there are some style issues

Test:
- it may be nice to test if polygon or ppBuffer are set to NULL on hr != 
D3D_OK or if they are untouched
- Why are you creating a buffer ppBuffer at all? From your 
implementation it looks like it is done when using adjacency? Also I'd 
expect, when D3DXCreateBuffer fails, ppBuffer is NULL and the release at 
the end label would crash, even if it still has uninitialized memory.
- there is an unlock for the vertexbuffer missing and you may check the 
return value from the unlock call
- there are some copy and paste mistakes
- I think a failing Lock*Buffer should also get a failing test, I 
wouldn't just print the skip.
- I'd check the count from all Release calls, that way, you'd see if you 
forgot to release something

Hope this helps
Rico

Am 07.11.2011 20:58, schrieb 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<david.adam.cnrs(a)gmail.com>
> Date: 2011/10/30
> Subject: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon
> To: wine-patches<wine-patches(a)winehq.org>
>
> Fix a possible crash when calling D3DXCreateMeshFVF.
>
> A+
>
> David

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

[Matteo Bruni]

2011/11/7 David Adam <david.adam.cnrs(a)gmail.com>:
> 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 <david.adam.cnrs(a)gmail.com>
> Date: 2011/10/30
> Subject: d3dx9_36 [patch 1/2, resent]: Implement D3DXCreatePolygon
> To: wine-patches <wine-patches(a)winehq.org>
>
> 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?