Re: d3dx9: CloneMesh test and improvements

[Michael Mc Donnell]

On Tue, Aug 9, 2011 at 3:17 PM, Matteo Bruni  wrote:
> 2011/8/8 Michael Mc Donnell :
> > 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