raysan5 / raylib

A simple and easy-to-use library to enjoy videogames programming
http://www.raylib.com
zlib License
22.17k stars 2.24k forks source link

[raymath] MatrixMultiply multiplies matricies in wrong order #3039

Closed al1-ce closed 1 year ago

al1-ce commented 1 year ago

Test case comparing Raylib implementation to GLM:

#include <raylib.h>
#include <raymath.h>
#include <stdio.h>
#include "glm/mat4x4.hpp"
#include <glm/vec3.hpp> // glm::vec3
#include <glm/vec4.hpp> // glm::vec4
#include <glm/mat4x4.hpp> // glm::mat4
#include <glm/ext/matrix_transform.hpp> // glm::translate, glm::rotate, glm::scale
#include <glm/ext/matrix_clip_space.hpp> // glm::perspective
#include <glm/ext/scalar_constants.hpp> // glm::pi

void printMatrix(Matrix mat) {
    printf("%.2f %.2f %.2f %.2f\n", mat.m0, mat.m4, mat.m8, mat.m12);
    printf("%.2f %.2f %.2f %.2f\n", mat.m1, mat.m5, mat.m9, mat.m13);
    printf("%.2f %.2f %.2f %.2f\n", mat.m2, mat.m6, mat.m10, mat.m14);
    printf("%.2f %.2f %.2f %.2f\n", mat.m3, mat.m7, mat.m11, mat.m15);
    printf("\n");
}

int main() {
    struct Matrix view = MatrixLookAt((Vector3) {0.0f, 2.0f, 5.0f}, (Vector3) {1.0f, 20.0f, 1.0f}, (Vector3) {0.5f, 1.0f, 0.0f});
    struct Matrix proj = MatrixPerspective(0.78f, 9.0f / 16.0f, 2.0f, 125.0f);
    struct Matrix vipr = MatrixMultiply(view, proj);
    struct Matrix vipr2 = MatrixMultiply(proj, view);
    glm::mat4 Projection = glm::perspective(0.78f, 9.0f / 16.0f, 2.0f, 125.f);
    glm::mat4 View = glm::lookAt(glm::vec3(0.0f, 2.0f, 5.0f), glm::vec3(1.0f, 20.0f, 1.0f), glm::vec3(0.5f, 1.0f, 0.0f));
    glm::mat4 ViewProj = View * Projection;  
    printf("raylib: MatrixMultiply(view, proj)\n");
    printMatrix(vipr);
    printf("raylib: MatrixMultiply(proj, view)\n");
    printMatrix(vipr2);
    printf("glm: MatrixMultiply(view, proj), aka view * proj\n");
    printf("%.2f %.2f %.2f %.2f\n", ViewProj[0][0], ViewProj[1][0], ViewProj[2][0], ViewProj[2][0]);
    printf("%.2f %.2f %.2f %.2f\n", ViewProj[0][1], ViewProj[1][1], ViewProj[2][1], ViewProj[2][1]);
    printf("%.2f %.2f %.2f %.2f\n", ViewProj[0][2], ViewProj[1][2], ViewProj[2][2], ViewProj[2][2]);
    printf("%.2f %.2f %.2f %.2f\n", ViewProj[0][3], ViewProj[1][3], ViewProj[2][3], ViewProj[2][3]);
    printf("\n");
}

Test result

raylib: MatrixMultiply(view, proj)
1.89 -0.94 -3.78 20.76
2.18 0.11 1.06 -5.55
0.06 1.01 -0.22 -4.96
0.05 0.97 -0.22 -0.87

raylib: MatrixMultiply(proj, view)
1.89 -0.53 -3.90 3.55
3.88 0.11 1.83 -1.78
-0.23 -2.37 -1.09 -0.88
0.00 0.00 -1.00 0.00

glm: MatrixMultiply(view, proj), aka view * proj
1.89 -0.53 -3.90 -3.90
3.88 0.11 1.83 1.83
-0.23 -2.37 -1.09 -1.09
0.00 0.00 -1.00 -1.00

Produced result indicates that raylib's MatrixMultiply multiplies matrices right to left instead of left to right.

Possibly same issue in other functions that use matrix multiplication (i.e. Vector3Unproject)

I know it's a duplicate of #3036, but please, look at code I provided. It uses GLM, same print order, same operations.

raysan5 commented 1 year ago

@al1-ce Thanks for the feedback, no plans to change it for the moment.

orcmid commented 1 year ago

@raysan5 It would be useful if the description of MatrixMultiply was that it produced the T(L) x T(R) since that's unexpected, as is knowing that the result is T(L x R). This is not an obvious convention.

@al1-ce I notice that the raylib proj x view and the glm view * proj differ in some unexpected places. The print statements are producing column ViewProj[2] twice, instead of ViewProj[3].

raysan5 commented 1 year ago

@orcmid A comment about raymath conventions was already added to header description to clarify it.

