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

[Michael Mc Donnell]

On Tue, May 31, 2011 at 2:30 PM, Michael Mc Donnell
<michael at mcdonnell.dk> wrote:
> On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell
> <michael at mcdonnell.dk> wrote:
>> On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
>>> 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.
>>
>> Ok, I've changed that.
>>
>>>>> 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...
>>
>> Ok I've changed it to the "align to be below the '(' + 1 space" convention.
>>
>>> There was also something wrong with spaces in a comment that caught my
>>> eye, I think.
>>
>> The only thing that stood out to me was that I had extra newlines in
>> my multi-line comments. I've removed those.
>>
>>> I told you I was nitpicking... :)
>>
>> That's ok, I prefer consistency too :-)
>
> Oops the patches I sent earlier had a tab character and some
> superfluous whitespace.

Henri, do you have any comments about my UpdateSemantics patches
before I send them off to wine-patches?

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-test-Added-UpdateSemanticsTest.patch
Type: text/x-patch
Size: 11016 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110608/3ea932a9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-d3dx9-Implemented-UpdateSemantics-mesh-method.patch
Type: text/x-patch
Size: 7787 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110608/3ea932a9/attachment-0003.bin>