TheCherno / Sparky

Cross-Platform High Performance 2D/3D game engine for people like me who like to write code.
Apache License 2.0
1.1k stars 222 forks source link

Orthographics matrix issue and vector inconsistency #34

Open CdTCzech opened 9 years ago

CdTCzech commented 9 years ago

Hi Cherno, Thanks for great series and sorry for my bad English. I'm actually at episode 6. But after I watched episode 5(.5) think that I found one issue and one inconsistency in your code which can be "weird" for begginers.

[1] Issue in mat4.cpp in orthographic method. In all your branches you have:

mat4 mat4::orthographic(float left, float right, float bottom, float top, float near, float far)
{
    mat4 result(1.0f);

    result.elements[0 + 0 * 4] = 2.0f / (right - left);

    result.elements[1 + 1 * 4] = 2.0f / (top - bottom);

    result.elements[2 + 2 * 4] = 2.0f / (near - far);

    result.elements[0 + 3 * 4] = (left + right) / (left - right);
    result.elements[1 + 3 * 4] = (bottom + top) / (bottom - top);
    result.elements[2 + 3 * 4] = (far + near) / (far - near);

    return result;
}

The problem is, I think, in line:

 result.elements[2 + 3 * 4] = (far + near) / (far - near);

Because "original" is

result.elements[2 + 3 * 4] = - (far + near) / (far - near);

So your line should be:

 result.elements[2 + 3 * 4] = (far + near) / (near - far);

[2] There is inconsistency in your code. I know - it is because of union which works only with PODs.

vec2 and vec3 have void parameter constuctor defined to fill these vectors with zeroes. vec4 have this constructor set as "= default". So your struct variables are uninitialized.

So if you have:

sparky::maths::vec2 a;
sparky::maths::vec3 b;
float aa = a.x;
float bb = b.x;

Than aa and bb is zeroes. But if you try this (in VS 2015 RC in my case):

sparky::maths::vec4 c;
float cc = c.x;

You will get compile time error:

Severity Code Description
Error C4700 uninitialized local variable 'c' used

Of course you can still get c filled with zeroes. For example like this:

sparky::maths::vec4 c{};
or
sparky::maths::vec4 c = sparky::maths::vec4();

To fix this you can set constructors of vec2 and vec3 to "= default" as well or instead of union use casting or... whatever - it is totally up to you.

Thanks, and have a nice day, CdTCzech

PedDavid commented 8 years ago

[1] http://www.songho.ca/opengl/gl_projectionmatrix.html seems to be right in his code

TheCherno commented 8 years ago

Hi CdTCzech,

Thanks for your issue. I'll definitely take a look at that vector issue, since that obviously wasn't my intention.

Regarding the matrix issue: both methods are correct, and will yield the same result. -(far + near) / (far - near) and (far + near) / (near - far) are the same.