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

[Michael Mc Donnell]

Thanks for the comments Matteo! I have a few questions below.

On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
> 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:

That sounds great :-)

> +    testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext));
>
> We usually use something like HeapAlloc(GetProcessHeap(), 0,
> sizeof(*testContext)); instead.

Ok, I guess that's better in case the type of the variable changes at
some point?

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

Is it too much or too little whitespace? Could you give me an example?

Thanks!