Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

[Michael Mc Donnell]

On Tue, Jun 21, 2011 at 5:36 PM, Dylan Smith <dylan.ah.smith at gmail.com> wrote:
> On Tue, Jun 21, 2011 at 6:32 AM, Michael Mc Donnell
> <michael at mcdonnell.dk> wrote:
>> Here is my test and implementation of ConvertAdjacencyToPointReps.
>
>
>> +    hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, (void**)&indices);
>> +    if (FAILED(hr)) goto cleanup;
>> +    memcpy(new_indices, indices, This->numvertices * sizeof(*indices));
>
> I think you want the number of faces * 3 rather than the number of
> vertices.  And the size of an index is not constant, it is 32-bit if
> This->options has the D3DXMESH_32BIT bit set and 16-bit if the bit
> isn't set.

Yes you're right, I'll change that. I had completely forgotten about
16-bit indices, so that required a lot more changes and a test.

>> +static void test_convert_adjacency_to_point_reps(void)
>> +{
> ...
>> +    /* All mesh data */
>> +    struct vertex_pnc *vertices[] = {vertices0, vertices1, vertices2, vertices3, vertices4, vertices5, vertices6};
>> +    ID3DXMesh *meshes[ARRAY_SIZE(vertices)];
>> +    DWORD *indices[] = {indices0, indices1, indices2, indices3, indices4, indices5, indices6};
>> +    DWORD num_vertices[] = {num_vertices0, num_vertices1, num_vertices2, num_vertices3, num_vertices4, num_vertices5, num_vertices6};
>> +    DWORD num_faces[] = {num_faces0, num_faces1, num_faces2, num_faces3, num_faces4, num_faces5, num_faces6};
>> +    DWORD *adjacencies[] = {adjacency0, adjacency1, adjacency2, adjacency3, adjacency4, adjacency5, adjacency6};
>> +    DWORD *point_reps[] = {point_rep0, point_rep1, point_rep2, point_rep3, point_rep4, point_rep5, point_rep6};
>> +    DWORD *exp_point_reps[] = {exp_point_rep0, exp_point_rep1, exp_point_rep2, exp_point_rep3, exp_point_rep4, exp_point_rep5, exp_point_rep6};
>
> I think it would make sense to put all these arrays of the same length
> into a structure (e.g. struct test_case) array.  This would make it
> clear what needs to be updated to add another test case, and less
> error prone than making sure each relevant individual array is
> updated.

Ok I've tried to make a struct for them.

> Also, several of these and similar array seem like they can be made constant.

Check, I've const'ified everything I could find.

>> +static void test_convert_adjacency_to_point_reps(void)
>> +{
> ...
>> +    for (i = 0; i < ARRAY_SIZE(meshes); i++)
>> +    {
> ...
>> +        hr = meshes[i]->lpVtbl->ConvertAdjacencyToPointReps(meshes[i], adjacencies[i], point_reps[i]);
>> +        todo_wine ok(hr == D3D_OK, "ConvertAdjacencyToPointReps failed. "
>> +                     "Got %x expected D3D_OK\n", hr);
>
> The value i should probably be printed in this error message.
> Otherwise, if you see a failure on test.winehq.org that you can't
> reproduce, it will be hard to figure out what might have caused the
> failure.

Yes, good idea.

> That's it for now.

Thanks your comments. I've attached an updated version.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-tests-Implemented-ConvertAdjacencyToPointReps-.patch
Type: text/x-patch
Size: 15688 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110622/70a745a8/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-d3dx9-Implemented-ConvertAdjacencyToPointReps-mesh-m.patch
Type: text/x-patch
Size: 9490 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110622/70a745a8/attachment-0003.bin>