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

[Michael Mc Donnell]

On Fri, Jul 1, 2011 at 7:22 AM, Dylan Smith <dylan.ah.smith at gmail.com> wrote:
> On Thu, Jun 30, 2011 at 3:50 PM, Michael Mc Donnell
> <michael at mcdonnell.dk> wrote:
>> I've implemented the look-up scheme that you described.
>
>> +    if (!point_reps) /* Indentity point reps */
>> +    {
>> +        memset(adjacency, -1, 3 * num_faces * sizeof(*adjacency));
>> +        return D3D_OK;
>> +    }
>
> I think you are missing the most common cases, where vertices are
> reused between triangles. For example:
>
> 0--1
> | /|
> |/ |
> 2--3
>
> If identity point reps are passed to your function it will find the
> adjacent edge, but if NULL is passed to the function for point reps,
> then it will find no adjacencies even though semantically they should
> be the same.

Ok, I've added that example to the test and fixed the implementation.

> Also, note the spelling error: Indentity -> Identity

Check :-)

>> +static void free_edge_face_map(struct edge_face_map *edge_face_map)
>> +{
>> +    if (!edge_face_map)
>> +        return;
>> +
>> +    HeapFree(GetProcessHeap(), 0, edge_face_map->lists);
>> +    HeapFree(GetProcessHeap(), 0, edge_face_map->entries);
>> +}
> ...
>>  static HRESULT WINAPI ID3DXMeshImpl_ConvertPointRepsToAdjacency(ID3DXMesh *iface, CONST DWORD *point_reps, DWORD *adjacency)
>>  {
>> +    struct edge_face_map *edge_face_map = NULL;
> ...
>> +    free_edge_face_map(edge_face_map);
>> +    iface->lpVtbl->UnlockIndexBuffer(iface);
>> +    return hr;
>>  }
>
> edge_face_map isn't freed. Also, it doesn't need to be allocated on the heap.

Ok, I've moved it to the stack.

>> +    hr = iface->lpVtbl->LockIndexBuffer(iface, 0, &ib_ptr);
>
> The LockIndexBuffer flags could be D3DLOCK_READONLY rather than 0
> (which seems to have read-write semantics).

Fixed.

Thank you for the good review.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-test-Implemented-ConvertPointRepsToAdjacency-t.patch
Type: text/x-patch
Size: 19130 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110702/924cb8fb/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-d3dx9-Implemented-ConvertPointRepsToAdjacency.patch
Type: text/x-patch
Size: 10568 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110702/924cb8fb/attachment-0003.bin>