Game engine WTF.

Ulvhamne

Heya. I'm a game dev, and I went bug hunting today in the engine Im currently forced to use. The engine were made by a bunch of other guys, and I have had some pretty big problems with it and once in a while I state "WTF" out loud when I see something. I thought I'd post todays piece of code here...

float Terrain::GetHeightOnPositionXZ( float x, float z ) const
{

float Px, Pz, Py;
float Ay, By, Cy;
float dAB, dAC, dCB;

x /= m_scale;
z /= m_scale;
Px = x -(int)x;
Pz = z -(int)z;

// Top triangle

if( Pz < ( 1 - Px ) )
{
Ay = m_heightValues[(int)x + (int)z * m_size];
By = m_heightValues[(int)x + (int)(z + 1) * m_size];
Cy = m_heightValues[(int)(x + 1) + (int)z * m_size];

dAB = By - Ay;
dAC = Cy - Ay;
Py = Ay + (dAB * Pz);
Py += dAC * Px;
}

// Lower triangle

//Snipped the same stuff as in the if only for the lower triangle. Pretty much exactly the same code.

return Py;

}

This code actually can be called with negative X and Y values.
This specific code is from a terrain class, and it returns the height on the position you feed it.

Lets see if you can find the WTF.

Ulvhamne

And yes, Im aware of the sad state of my english, thats not the WTF. ;)

trollable

@Ulvhamne said:

Py = Ay + (dAB * Pz)+dAC * Px;

Looks suspicious for me. Bad interpolation?

Ulvhamne

Well, thats a minor one. ;)

Narcissus

x /= m_scale;
z /= m_scale;
Px = x -(int)x;
Pz = z -(int)z;

Px and Pz are always 0?

osp70

I think it lies with the Pz and Pz calculations

I'm not sure how the rounding will be handled for the int casting. So the results are not consistent.

Narcissus

Yeah, I'm an idiot. Px and Pz aren't always zero...

ferrengi

Looks like Py does not get set to anything unless the if statement if ( Pz < ( 1 - Px ) ) is true
I guess it would return 0 in that case?

Obviously, the lack of any helpful comments or variable names makes this difficult to understand and maintain and is WTFworthy.
Also, no explanation of m_scale and m_size
I guess they are member variables of the Terrain class or global variables.

gonzo

@ferrengi said:

Looks like Py does not get set to anything unless the if statement if ( Pz < ( 1 - Px ) ) is true
I guess it would return 0 in that case?

I might be wrong but I think Py will be handled by the else block that was snipped out by the original poster. As for the WTF, yes I also think it is the slightly weird calc of the reminder of Px and Pz. And the function should at least check for negative values.

Ulvhamne

Not quite there yet. ;)

And yes, the m_ are members.
And yes, ´there is supposed to be an else block there too.. Accidently snipped away the "else { " too. Heh.

Nallebjorn

Well, i'll take a shot on it also, couldnt help my self trying to think about it :)
could this be the problem perhaps ?

pretend z = -5.9 and m_scale = 1
pz = z - (int)z = -5.9 - (-5) => pz = -0.9

if(m_size > 1) and
if x < -(z * m_size) or x < 0 indexation of array m_heightValues will be negative

ferrengi

@Ulvhamne said:

Heya. I'm a game dev, and I went bug hunting today in the engine Im currently forced to use. The engine were made by a bunch of other guys, and I have had some pretty big problems with it and once in a while I state "WTF" out loud when I see something. I thought I'd post todays piece of code here...

 
float Terrain::GetHeightOnPositionXZ( float x, float z ) const
{
if( Pz < ( 1 - Px ) )
{
Ay = m_heightValues[(int)x + (int)z * m_size];
By = m_heightValues[(int)x + (int)(z + 1) * m_size];
Cy = m_heightValues[(int)(x + 1) + (int)z * m_size];

}

// Lower triangle

This code actually can be called with negative X and Y values.
This specific code is from a terrain class, and it returns the height on the position you feed it.

Lets see if you can find the WTF.

If x and z can be negative, that means that it is possible that the index being fed into the m_heightValues[] array can be negative. That means that there is an out of bounds error that is not being caught. The values in Ay, By, and Cy are not being checked so the code has no way of knowing if a valid value was read from the array.

Ulvhamne

yeah, and the terrain is supposed to work with negative numbers. At least according to the spec.

Lews

Wow, count the WTFs:

TheUseOfCamelCasing

sHungarian sNotation

Defining all the variables up at the top

Poor checking to see which triangle the point is in

Having "pretty much exactly the same code" twice in the same function

ferrengi

@Ulvhamne said:

yeah, and the terrain is supposed to work with negative numbers. At least according to the spec.

So, we got it right?

Ulvhamne

yeah. ;)

