d3dx9: Implement D3DXCreateTorus() + tests

[Matteo Bruni]

Hi Gediminas,


2014-06-30 19:29 GMT+02:00 Gediminas Jakutis <gediminas at varciai.lt>:
> ---
>  dlls/d3dx9_36/d3dx9_36.spec |   2 +-
>  dlls/d3dx9_36/mesh.c        | 129 +++++++++++++++++++++++++++++++
>  dlls/d3dx9_36/tests/mesh.c  | 183 ++++++++++++++++++++++++++++++++++++++++++++
>  include/d3dx9shape.h        |   2 +
>  4 files changed, 315 insertions(+), 1 deletion(-)
>

+HRESULT WINAPI D3DXCreateTorus(struct IDirect3DDevice9 *device,
+    float innerradius, float outerradius, UINT sides, UINT rings,
struct ID3DXMesh **mesh, ID3DXBuffer **adjacency)

The "usual" style is 8 spaces for line continuations (that also
applies to some other places in the patch).

+    float sin;
+    float cos;

Not really an issue but I'd prefer if these wouldn't shadow the
functions from math.h.

+    if (!device || innerradius < 0.0f || outerradius < 0.0f || sides
< 3 || rings < 3 || !mesh)
+    {
+        return D3DERR_INVALIDCALL;
+    }

Not strictly required but you can either avoid the curly braces (here
and below) or add a WARN for the error return condition. Probably a
WARN here is nicer.

+    rotation_step = D3DX_PI / sides * -2.0f;
+    rotation_curr = 0;

Please use 0.0f there instead.

+    for (i = 0; i < sides; ++i)
+    {
+        sin = sinf(rotation_curr);
+        cos = cosf(rotation_curr);
+        vertices[i].position.x = innerradius * cos + outerradius;
+        vertices[i].position.y = 0.0f;
+        vertices[i].position.z = innerradius * -sin;
+        vertices[i].normal.x = cos;
+        vertices[i].normal.y = 0.0f;
+        vertices[i].normal.z = -sin;
+
+        rotation_curr += rotation_step;
+    }

I might be mistaken but I think you can remove the "-" from the
previous "-2.0f" and from both "-sin" here.

+    for (i = 0; i < numfaces - sides * 2; ++i)
+    {
+        faces[i][0] = i % 2 ? i / 2 + sides : i / 2;
+        faces[i][1] = (i / 2 + 1) % sides ? i / 2 + 1 : i / 2 + 1 - sides;
+        if ((i + 1 >= sides * 4) && !((i + 1) % sides) && !((i + 1) /
sides % 2))
+        {
+            faces[i][2] = (i + 1) / 2;
+        }
+        else if (i)
+        {
+            faces[i][2] = (i + 1) % (sides * 2) ? (i + 1) / 2 + sides
: (i / sides) * sides;
+        }
+        else
+        {
+            faces[i][2] = sides;
+        }
+    }

Can't the last "else" just be dropped (and the condition from the
previous "else" removed)? I.e. the first "else" for the i == 0 case is
equivalent to the second one, AFAICS.

Actually the first two branches also look to be the same so this
should be enough for the third vertex of each face:

faces[i][2] = (i + 1) % (sides * 2) ? (i + 1) / 2 + sides : (i + 1) / 2;

+    for (j = 0; i < numfaces; ++i, ++j)
+    {
+        faces[i][0] = i % 2 ? j / 2 : sides * (rings - 1) + j / 2;

This should be equivalent:

faces[i][0] = i % 2 ? j / 2 : i / 2;

I don't mind all that much for this one though.

+        faces[i][1] = (i / 2 + 1) % sides ? i / 2 + 1 : i / 2 + 1 - sides;
+        faces[i][2] = i == numfaces - 1 ? 0 : (j + 1) / 2;
+    }

I guess this separate loop for the last "strip" of the mesh is there
to avoid a bunch of "% numvert" in the previous loop. That's okay for
me.

+    hr = D3DXCreateTorus(NULL, 0.0f, 0.0f, 3, 3, &torus, NULL);
+    ok(hr == D3DERR_INVALIDCALL, "Got result %x, expected %x
(D3DERR_INVALIDCALL)\n",hr,D3DERR_INVALIDCALL);

Missing spaces after the commas.

The notes regarding vertices and faces generation also apply to
compute_torus in the test obviously.


For some reason your patch has DOS-style CRLF line terminators and
that made "git apply" fail for me. Testbot didn't complain though so
probably that's not an issue. Generally it's preferred to generate
patches with "git format-patch" or even better directly "git
send-email".