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

Tue May 31 07:30:31 CDT 2011

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.
-------------- 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/20110531/7ef72d82/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/20110531/7ef72d82/attachment-0003.bin>