Raylib: [rlgl] rlPushMatrix() is incorrect

[define-private-public]

Look at this line here:

https://github.com/raysan5/raylib/blob/master/src/rlgl.c#L413

glPushMatrix() is supposed to push on a new matrix onto the stack, with cloning the previous one. Though rlPushMatrix(), it's also loading the identity matrix (because of a call to rlLoadIdentity()). This is incorrect if you want to emulate the original OpenGL fixed function API.

[raysan5]

Actually rlgl module pretends to be a kind-of-emulation layer but not a direct implementation (like Mesa project).

Do you know any specific case where reseting the current matrix after pushing it to stack could suppose an issue?

[define-private-public]

Resetting the current matrix to the identity matrix kind of destroys the purpose of the stack. It's purpose was that you could chain on other transforms (scale, rotate, translate, etc...) upon an existing base transform. And then (with ease) call the pop function to undo those transforms in a single command, instead of having to reverse them.

Take for example a the skeleton rig for a stick figure. You could push on the first (base) matrix transforms for the torso, then push/pop on the matrix transforms for the legs, arms, & head; though have these transforms be relative to the body. So if someone moved the body, it would the move the arms, legs, and head along with it.

With how rlPushMatrix() right now functions, you can't do that. It kind of makes push/pop functionality useless. Take a look at the official docs for glPushMatrix():
https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glPushMatrix.xml

[raysan5]

Thanks for the detailed explanation. You are right, I'll review it!

[raysan5]

Corrected in commit https://github.com/raysan5/raylib/commit/ff9b486ebe61d84d14add801d67c3bbdaca111e2

[raysan5]

Just reverted this change in commit https://github.com/raysan5/raylib/commit/412c52499a4288b0b1f1b0626229ebc5ce2b5c27 because it breaks 3D shapes drawing examples, transformations are accumulated into currentMatrix and drawn doesn't work as expected.

Need to think about this change more carefully...

[questor]

short notice, without this change and the models using rlPushMatrix() it's not possible to draw a rotated cube, isn't it? I've tried the following setup:

   rlPushMatrix();
   rlRotate(45,0,0,1);
   DrawCube(...);
   rlPopMatrix();

but because DrawCube internally also does a rlPushMatrix my rotation isn't used to draw the cube.

In my test without the LoadIdentity() in rlPushMatrix() the camera is in the beginning off, but I'm not sure why.

[define-private-public]

Hey, I'm bumping this because this is kind of a semi-critical issue. It at least deserves a special tag.

[raysan5]

Hi @define-private-public! Thanks for the reminder, I couldn't find the moment to work on that...

Previously proposed change breaks shapes/textures drawing while using internal buffers...

[questor]

I think it does not break the shapes/textures, it just has the wrong initialization for the camera for your examples. I'm quite sure when you move the camera around (in the opposite direction) you will see the expected content.

[define-private-public]

This is going to be one of the more nasty/frustrating things to fixup. Do you have a list of all the things that are going to need to be adjusted?

[raysan5]

This is going to be one of the more nasty/frustrating things to fixup.

Yes, I can't find the right time to jump into this...

Do you have a list of all the things that are going to need to be adjusted?

Mainly focused on rlgl module (OpenGL abstraction layer), I'd start reviewing rlPushMatrix() and rlPopMatrix() implementations. Behaviour is not exactly the same as their OpenGL 1.1 counterparts.

[questor]

I've looked at the code and found the following: with the current implementation (with setting the matrix to identity) you do (for example) in DrawCube a rlTranslatef which leads from your logic to an absolute position for the cube (because first the matrix is set to identity by the pushmatrix).

you're using the matrix-stack like a normal stack of integers, just to safe the old value and be able to use a new one, but the matrix-stack was not meant to be used like that, is was meant to be used in an object-hierachy (think about a human 3d object consisting of a body, arms and legs). with that hierachy you can modify the matrix of the base object (the body for the human object) and by using the stack the arms and legs are automatically positioned correctly if the matrix-stack is used for drawing them (because the transformation of the body is automatically also used for the other objects).

