d3dx9: CloneMesh test and improvements

[Michael Mc Donnell]

On Tue, Aug 9, 2011 at 3:17 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
> 2011/8/8 Michael Mc Donnell <michael at mcdonnell.dk>:
>> Hi
>>
>> I've been working on a test and improvements for CloneMesh as part of
>> GSoC 2011. I would be grateful if anyone could comment on my work.
>> There are five test cases in the attached patch:
>>
>>  0. Basic mesh cloning. Declaration has position and normal, and the
>> new declaration is the same as the original.
>>  1. Same as test 0, but with 16-bit indices.
>>  2. Shows that the vertex buffer can be widened so that it has room
>> for a new vertex component. The declaration has position and normal,
>> and the new declaration has texture coordinates after position and
>> normal.
>>  3. Same as test 2, but the new declaration has the texture
>> coordinates in between position and normal.
>>  4. Shows that a vertex component can be converted into another type
>> if the new declaration specifies another type. The declaration has
>> position and normal. The normal is a FLOAT2 and in the new declaration
>> it is instead FLOAT16_2.
>>
>> Test 0 and 1 tests what is already known to work in the current
>> implementation. Test 2, 3 and 4 do not work in the current
>> implementation because the original and new declarations are not the
>> same.
>>
>> I've also improved the current CloneMesh implementation so that it
>> also passes test 2, 3, and 4. The patch is not complete because it
>> does not support converting all the possible types. I expect to
>> implement conversion for the remaining types during this week.
>>
>> Again any comments or ideas are welcome.
>>
>> Thanks,
>> Michael Mc Donnell
>>
>
> Hello Michael,
> I have some comments:
>
> +const UINT d3dx_decltype_size[D3DDECLTYPE_UNUSED] =
> +{
>
> I don't really like sizing the vector like that. Just remove the
> D3DDECLTYPE_UNUSED between the square brackets, I'd say. Otherwise,
> you could use the D3DMAXDECLTYPE define.
>
> +   /* D3DDECLTYPE_FLOAT1    */ 1 * 4,
> ...
>
> You may want to replace the 4 with a sizeof(float) for better clarity.
> Same for the others.

Yeah that does look better, I'll do that. It was code that I moved up
higher in the source file. I hope it's ok to change it too.

> +        if (orig_declaration.Method == declaration[i].Method
> +            && orig_declaration.Usage == declaration[i].Usage
> +            && orig_declaration.UsageIndex == declaration[i].UsageIndex)
>
> Does the Method field really need to be compared? Maybe add a test if
> that is the case.

Good catch. It doesn't do that comparison on Windows. I've added test
18 to show that and I've removed the comparison.

> You may also e.g. test if in a POSITION, NORMAL, TEXCOORD(0) ->
> POSITION, NORMAL, TEXCOORD(1) CloneMesh (or even the other way around)
> the texture coordinates are actually lost or not.

I've added test 19 and 20 to show that the data is lost.

> +    for (i = 0; orig_declaration[i].Stream != 0xff; i++) {
> +        if (memcmp(&orig_declaration[i], &declaration[i],
> sizeof(*declaration))) {
> +            same_declaration = FALSE;
> +            break;
> +        }
> +    }
> +
> +    if (vertex_size != orig_vertex_size)
> +        same_declaration = FALSE;
>
> Not really a big thing, but you could do the vertex size check first
> and then compare the fields only if same_declaration is still TRUE.

I just noticed that there is also a bug there. It should have been the
declaration length and not the vertex size. I've extracted it out into
the function declaration_equals for the sake of clarity.

> The tests look good to me, except some nits like the ok() in
> check_vertex_components in the D3DDECLTYPE_FLOAT1 case, splitting long
> lines and such.

I've fixed the ok(). What do you mean by splitting long lines. Should
I split them at 80 characters?

I've also implemented the rest of the FLOAT2 conversions except for
UDEC3 and DEC3N which seemed to cause a memory corruption on Windows.
I'm going to work next on the remaining conversions.

Thank you very much for reviewing the patch Matteo.

Cheers,
Michael Mc Donnell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-Implemented-CloneMesh-conversion-and-test.patch
Type: text/x-patch
Size: 68837 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110810/e89978f8/attachment-0001.bin>