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

[Matteo Bruni]

2011/5/29 Michael Mc Donnell <michael at mcdonnell.dk>:
> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger
> <stefandoesinger at gmx.at> wrote:
>> On Saturday 28 May 2011 19:55:55 you wrote:
>>
>>> It was implemented by setting the vertex declaration to null when the
>>> declaration is invalid. That seems to match what happens on Windows,
>>> because if a program has set an invalid declaration and then calls
>>> GetDeclaration or DrawSubset it will fail with an access violation.
>>> Should Wine also fail with an access violation or should it be
>>> slightly more graceful and return E_FAIL instead (which my new patch
>>> does)?
>> Yes, I think returning E_FAIL is a good idea, but write a WARN if
>> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not
>> be shown by default(unlike a FIXME), it is used when the app does something
>> fishy but you think you handled it correctly.
>>
>>> +    if (FAILED(hr))
>>> +        new_vertex_declaration = NULL;
>>> +
>>> +    /* Free old vertex declaration */
>>> +    if (This->vertex_declaration)
>>> +        IDirect3DVertexDeclaration9_Release(This->vertex_declaration);
>> In this situation you can just release the old decl before calling
>> CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl.
>> That simplified the code a bit. Don't forget to set This->vertex_declaration to
>> NULL in case of an error though
>
> Ok I've simplified the code. I've also inserted three WARNs. I've
> skipped a WARN when freeing the mesh because that works fine on
> Windows with an invalid declaration.
>
>>> +    /* Set the position type to color instead of float3 (invalid
>> declaration) */
>>> +    hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color);
>>> +    todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, "
>>> +        "got %#x expected D3D_OK\n", hr);
>> Is this really invalid? I don't know it on top of my head, but in general
>> types and usage are independent, especially with shaders. I don't see a check
>> in d3d9 or wined3d for this case.
>
> You're right it isn't invalid. It doesn't crash, it just doesn't draw
> anything. I've change that comment and re-ordered the tests.
>
>> Beyond that I think the patches look OK. I'd recommend to send them to wine-
>> devel one more time and then next week to wine-patches. Henri is on Vacation
>> this week, and he's usually more picky than I am. I also hope some of the
>> people who regularly contribute to d3dx9 will have a look and comment.
>
> Ok, I've attached the newest version. I'll let the patches sit here
> for a week, and start working on the other functions in the mean time.
>

The patches are OK with me, just some nitpicking:

+    testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));

We usually use something like HeapAlloc(GetProcessHeap(), 0,
sizeof(*testContext)); instead.

I think you can remove some obvious comments (like /* Create the test
mesh */). Also, check your whitespaces.