Here is a really good explanation of the concept: https://www.processing.org/tutorials/transform2d/

My advice would be to remove the position argument for the cube and instead let the user first define the wanted transformation by placing it on top of the matrix stack and then only draw the cube (with the current implementation it's not possible for the user to let the cube rotate locally because the push-matrix clears the transform-matrix and for a rotating cube you would need a rotate AFTER the pushmatrix.). So remove the rlPushMatrix from the DrawCube (and other primitives) and remove also the translate and let the user set the matrix for the object to be drawn. Then you have a proper implementation for your primitives with respect for object-/world- and camera-space.

EDIT: I could make a pull-request with my proposal if that would help you....

[raysan5]

Hi @questor, thanks for the review.

The proposed changes require a big redesign of the library, several shapes drawing functions and most of geometric models drawing functions require breaking changes. Additionally, library usage becomes more complex than just calling a drawing function with required parameters.

Need to investigate other alternatives before going that route.

[questor]

here some comments to have some sort of work-log about the differences between raylibs handling of the matrix-stack and the opengl way:

I've analysed gl-gears how opengl handles the things and the structure is:

OpenGL (matrix-stack means a datastructure which combines elements of the stack!)
   Gears:
      glMatrixMode(PROJECTION)   mode=1
         glLoadIdentity
         glFrustum
      glMatrixMode(MODELVIEW)    mode=0
         glLoadIdentity
         glTranslate

      draw-loop:
         glPushMatrix
            glRotate
            glPushMatrix
               glTranslate
               glCallList(gear) (glVertex3f)
            glPopMatrix
            ----
            glPushMatrix
               glTranslate
               glCallList(gear)
            glPopMatrix
         glPopMatrix

   Implementation Details:
      - there are three matrix stacks, one for each mode (projection, modelview and texture)
      - glLoadIdentity loads the identity matrix to the matrix stack pointer with the index of the current matrix_mode
      - glPushMatrix goes on the stack one index higher and copies the old index matrix to the new one!
      - glPopMatrix simply goes one index lower
      - glRotate creates rotation matrix and mulleft to the top matrix stack element
      - glFrustum creates matrix and mulleft to the top matrix stack element
      - glBegin does a multiplication of the current element of MODELVIEW-stack and PROJECTION-stack to transform all vertices between begin and end

Raylib in contrast has this structure in the examples:

Raylib (Matrix-Stack means more like a software-stack datastructure to remember old values, but not to combine elements on the stack and has only one stack for everything)
   Example:
      initGraphicDevice - sets ortographic projection to matrix stack
      BeginDrawing - (depends on beeing in MODELVIEW-Mode) rlLoadIdentity, downscaleview-matrix
         Begin3DMode(camera) - PushMatrix, set Projection and Modelview based on camera
            glvertex3f....
         End3dMode - pop Projection matrix, set modelview to identity
      EndDrawing

Biggest drawback with current implementation in raylib: opengl and raylib both uses a matrix-stack, but they're completely different in it's behaviour. will continue to investigate about good solutions to this.

[raysan5]

Hi @questor!

Many thanks for the time and effort you put to track this issue! Great report provided!

raylib matrix stack implementation is way simpler than the OpenGL one and does not combine matrices on the stack, just push-pop of data.

[questor]

the big question is, how to fix this:

• keep it as it is currently, but make a big warning box on the description that it does not work like the opengl-matrix-stack (or keep it private to raylib only). has the drawback that opengl1-devices will have a different behaviour if the user uses the matrix-stack.
• make the raylib matrix stack work like the opengl one (which is used on opengl1 devices none the less).
• remove the matrix stack completely from raylib (opengl3 doesn't have something like that). raylib itself can easily be refactored to remove internal usage of the matrix stack as far as I have seen.

you could also refactor the matrix-stack to an optional component which the user can use or not. if you do this you should change the arguments to the Draw-Functions like DrawCube and so on (from "Vector3 position" to "Matrix4 objectTransform" to be able to specify translations AND rotations in object-space; without this you can't simply draw 3 independent rotating cubes without modifications in raylib itself).

[define-private-public]

@raysan5 you said there were lots of places that you would have change/adjust to work properly with the correct rlPushMatrix() implementation. Do you have an audit/list of those?

[raysan5]

@define-private-public rlPushMatrix() is only used in the following functions and, actually, it could be replaced in all of them by a direct vertex transformation.

// Module: core.c
void Begin3dMode(Camera camera);

// Module: shapes.c
void DrawLineEx(Vector2 startPos, Vector2 endPos, float thick, Color color);
void DrawRectanglePro(Rectangle rec, Vector2 origin, float rotation, Color color);
void DrawPoly(Vector2 center, int sides, float radius, float rotation, Color color);

// Module: textures.c
void DrawTexturePro(Texture2D texture, Rectangle sourceRec, Rectangle destRec, Vector2 origin, float rotation, Color tint);

// Module: models.c
void DrawCircle3D(Vector3 center, float radius, Vector3 rotationAxis, float rotationAngle, Color color);
void DrawCube(Vector3 position, float width, float height, float length, Color color);
void DrawCubeWires(Vector3 position, float width, float height, float length, Color color);
void DrawSphereEx(Vector3 centerPos, float radius, int rings, int slices, Color color);
void DrawSphereWires(Vector3 centerPos, float radius, int rings, int slices, Color color);
void DrawCylinder(Vector3 position, float radiusTop, float radiusBottom, float height, int sides, Color color);
void DrawCylinderWires(Vector3 position, float radiusTop, float radiusBottom, float height, int sides, Color color);
void DrawPlane(Vector3 centerPos, Vector2 size, Color color);
void DrawGizmo(Vector3 position);

For simple examples were matrix accumulation is not required, current implementation works like OpenGL 1.1, actually, compiling the library with OpenGL 1.1 all related examples that use any of those functions work ok. Problem comes when trying to accumulate transformations, specially noticeable while dealing with 3d animated models.

All the simple 3d shapes drawing functions are not prepared to work in combination with rlPushMatrix(), I mean, they just work with the parameters they are prepared to work with, some support rotations, others not. Actually, rlgl module is a layer below shapes and models modules. If some user has especial needs on 3d shapes drawing (i.e. support rotation), custom functions could be easily created over rlgl. Actually, for custom models transformations, it's not recommended to use the immediate mode drawing API (rlVertex(), rlTranslate()...); Model struct supports a transform matrix value and DrawModel() applies that transformation properly.

Summarizing, the effort and the breaking changes required to support a specific usage situation (and aligning to the dying OpenGL 1.1 stack) probably don't compensate the benefits of doing it. One of the objectives of raylib is being simple and easy-to-use for beginners, not only on usage terms but also understanding and allowing library internals hacking... and I think the library excels on that.

...without this you can't simply draw 3 independent rotating cubes without modifications in raylib itself

You can't using DrawSphere()... but raylib provides alternative ways to get that, for example, create 3 Models (GenMeshSphere()), modify model.transform Matrix (MatrixRotate()) and DrawModel().

[questor]

okay, will not investigate in this further.

[define-private-public]

Hey, has there been any movement on this issue lately? It's kind of a very important one for proper OpenGL compatibility.

[raysan5]

Well, actually, issue is in stand-by, I didn't close it because if I have enough time in a future I could try to look for an easy solution to align implementation with OpenGL 1.1 but as stated in my last post I have my doubts if it really worths it.

[define-private-public]

You're going to be confusing a lot of people though. Is there a document somewhere that outlines what the differences are between rlgl and the old OpenGL 1.1 API?

[raysan5]

Issue corrected in commit https://github.com/raysan5/raylib/commit/97e40ced57489f4d66d4b9bc9fe213c388e1a827.