the density of wtf's in that class are the same all over..

I actually removed 1/3rd of the code because it didnt do something once.

I might post something more later on. ;) Or a horror story.

LoztInSpace

@Lews said:

Wow, count the WTFs:
TheUseOfCamelCasing
sHungarian sNotation
Defining all the variables up at the top
Poor checking to see which triangle the point is in
Having "pretty much exactly the same code" twice in the same function

use of float

Honestly, and I'm ashamed to admit it, this looks almost identical to my own code.

The only difference is that for me, the terrain was stored as a grid of triangles, not just an array of height values.

trollable

1) TheUseOfCamelCasing
2) sHungarian sNotation
3) Defining all the variables up at the top
4) Poor checking to see which triangle the point is in
5) Having "pretty much exactly the same code" twice in the same function
6) use of float

Let's see.
1) used in many langages and many APIs. Maybe it is adequate in that case. not a WTF
2) widely used. recommanded by many (but not me). not a WTF
3) sometimes it makes the code much more readable. not a WTF
4) bug? not a WTF
5) can it be factorized? not sure. not a WTF
6) this is the best choice. fast. thread-safe. precise. not a WTF.

Zlodo

Another one:

The width of the terrain is called "size".

@trollable said:

5) can it be factorized? not sure. not a WTF

Most likely, it can. If you're doing the same thing several times with only a few differences, you make a goddamn function. It can be inline, so performance is not an issue.

@Zlodo said:

Another one:

The width of the terrain is called "size".

@trollable said:
5) can it be factorized? not sure. not a WTF

Most likely, it can. If you're doing the same thing several times with only a few differences, you make a goddamn function. It can be inline, so performance is not an issue.

You take that as a rule? Personally, I think it confuses and adds obfuscation if you factor code that is only repeated once or twice, and stick it in a function. In this case, it is either one triangle or another - you don't need to make a function to do that.

mrprogguy

@trollable said:

1) TheUseOfCamelCasing
2) sHungarian sNotation
3) Defining all the variables up at the top
4) Poor checking to see which triangle the point is in
5) Having "pretty much exactly the same code" twice in the same function
6) use of float

Let's see.
1) used in many langages and many APIs. Maybe it is adequate in that case. not a WTF
2) widely used. recommanded by many (but not me). not a WTF
3) sometimes it makes the code much more readable. not a WTF
4) bug? not a WTF
5) can it be factorized? not sure. not a WTF
6) this is the best choice. fast. thread-safe. precise. not a WTF.

Ditto from me on all counts...except maybe the float/double thing. Pretty sure doubles are handled without multiple instructions in the FPU, which would make them thread-safe. On the other hand, I really should go look that up. [^o)]

jwenting

@mrprogguy said:

@trollable said:
1) TheUseOfCamelCasing
2) sHungarian sNotation
3) Defining all the variables up at the top
4) Poor checking to see which triangle the point is in
5) Having "pretty much exactly the same code" twice in the same function
6) use of float

Let's see.
1) used in many langages and many APIs. Maybe it is adequate in that case. not a WTF
2) widely used. recommanded by many (but not me). not a WTF
3) sometimes it makes the code much more readable. not a WTF
4) bug? not a WTF
5) can it be factorized? not sure. not a WTF
6) this is the best choice. fast. thread-safe. precise. not a WTF.

Ditto from me on all counts...except maybe the float/double thing. Pretty sure doubles are handled without multiple instructions in the FPU, which would make them thread-safe. On the other hand, I really should go look that up. [^o)]

depends on the CPU...

Hungarian Notation is a WTF, though it's a WTF not so much of the persons using it but of the person inventing it [;)]

