vertices[vertI].mLocation = KMath::CVector3(DMF::GetNextFloat(fileStream), DMF::GetNextFloat(fileStream), DMF::GetNextFloat(fileStream));


I am working on a simple graphics engine as part of my game and once I got enough stuff done to be able to control my mesh I quickly learned that objects weren't orienting quite properly.

"Crap, left vs. right hand coordinates" I thought, as I am using D3D. I load up a mesh with 3 direction arrows as an indication, spit out tons of debug to find out where Im missing the conversion and- wait a minute. Why is the Z arrow pointing in X?

Maybe the color of the arrow is just wrong - nope. Maybe the transform matrix is mixed up - nope. Wtf? I load up a simple pyramid pointing in X+ and set up my camera looking at Z+ and the pyramid isn't pointing to the side - it is pointing away from the camera - wtfx2? Ok my view matrix is messed up so I strip my shader from any view transform and just leave the proj trans- wtf is STILL pointing away from the camera.

I debug the code and - oh what the hell - the pyramid is pointing in Z. Ok I must be messing up on creating the buffer then from raw data. No, wait, the raw data is wrong? I inspect the file over and over, everything is in check.

Shit, I must have mixed up order of params I passed inside the Vector3 constructor. I look at the constructor and - nope - its fine. No error there. Well, maybe I am passing them wrong, but nope, all in the right order. What the hell. I debug the code step by step - I am reading the values in the right order and I am pass...oooh... ooooooh.....

vertices[vertI].mLocation = KMath::CVector3(DMF::GetNextFloat(fileStream), DMF::GetNextFloat(fileStream), DMF::GetNextFloat(fileStream));


And then I remember, the order in which parameters are evaluated in C++ is not guaranteed. In this case, it was loading backwards, meaning I was getting the values in reverse order. And so my X became my Z.

I spent a good five hours just to find that simple, evil, line of code. FML

Yeah, actually...what you're doing is still bad practice. On x86 machines, arguments are passed left to right (Pascal conventions). In the C calling convention (_cdecl), it is right to left. The order in which they're evaluated is not guaranteed at all.

You're creating non-portable code.

I suspect 50% of those, including your example, would be summarized as "Today, I used side-effects where none were needed. FML"

The bug is that the GetNextFloat function exists in the first place [wink]

Quote: Original post by Programmer One
The order in which they're evaluated is not guaranteed at all.

Noooooo, really?

Quote: Original post by Koobazaur
And then I remember, the order in which parameters are evaluated in C++ is not guaranteed.

Quote: Original post by ToohrVyk
The bug is that the GetNextFloat function exists in the first place [wink]

What function would you recommend to use instead?

Quote: Original post by shurcool

Quote: Original post by ToohrVyk
The bug is that the GetNextFloat function exists in the first place [wink]

What function would you recommend to use instead?

You could perhaps define operator<<() (the extraction operator) with a KMath::CVector3 on the left-hand-side and istream on the right-hand-side.

e.g.

vertices[vertI].mLocation << fileStream;

*edit* I've messed up some minor points in my suggestion. For instance I said extraction operator when I actually used the insertion operator, which is slightly counter intuitive anyways. As usual, ToohrVyk's explanation following my post covers this topic better than I have :)

[Edited by - fpsgamer on January 4, 2011 12:30:25 PM]

Quote: Original post by shurcool
What function would you recommend to use instead?

As you might have guessed, the issue here is that GetNextFloat involves a subtle side-effect that is easy to miss - despite being called 'Get', the 'Next' part involves a change in the underlying stream object.

The solution is to make this side-effect obvious. In a functional language, you would simulate the side-effect by having GetNextFloat return a pair composed of the float and the remaining data in the stream. This way, you can't get the float without noticing that a stream was also returned. Of course, this requires a proper implementation of streams as immutable lazy linked lists, which is too complex for weak languages like C++ to handle elegantly.

The C++ solution follows this idea but gives up on immutability: stream operations return the modified stream, which means that if you need to access the read value, you need to add a variable access and unless you're deep into code obfuscation, you'll use a distinct statement (solving the problem). Still, streams are not immutable, which means that the original stream is also modified (and thus, returned for optimization purposes) which means that stream reading remains vulnerable to exceptions (an exception does not "roll back" the read data back into the stream).

In short, that line of code (at least in C++) should have been:

float x, y, z;fileStream >> x >> y >> z; // operator precedence constrains order of evaluationvertices[vertl].mLocation = KMath::CVector3(x,y,z);

Or possibly even:

fileStream >> vertices[vertl];

Quote: Original post by Koobazaur

Quote: Original post by Programmer One
The order in which they're evaluated is not guaranteed at all.

Noooooo, really?

Quote: Original post by Koobazaur
And then I remember, the order in which parameters are evaluated in C++ is not guaranteed.

tl;dr, also you never posted your solution - so for all I know, you could have also thrown in a divide by zero somewhere. [grin]

Quote: Original post by Programmer One

Quote: Original post by Koobazaur

Quote: Original post by Programmer One
The order in which they're evaluated is not guaranteed at all.

Noooooo, really?

Quote: Original post by Koobazaur
And then I remember, the order in which parameters are evaluated in C++ is not guaranteed.

tl;dr, also you never posted your solution - so for all I know, you could have also thrown in a divide by zero somewhere. [grin]

Unless I'm misreading the code, I would imagine that the fix would be as simple as loading each component into a temporary before creating the vector.

Quote: Original post by ToohrVyk

Quote: Original post by shurcool
What function would you recommend to use instead?

As you might have guessed, the issue here is that GetNextFloat involves a subtle side-effect that is easy to miss - despite being called 'Get', the 'Next' part involves a change in the underlying stream object.

The solution is to make this side-effect obvious. In a functional language, you would simulate the side-effect by having GetNextFloat return a pair composed of the float and the remaining data in the stream. This way, you can't get the float without noticing that a stream was also returned. Of course, this requires a proper implementation of streams as immutable lazy linked lists, which is too complex for weak languages like C++ to handle elegantly.

The C++ solution follows this idea but gives up on immutability: stream operations return the modified stream, which means that if you need to access the read value, you need to add a variable access and unless you're deep into code obfuscation, you'll use a distinct statement (solving the problem). Still, streams are not immutable, which means that the original stream is also modified (and thus, returned for optimization purposes) which means that stream reading remains vulnerable to exceptions (an exception does not "roll back" the read data back into the stream).

In short, that line of code (at least in C++) should have been:

float x, y, z;fileStream >> x >> y >> z; // operator precedence constrains order of evaluationvertices[vertl].mLocation = KMath::CVector3(x,y,z);

Or possibly even:

fileStream >> vertices[vertl];

Yes, I could create a complete streaming abstraction layer with classes deriving from the std::stream class, handling every possible scenario and in case of errors, of course, throw a new custom set of exceptions that will be handled at each level of my file loading mechanism.

Or... I could actually finish making my game. But I guess YMMV