Re: [1/2] d3dx9_36: Implement D3DXCreatePolygon.

[Matteo Bruni]

2014-11-03 6:35 GMT+01:00 Sebastian Lackner :
> Based on a patch by David Adam.
>
> For bug 13632.
>
> dlls/d3dx9_36/d3dx9_36.spec |  2 +-
>  dlls/d3dx9_36/mesh.c        | 89 +++++++++++++++++++++++++++++++++++++++++++++
>  include/d3dx9shape.h        |  2 +
>  3 files changed, 92 insertions(+), 1 deletion(-)

Hi Sebastian,

I have a few nitpicks:

+    TRACE("(%p, %f, %u, %p, %p)\n", device, length, sides, mesh, adjacency);

Generally you want that to be something like:

TRACE("device %p, length %f, sides %u, mesh %p, adjacency %p.\n",
device, length, sides, mesh, adjacency);

+    if (!device || length < 0.0f || sides == 0 || !mesh)

Probably "!sides" is more in the usual Wine d3d* style. Also, no need
to use brackets for single-line branches.

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

Line continuation should be indented by 8 blanks. Also, missing
whitespace after a comma.

+    scale = 0.5f * length / sin(D3DX_PI / sides);

You probably want to use sinf() here. Same below.

+    for (i = 0; i < sides; i++)

Not important, but the current wined3d style wants "++i" here.

+        for (i = 0; i < sides; i++)
+        {
+            adjacency_buf[i][0] = i - 1;
+            adjacency_buf[i][1] = -1;
+            adjacency_buf[i][2] = i + 1;
+        }

Really a detail, but adjacency_buf and i are both defined as unsigned
int, so either defining them as plain, signed int or using ~0 instead
of -1 would be nicer.

I haven't seen the test yet but in some other D3DX9 functions the
"out" pointers are set to NULL on error. If this function has the same
behavior you should probably add a "*mesh = NULL;" in the first few
lines of the function.

[1/2] d3dx9_36: Implement D3DXCreatePolygon.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-11-03 18:44, schrieb Matteo Bruni:
> Really a detail, but adjacency_buf and i are both defined as
> unsigned int, so either defining them as plain, signed int or using
> ~0 instead of -1 would be nicer.

Nitpicking the nitpick: ~0U.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUWLBTAAoJEN0/YqbEcdMwVPwP/jg8/7cWzsomx1epL0k/tZ99
8f7KliBkX2crOwEhVgshK081qMDN6y54udi/8szzOQ/Wt31QhNmLH3inVpzGVwfr
3u+v5jr5Bgdqsn935skYfLBc6cYmBtTz/TZDWoexu8CHL7mWMz2oEU1sgYkt3qku
ppLRUuURabZx3d5Dyk4TQQYe9/ySD8x8McVVM/mtW0oTTi4fto1KHpPjWpjRQswE
IEbw1F0DKa4ZRkOEW8v+SS7hP6+kYYbzBOSnHWD0DO+DVI8VBpOwv1VBK0GUV1Tp
S1sicnWFuk+Lq64k7YS0V9yIHmO2NF09MVOcWXei5h7BdteX+RUFM5ItGpb+IiyV
xq6wJW8IRkcQbmZWq2iiFdC7bOJJdfPW8EE/XrTIdIZ+c2VT0Z5RI1ey714XHHCb
TwGEUjF596dcvHXBC6x1LCr5JNSdJbHyMDNAlyiwpAwa1p7oT6h1vytU7nP3v6aM
33kYwsAdcZcG0GYPSHi9nJvgEEau4nMgYilN8a1mOh+S7iPAlue1r/TJgUlDcN2P
v3qeTFQxJxuMTP/zsDcJi/3wg0Kcu9GUq9+2u1jGmxHHGjd3JnPQ+kEovoIgfYNk
8YPb6oqWtRkNxN4Q+OQMi81VFD6is5w+NM5OqG5g9H99/jTRe11REtTQNNr6HcAK
SBh9WBaav7Lz71ZmH801
=/t7W
-----END PGP SIGNATURE-----

[1/2] d3dx9_36: Implement D3DXCreatePolygon.

[Dmitry Timoshkov]

Stefan Dösinger  wrote:
> Am 2014-11-03 18:44, schrieb Matteo Bruni:
> > Really a detail, but adjacency_buf and i are both defined as
> > unsigned int, so either defining them as plain, signed int or using
> > ~0 instead of -1 would be nicer.
>
> Nitpicking the nitpick: ~0U.

Do you guys (Matteo and Stefan) understand that you are essentially ban
an otherwise perfectly fine patch from inclusion? No wonder why almost
nobody else is touching the d3d code, and Sebastian (a real one, very
smart and qualified developer) next time will think twise before touching
it either. And considering dwindling and sparse commits last several months
I don't think you really want that.