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

[Dylan Smith]

On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell <michael at mcdonnell.dk> wrote:
> Thank you for the good review.

Glad you appreciate them.  Here is another.

> +static HRESULT init_edge_face_map(struct edge_face_map *edge_face_map, CONST DWORD *index_buffer, CONST DWORD *point_reps, CONST DWORD num_faces)
> +{
...
> +    TRACE("(%p, %p, %p, %d)\n", edge_face_map, index_buffer, point_reps, num_faces);

A TRACE call for an internal initialization function like this seems
unnecessary. They are commonly provided for external functions since
it captures application behavior.

> +    if (!point_reps) /* Identity point reps */
> +    {
> +        id_point_reps = generate_identity_point_reps(num_faces, num_vertices);
> +        if (!id_point_reps)
> +        {
> +            hr = E_OUTOFMEMORY;
> +            goto cleanup;
...
> +    hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, &ib_ptr);
> +    if (FAILED(hr)) goto cleanup;
...
> +cleanup:
> +    HeapFree(GetProcessHeap(), 0, id_point_reps);
> +    if (indices_are_16_bit) HeapFree(GetProcessHeap(), 0, ib);
> +    free_edge_face_map(&edge_face_map);
> +    iface->lpVtbl->UnlockIndexBuffer(iface);
> +    return hr;
>  }

The index buffer is unconditionally unlocked in the cleanup code, but
goto cleanup can now be called before the index buffer is locked.

> +static DWORD *generate_identity_point_reps(DWORD num_faces, DWORD num_vertices)
> +{
> +        DWORD *id_point_reps;
> +        DWORD i;
> +
> +        TRACE("(%d, %d)\n", num_faces, num_vertices);
> +
> +        id_point_reps = HeapAlloc(GetProcessHeap(), 0, 3 * num_faces * sizeof(*id_point_reps));
> +        if (!id_point_reps)
> +            return NULL;
> +
> +        for (i = 0; i < num_vertices; i++)
> +        {
> +            id_point_reps[i] = i;
> +        }
> +        for (i = num_vertices; i < 3 * num_faces; i++)
> +        {
> +            id_point_reps[i] = -1;
> +        }
> +
> +        return id_point_reps;
> +}

Why are there exra point reps allocated with -1 value?  Won't only
num_vertices point_reps get used?