d3dxcreatebox patch

[David Adam]

Hello,

is there any problem with my d3dxcreatebox patch?

Thanks in advance for any feedback.

A+

David

[Matteo Bruni]

2011/3/9 David Adam :
> Hello,
>
> is there any problem with my d3dxcreatebox patch?
>
> Thanks in advance for any feedback.
>
> A+
>
> David

Do you refer to http://source.winehq.org/patches/data/71983 ? I just
had a cursory look at it: I presume there isn't anything fundamentally
broken with your patch (you are forcing a particular vertex ordering,
however that shouldn't be a problem for the D3DXCreateBox
implementation) but, for sure, code style and formatting should be
fixed.
I know mesh.c is already quite bad in that regard, but that doesn't
mean it needs more loose code. For example, in your patch there isn't
a consistent usage of whitespace-after-comma, the same for spaces
around operators or between the "if" and its condition. To compare
expected and actual vertex data you should probably use the
compare_vec3() function. Also, are "vertice", "normale" and "indice"
Italian words? That doesn't look right. :) (also, the first vertex
component is the position, not really "vertex")

[David Adam]

Thanks for the feedback. What do you mean by "you are forcing a particular
vertex ordering3"?
I just test the vertex that D3DXCreateBox sent. I think it is not possible
to choose the ordering of the vertices for D3DXCreateBox. Did I miss
anything?

A+

David

2011/3/9 Matteo Bruni
> 2011/3/9 David Adam :
> > Hello,
> >
> > is there any problem with my d3dxcreatebox patch?
> >
> > Thanks in advance for any feedback.
> >
> > A+
> >
> > David
>
> Do you refer to http://source.winehq.org/patches/data/71983 ? I just
> had a cursory look at it: I presume there isn't anything fundamentally
> broken with your patch (you are forcing a particular vertex ordering,
> however that shouldn't be a problem for the D3DXCreateBox
> implementation) but, for sure, code style and formatting should be
> fixed.
> I know mesh.c is already quite bad in that regard, but that doesn't
> mean it needs more loose code. For example, in your patch there isn't
> a consistent usage of whitespace-after-comma, the same for spaces
> around operators or between the "if" and its condition. To compare
> expected and actual vertex data you should probably use the
> compare_vec3() function. Also, are "vertice", "normale" and "indice"
> Italian words? That doesn't look right. :) (also, the first vertex
> component is the position, not really "vertex")

[Matteo Bruni]

2011/3/10 David Adam :
> Thanks for the feedback. What do you mean by "you are forcing a particular
> vertex ordering3"?

I mean that you are testing the vertices of the mesh returned by
D3DXCreateBox in the order generated by native d3dx9, while they could
in general be in any order while still making up a box. That's not a
problem in my opinion because our implementation (when it will be
written :)) could generate the vertices with the same ordering without
any issue.
> I just test the vertex that D3DXCreateBox sent. I think it is not possible
> to choose the ordering of the vertices for D3DXCreateBox. Did I miss
> anything?
>
> A+
>
> David
>
> 2011/3/9 Matteo Bruni
> > 2011/3/9 David Adam :
> > > Hello,
> > >
> > > is there any problem with my d3dxcreatebox patch?
> > >
> > > Thanks in advance for any feedback.
> > >
> > > A+
> > >
> > > David
> >
> > Do you refer to http://source.winehq.org/patches/data/71983 ? I just
> > had a cursory look at it: I presume there isn't anything fundamentally
> > broken with your patch (you are forcing a particular vertex ordering,
> > however that shouldn't be a problem for the D3DXCreateBox
> > implementation) but, for sure, code style and formatting should be
> > fixed.
> > I know mesh.c is already quite bad in that regard, but that doesn't
> > mean it needs more loose code. For example, in your patch there isn't
> > a consistent usage of whitespace-after-comma, the same for spaces
> > around operators or between the "if" and its condition. To compare
> > expected and actual vertex data you should probably use the
> > compare_vec3() function. Also, are "vertice", "normale" and "indice"
> > Italian words? That doesn't look right. :) (also, the first vertex
> > component is the position, not really "vertex")

[Alexandre Julliard]

Matteo Bruni  writes:
> 2011/3/10 David Adam :
> > Thanks for the feedback. What do you mean by "you are forcing a particular
> > vertex ordering3"?
>
> I mean that you are testing the vertices of the mesh returned by
> D3DXCreateBox in the order generated by native d3dx9, while they could
> in general be in any order while still making up a box. That's not a
> problem in my opinion because our implementation (when it will be
> written :)) could generate the vertices with the same ordering without
> any issue.

There's no reason for the tests to require that. Also, hardcoding the
full list of vertices is ugly (and in the case of the teapot, clearly
absurd).

[David Adam]

2011/3/10 Alexandre Julliard
> Matteo Bruni  writes:
> > 2011/3/10 David Adam :
> > > Thanks for the feedback. What do you mean by "you are forcing a
>
> particular
> > > vertex ordering3"?
> >
> > I mean that you are testing the vertices of the mesh returned by
> > D3DXCreateBox in the order generated by native d3dx9, while they could
> > in general be in any order while still making up a box. That's not a
> > problem in my opinion because our implementation (when it will be
> > written :)) could generate the vertices with the same ordering without
> > any issue.
>
> There's no reason for the tests to require that. Also, hardcoding the
> full list of vertices is ugly (and in the case of the teapot, clearly
> absurd).
>
> --
> Alexandre Julliard
> [e-mail]

Must I pick up only a few vertices and test them? Or I do not test the
vertices returned by D3DXCreateBox

A+

David