e_thermal

@Ulvhamne said:

Heya. I'm a game dev, and I went bug hunting today in the engine Im currently forced to use. The engine were made by a bunch of other guys, and I have had some pretty big problems with it and once in a while I state "WTF" out loud when I see something. I thought I'd post todays piece of code here...


float Terrain::GetHeightOnPositionXZ( float x, float z ) const
{

float Px, Pz, Py;
float Ay, By, Cy;
float dAB, dAC, dCB;

x /= m_scale;
z /= m_scale;
Px = x -(int)x;
Pz = z -(int)z;

// Top triangle

if( Pz < ( 1 - Px ) )
{
Ay = m_heightValues[(int)x + (int)z * m_size];
By = m_heightValues[(int)x + (int)(z + 1) * m_size];
Cy = m_heightValues[(int)(x + 1) + (int)z * m_size];

dAB = By - Ay;
dAC = Cy - Ay;
Py = Ay + (dAB * Pz);
Py += dAC * Px;
}

// Lower triangle

//Snipped the same stuff as in the if only for the lower triangle. Pretty much exactly the same code.

return Py;

}



This code actually can be called with negative X and Y values.
This specific code is from a terrain class, and it returns the height on the position you feed it.

Lets see if you can find the WTF.

Looks to me like sample code straight out of an Andre Lamothe book written in 1995

Hux

@Lews said:

Wow, count the WTFs:
sHungarian sNotation

Is this in reference to the dAB variable naming? In all honesty, I thought the 'd' in this case was referring to distance, perhaps, rather than double.

Jorg

@Hux said:

Is this in reference to the dAB variable naming? In all honesty, I thought the 'd' in this case was referring to distance, perhaps, rather than double.

There's another strike against hungarian notation. Can easily be misleading, rather than informational.

Ulvhamne

@e.thermal said:

@Ulvhamne said:
Heya. I'm a game dev, and I went bug hunting today in the engine Im currently forced to use. The engine were made by a bunch of other guys, and I have had some pretty big problems with it and once in a while I state "WTF" out loud when I see something. I thought I'd post todays piece of code here...


float Terrain::GetHeightOnPositionXZ( float x, float z ) const
{

float Px, Pz, Py;
float Ay, By, Cy;
float dAB, dAC, dCB;

x /= m_scale;
z /= m_scale;
Px = x -(int)x;
Pz = z -(int)z;

// Top triangle

if( Pz < ( 1 - Px ) )
{
Ay = m_heightValues[(int)x + (int)z * m_size];
By = m_heightValues[(int)x + (int)(z + 1) * m_size];
Cy = m_heightValues[(int)(x + 1) + (int)z * m_size];

dAB = By - Ay;
dAC = Cy - Ay;
Py = Ay + (dAB * Pz);
Py += dAC * Px;
}

// Lower triangle

//Snipped the same stuff as in the if only for the lower triangle. Pretty much exactly the same code.

return Py;

}



This code actually can be called with negative X and Y values.
This specific code is from a terrain class, and it returns the height on the position you feed it.

Lets see if you can find the WTF.



Looks to me like sample code straight out of an Andre Lamothe book written in 1995

Not impossible. Never read the book though. And a book that simply disregards cheching values doesnt really sound too hot.

LoztInSpace

@e.thermal said:

Not impossible. Never read the book though. And a book that simply disregards cheching values doesnt really sound too hot.

That's a bit unfair. Most code in books has a lot of that kind of thing stripped out to focus on illustrating the subject at hand without adding too much noise. It's pretty common for error checking, return codes, exceptions etc to fall by the wayside to aid clarity.

Ulvhamne

@LoztInSpace said:

@e.thermal said:
Not impossible. Never read the book though. And a book that simply disregards cheching values doesnt really sound too hot.

That's a bit unfair. Most code in books has a lot of that kind of thing stripped out to focus on illustrating the subject at hand without adding too much noise. It's pretty common for error checking, return codes, exceptions etc to fall by the wayside to aid clarity.

Yes, forgive me for being a bit bitter. I have to work with these obnoxiously slightly broken classes. ;)
But any books that show off this terrain class... *Shudder*