Hi Gediminas,
2014-06-30 19:29 GMT+02:00 Gediminas Jakutis gediminas@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".