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

[Matteo Bruni]

2011/5/30 Michael Mc Donnell <michael at mcdonnell.dk>:
> 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?
>

Yes, that's a reason. The other one is simply consistency.

>> 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?

+    todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, "

+        "got %#x expected D3D_OK\n", hr);

Here you put 4 spaces on the line continuation, which is neither the
"align to be below the '(' + 1 space" used in most of the file, nor
the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a
nice and consistent coding style (I'm sure you noticed that), so
that's mostly trying to not mix even more style variants...
There was also something wrong with spaces in a comment that caught my
eye, I think.

I told you I was nitpicking... :)

>
> Thanks!
>