Re: Patchwatcher: patchwatcher bug: Try number 2 drawprim.c drawStridedSlow

[Henri Verbeet]

2008/9/11 Chris Ahrendt :
> Ok what now.... I did a git to get the latest tree... I generated the
> patch off the latest tree.. and submitted it again... what I am failing.
> so I can fix it =).. the code compiles just fine and runs on my machine
> so not sure...
>
> chris

The patch is not against current git, it's against an earlier version
of your patch.

More importantly though, the patch is completely unacceptable. You
really need to try to get a better understanding of how D3D vertex
declarations work, and what the different D3DDECLTYPE values mean.

Patchwatcher: patchwatcher bug: Try number 2 drawprim.c drawStridedSlow

[Chris Ahrendt]

Henri Verbeet wrote:
> 2008/9/11 Chris Ahrendt :
> > Ok what now.... I did a git to get the latest tree... I generated the
> > patch off the latest tree.. and submitted it again... what I am failing.
> > so I can fix it =).. the code compiles just fine and runs on my machine
> > so not sure...
> >
> > chris
>
> The patch is not against current git, it's against an earlier version
> of your patch.
>
> More importantly though, the patch is completely unacceptable. You
> really need to try to get a better understanding of how D3D vertex
> declarations work, and what the different D3DDECLTYPE values mean.

Henri,

Actually it was based on the current Git tree not a back level.... but 
will try and do a merge to see if there is an issue...

I disagree on your comment... I am following the MSDN definition of what 
the function does.  I do know what they mean I have been discussing 
these with stefan.

Chris

Patchwatcher: patchwatcher bug: Try number 2 drawprim.c drawStridedSlow

[Henri Verbeet]

2008/9/11 Chris Ahrendt :
> Henri Verbeet wrote:
> > 2008/9/11 Chris Ahrendt :
> > > Ok what now.... I did a git to get the latest tree... I generated the
> > > patch off the latest tree.. and submitted it again... what I am failing.
> > > so I can fix it =).. the code compiles just fine and runs on my machine
> > > so not sure...
> > >
> > > chris
> >
> > The patch is not against current git, it's against an earlier version
> > of your patch.
> >
> > More importantly though, the patch is completely unacceptable. You
> > really need to try to get a better understanding of how D3D vertex
> > declarations work, and what the different D3DDECLTYPE values mean.
>
> Henri,
>
> Actually it was based on the current Git tree not a back level.... but will
> try and do a merge to see if there is an issue...

Current git:
 449                     /* The coords to supply depend completely on
the fvf / vertex shader */
 450                     switch (coordsToUse) {
 451                     case 4: q = ptrToCoords[3]; /* drop through */
 452                     case 3: r = ptrToCoords[2]; /* drop through */
 453                     case 2: t = ptrToCoords[1]; /* drop through */
 454                     case 1: s = ptrToCoords[0];
 455                     }

