Re: [2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon.

[Matteo Bruni]

2014-11-03 6:35 GMT+01:00 Sebastian Lackner :
> Based on a patch by David Adam.
>
> dlls/d3dx9_36/tests/mesh.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 154 insertions(+)

More nitpicks...

+static BOOL compute_polygon(struct mesh *mesh, float length, UINT sides)

I'd prefer a plain unsigned int there.

The various comments I made for the implementation patch obviously
apply to the body of compute_polygon() too.

+    IDirect3D9* d3d;
+    IDirect3DDevice9* device;
+    D3DPRESENT_PARAMETERS d3dpp;
+    ID3DXMesh* polygon;
+    ID3DXBuffer* ppBuffer;

Watch out for the '*' placement. Also, no hungarian notation please.

+    hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT,
D3DDEVTYPE_HAL, wnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp,
&device);

Not sure we care but the other tests in mesh.c use
D3DCREATE_SOFTWARE_VERTEXPROCESSING.

+    ok(buffer_size == 33 * sizeof(DWORD), "expected size %d, received %d\n",
+       33 * sizeof(DWORD), buffer_size);

I think it's usually better not to print sizeof() results.

+        ok(buffer[i][1] == -1, "wrong adjacency[%d][1] = %d\n", i,
buffer[i][1]);

I'd prefer ~0 here. Also, unsigned int should be printed with "%u".

+    if (ppBuffer) ID3DXBuffer_Release(ppBuffer);

Please put the "if" body on a separate line.

I'd like to see an additional test to check whether the ID3DXMesh
pointer is set to NULL on D3DXCreatePolygon() failure. Probably you
could simply modify (at least) one of the failure tests at the
beginning for that.

[2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon.

[Sebastian Lackner]

Thanks for the feedback.

I am fine with changing what you suggested, but please note that most of these "issues" are also present in existing code, which I used as a template for implementing the new function. To point out a few examples:

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

Thats also the way its implemented for D3DXCreateSphere, D3DXCreateCylinder, D3DXCreateTeapot, D3DXCreateTextW, ...

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

"i++" has 95 matches in mesh.c, "++i" only 4 ...

Setting *mesh = NULL on error is only used at one place in the whole file. If it is necessary to match native behaviour, it is basically missing for all functions.

+static BOOL compute_polygon(struct mesh *mesh, float length, UINT sides)

See compute_sphere, compute_cylinder, compute_torus, ...

+    IDirect3D9* d3d;
+    IDirect3DDevice9* device;
+    D3DPRESENT_PARAMETERS d3dpp;
+    ID3DXMesh* polygon;
+    ID3DXBuffer* ppBuffer;

My fault, copy & paste from existing tests.

Regards,
Sebastian

On 03.11.2014 18:57, Matteo Bruni wrote:
> 2014-11-03 6:35 GMT+01:00 Sebastian Lackner :
> > Based on a patch by David Adam.
> >
> > dlls/d3dx9_36/tests/mesh.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 154 insertions(+)
>
> More nitpicks...
>
> +static BOOL compute_polygon(struct mesh *mesh, float length, UINT sides)
>
> I'd prefer a plain unsigned int there.
>
> The various comments I made for the implementation patch obviously
> apply to the body of compute_polygon() too.
> - IDirect3D9* d3d;
> - IDirect3DDevice9* device;
> - D3DPRESENT_PARAMETERS d3dpp;
> - ID3DXMesh* polygon;
> - ID3DXBuffer* ppBuffer;
>
> Watch out for the '*' placement. Also, no hungarian notation please.
> - hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT,
>
> D3DDEVTYPE_HAL, wnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp,
> &device);
>
> Not sure we care but the other tests in mesh.c use
> D3DCREATE_SOFTWARE_VERTEXPROCESSING.
> - ok(buffer_size == 33 * sizeof(DWORD), "expected size %d, received %d\n",
> -
>   33 * sizeof(DWORD), buffer_size);
>
> I think it's usually better not to print sizeof() results.
> -
>    ok(buffer[i][1] == -1, "wrong adjacency[%d][1] = %d\n", i,
>
> buffer[i][1]);
>
> I'd prefer ~0 here. Also, unsigned int should be printed with "%u".
> - if (ppBuffer) ID3DXBuffer_Release(ppBuffer);
>
> Please put the "if" body on a separate line.
>
> I'd like to see an additional test to check whether the ID3DXMesh
> pointer is set to NULL on D3DXCreatePolygon() failure. Probably you
> could simply modify (at least) one of the failure tests at the
> beginning for that.