chriscamacho commented 1 year ago
raylib: MatrixMultiply(proj, view)
1.89 -0.53 -3.90 3.55
3.88 0.11 1.83 -1.78
-0.23 -2.37 -1.09 -0.88
0.00 0.00 -1.00 0.00

glm: MatrixMultiply(view, proj), aka view * proj
1.89 -0.53 -3.90 3.55
3.88 0.11 1.83 -1.78
-0.23 -2.37 -1.09 -0.88
0.00 0.00 -1.00 0.00

with corrected "test"

orcmid commented 1 year ago

@raysan5

@orcmid A comment about raymath conventions was already added to header description to clarify it.

I see the important list of conventions now. I don't think the added sentence helps. I think I would say, "in Matrix struct order [m0 m4 m8 m12] identifies row 0 (and a transposed column 0) and [m0 m1 m2 m3] identifies column 0 (and a transposed row 0) ." That might guide the thinking better.

Also, I think comments where the default interpretation is atypical but in accordance with the conventions still needs clarity at the individual function level.

I apologize for not appreciating the conventions with regard to transpositions. It still would not have been obvious from the namings what was intended. I can understand how avoiding separate transpositions and instead using transposition-propagation math into operand interchange and/or coordinate switching is very handy along with the other efforts to flatten and unwind the implementations.

chriscamacho commented 1 year ago

I've done some fairly advanced stuff with raymath, and the only time I had to mess with m0 - m15 was when actually contributing to raymath.h for a bug fix... yeah it seems counter-intuitive, but then its either one way or the other and this way might be initially confusing, but it is more convenient behind the scenes...

and honestly your

"in Matrix struct order [m0 m4 m8 m12] identifies row 0 (and a transposed column 0) and [m0 m1 m2 m3] identifies column 0 (and a transposed row 0) ."

could be just as confusing!

the alternative would be to refactor it (with an IDE) in the form of Mcr where c=column and r=row

I don't think anything outside raymath uses m0 - m15, so perhaps thought should be given to making it an opaque structure....

orcmid commented 1 year ago

@chriscamacho Yes, I was hoping that way of putting it would lead people to think about it. You do need to examine the declaration for struct Matrix though.

I think the big trick in raymath.h is the use of structures to wrap fixed-size vectors so that they can be passed into functions by value and also delivered as the result of functions. Doing assignments to structures using {value0, ..., valueN} is by 1-to-1 correspondence with the definition of struct fields and doesn't have anything to do with arrays (except when it does). It is also helpful to understand that the ;-s in the struct Matric definition are merely suggestive and are unimportant in C Language, just alternative ,-s there.

My take is that struct Matrix is a different thing because it is intended to order a 2-dimensional 4 x 4 array but not as a C Language array (i.e., a vector of [pointers to] vectors). That's a good thing. But there is no C Language sense of even a simple vector there (and likewise with struct Vector2 and struct Vector3, and member names like x, y, z, and w, whether figuratively or in actual struct definitions).

As we have just observed, this is all very clever but the cognitive load is intense. I must look at the wiki to see if there is any clarity about this.

I disagree about appropriating well-known names of mathematical functions for alternative views that pretty much destroy anyone's degree of linear algebra understanding. It is too late to do anything about that, although I would, for myself, choose alternative names alongside those, using ones that reveal the connection to established linear algebra conventions in an explicit way. I will do that to preserve my own sanity and be able to work with raymath in a confident manner. It's that or give up on raylib and I am not ready to take such an irrevocable step.

PS: The idea of getting a transposition "for free" where one might be needed in further operations by a kind of transposition-propagation is very nice, a bit like propagating-away "~" in Boolean propositions and (unary) "-" in numerical procedures. But ... (silent) cognitive load?

chriscamacho commented 1 year ago

@orcmid these days if I need a complex matrix stack for some unusual transformation, I just use a tool I put together myself https://bedroomcoders.co.uk/matrix-chain/

orcmid commented 1 year ago

@chriscamacho

@orcmid these days if I need a complex matrix stack for some unusual transformation, I just use a tool I put together myself https://bedroomcoders.co.uk/matrix-chain/

Matrix Chain seems very nice, especially for having a way to visualize the operations under the raymath geometric interpretation.

I didn't know people still use t.gz as the package of choice. Nicely retro. I seem to have misplaced tools for that, though I'm certain there's some in my archives.

chriscamacho commented 1 year ago

.tgz - if it works why fix it...

orcmid commented 1 year ago

@chriscamacho tgz is fine if you are essentially devoted to *nix platforms. I remember arc and PKzip too. There was an alternative to tar but I forget the name now. I suppose my PC focus has .zip be my package of choice :). WinZip would open .t.gz and I must have to find an installation of it in your honor. :).

chriscamacho commented 1 year ago

@orcmid or... install WSL and save yourself the horror that is well the whole windows stack really ;-) ...