Your patch:
>                 switch (coordsToUse) {
> -
>                case 16:
> -
>                case 15:
> -
>                case 14:
> -
>                case 17:
>               case 13:
> -
>                case 12:

Where do you see those lines you're removing in current git?
> I disagree on your comment... I am following the MSDN definition of what the
> function does.  I do know what they mean I have been discussing these with
> stefan.

Well, proof me wrong then:
Did you test on a native Windows system if the code should behave the
way you think it should? Why are you creating a switch statement based
on numeric constants instead of the appropriate enum values? Do you
understand what, for example, D3DDECLTYPE_UBYTE4N means? What is the
size of a D3DDECLTYPE_UBYTE4N element? What is the range of a
component of a D3DDECLTYPE_UBYTE4N element? What is the size of a
float? What is it's range? What are the implications of all that for
using ptrToCoords, which is a float pointer, for D3DDECLTYPE_UBYTE4N
in combination with glMultiTexCoord4fARB(), which expects four
normalized floating point values? How is all this handled for the
other attributes, like diffuse, specular, normal and position? How
does drawStridedSlowVs() handle this? What about
drawStridedInstanced()?

Patchwatcher: patchwatcher bug: Try number 2 drawprim.c drawStridedSlow

[Chris Ahrendt]

Henri Verbeet wrote:
> 2008/9/11 Chris Ahrendt :
> > Henri Verbeet wrote:
> > > 2008/9/11 Chris Ahrendt :
> > > > Ok what now.... I did a git to get the latest tree... I generated the
> > > > patch off the latest tree.. and submitted it again... what I am failing.
> > > > so I can fix it =).. the code compiles just fine and runs on my machine
> > > > so not sure...
> > > >
> > > > chris
> > >
> > > The patch is not against current git, it's against an earlier version
> > > of your patch.
> > >
> > > More importantly though, the patch is completely unacceptable. You
> > > really need to try to get a better understanding of how D3D vertex
> > > declarations work, and what the different D3DDECLTYPE values mean.
> >
> > Henri,
> >
> > Actually it was based on the current Git tree not a back level.... but will
> > try and do a merge to see if there is an issue...
>
> Current git:
>  449                     /* The coords to supply depend completely on
> the fvf / vertex shader */
>  450                     switch (coordsToUse) {
>  451                     case 4: q = ptrToCoords[3]; /* drop through */
>  452                     case 3: r = ptrToCoords[2]; /* drop through */
>  453                     case 2: t = ptrToCoords[1]; /* drop through */
>  454                     case 1: s = ptrToCoords[0];
>  455                     }
>
> Your patch:
> >                 switch (coordsToUse) {
> > -
> >                case 16:
> > -
> >                case 15:
> > -
> >                case 14:
> > -
> >                case 17:
> >               case 13:
> > -
> >                case 12:
>
> Where do you see those lines you're removing in current git?
> > I disagree on your comment... I am following the MSDN definition of what the
> > function does.  I do know what they mean I have been discussing these with
> > stefan.
>
> Well, proof me wrong then:
> Did you test on a native Windows system if the code should behave the
> way you think it should? Why are you creating a switch statement based
> on numeric constants instead of the appropriate enum values? Do you
> understand what, for example, D3DDECLTYPE_UBYTE4N means? What is the
> size of a D3DDECLTYPE_UBYTE4N element? What is the range of a
> component of a D3DDECLTYPE_UBYTE4N element? What is the size of a
> float? What is it's range? What are the implications of all that for
> using ptrToCoords, which is a float pointer, for D3DDECLTYPE_UBYTE4N
> in combination with glMultiTexCoord4fARB(), which expects four
> normalized floating point values? How is all this handled for the
> other attributes, like diffuse, specular, normal and position? How
> does drawStridedSlowVs() handle this? What about
> drawStridedInstanced()?

Because the original code was set up with the numbers originally..
If you look at any of the pre say 1.1.3 code you will see only the
switch code with 4 - 1 in it.. and nothing more...
to get rid of an error I was getting in an application which was sending 
  a 7 for coordstouse which is a valid value (its a 6 the code adds 1 to 
it for some odd reason so I followed the present convention as last time 
I cleaned something up and did it with #defines or moved code I was told 
to leave it as it is if I see it that way).
I know the various types... I put the comment in there and was telling
stefan what they were so I am aware of it... however it does not seem to 
make a difference if its float or short). The values are not normalized 
in the code but by the application and passed into the call.

That I do not know how they are handled in other places...   the code 
was really simple when I started working with it to clear what I 
submitted up...

Chris

Patchwatcher: patchwatcher bug: Try number 2 drawprim.c drawStridedSlow

[Stefan Dösinger]

> Because the original code was set up with the numbers originally..

And this is wrong. So the original code is wrong here. The assumption the
code makes just happens to work for FLOAT1 to FLOAT4 D3DDECL types, and not
for higher ones