[2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon.

[Matteo Bruni]

2014-11-03 19:18 GMT+01:00 Sebastian Lackner :
> Thanks for the feedback.
>
> I am fine with changing what you suggested, but please note that most of these "issues" are also present in existing code, which I used as a template for implementing the new function.

Yes, the existing code in d3dx9 (especially in mesh.c - even worse in
the tests) is hardly a good reference, unfortunately...
The general idea is to have the new code more or less follow the
"modern" wined3d style (cs.c should be a decent example of that).
> To point out a few examples:
> - TRACE("(%p, %f, %u, %p, %p)\n", device, length, sides, mesh, adjacency);
>
> Thats also the way its implemented for D3DXCreateSphere, D3DXCreateCylinder, D3DXCreateTeapot, D3DXCreateTextW, ...
> - for (i = 0; i < sides; i++)
>
> "i++" has 95 matches in mesh.c, "++i" only 4 ...
>
> Setting *mesh = NULL on error is only used at one place in the whole file. If it is necessary to match native behaviour, it is basically missing for all functions.

That might be the case. Thus the test I mentioned, either we need to
fix them all (and we can start by implementing it right for
D3DXCreatePolygon) or we don't and the implementation is fine as is.
Either way it seems a useful test to me.
> +static BOOL compute_polygon(struct mesh *mesh, float length, UINT sides)
>
> See compute_sphere, compute_cylinder, compute_torus, ...
> - IDirect3D9* d3d;
> - IDirect3DDevice9* device;
> - D3DPRESENT_PARAMETERS d3dpp;
> - ID3DXMesh* polygon;
> - ID3DXBuffer* ppBuffer;
>
> My fault, copy & paste from existing tests.
>
> Regards,
> Sebastian
>
> On 03.11.2014 18:57, Matteo Bruni wrote:
> > 2014-11-03 6:35 GMT+01:00 Sebastian Lackner :
> > > Based on a patch by David Adam.
> > >
> > > dlls/d3dx9_36/tests/mesh.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 154 insertions(+)
> >
> > More nitpicks...
> >
> > +static BOOL compute_polygon(struct mesh *mesh, float length, UINT sides)
> >
> > I'd prefer a plain unsigned int there.
> >
> > The various comments I made for the implementation patch obviously
> > apply to the body of compute_polygon() too.
> > - IDirect3D9* d3d;
> > - IDirect3DDevice9* device;
> > - D3DPRESENT_PARAMETERS d3dpp;
> > - ID3DXMesh* polygon;
> > - ID3DXBuffer* ppBuffer;
> >
> > Watch out for the '*' placement. Also, no hungarian notation please.
> > - hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT,
> >
> > D3DDEVTYPE_HAL, wnd, D3DCREATE_MIXED_VERTEXPROCESSING, &d3dpp,
> > &device);
> >
> > Not sure we care but the other tests in mesh.c use
> > D3DCREATE_SOFTWARE_VERTEXPROCESSING.
> > - ok(buffer_size == 33 * sizeof(DWORD), "expected size %d, received %d\n",
> > -
> >   33 * sizeof(DWORD), buffer_size);
> >
> > I think it's usually better not to print sizeof() results.
> > -
> >    ok(buffer[i][1] == -1, "wrong adjacency[%d][1] = %d\n", i,
> >
> > buffer[i][1]);
> >
> > I'd prefer ~0 here. Also, unsigned int should be printed with "%u".
> > - if (ppBuffer) ID3DXBuffer_Release(ppBuffer);
> >
> > Please put the "if" body on a separate line.
> >
> > I'd like to see an additional test to check whether the ID3DXMesh
> > pointer is set to NULL on D3DXCreatePolygon() failure. Probably you
> > could simply modify (at least) one of the failure tests at the
> > beginning for that.