flyx / OpenGLAda

Thick Ada binding for OpenGL and GLFW
flyx.github.io/OpenGLAda/
MIT License
95 stars 13 forks source link

Matrix Multiplication #66

Closed rogermc2 closed 7 years ago

rogermc2 commented 7 years ago

After working with right multiplication for a while I've realized that left multiplication could probably be implemented by reversing the order of multiplication in the matrix multiplication routines. That is, implement (left, right) by right left multiplication. This would then provide for left multiplication which is generally conventional in mathematics, physics and engineering as well as computer graphics theory. That is, R(object) rather than (object)R. This would also be consistent with GLSL as, when coding shaders, multiplication is left right even though the matrix convention is column, row order. For example, projection view in shaders whereas in OpenGLAda I have to use viewprojection. Therefore, if possible, can the matrix multiplication routines be changed to implement this?

flyx commented 7 years ago

Not sure I'm following you.

The basic pipeline is that a vector in OpenGL is transformed first by the model, then by the view, and finally by the projection matrix. Ergo

result = projection * (view * (model * vector))

matrix multiplication is associative (while not commutative), so this equals

result = (projection * view * model) * vector

Which is how it should work. If, for some reason, OpenGLAda matrix multiplication doesn't work like this, that's a bug. This might have been introduced by switching from row-major to column-major format, although I assumed that the matrix multiplication routines would be orthogonal.

Can you give me some test data? From your text, I am not quite sure how that problem unfolds.

rogermc2 commented 7 years ago

I absolutely agree that this is how it should work. Your description is excellent. Although, I'm not clear on what you mean by orthogonal in this context.

Using the mathematical rule for matrix transposition: T(AB) = T(B)T(A) (where T means Transpose)

What needs to be sent to the vertex shader is T(AB) = T(B)T(A) However, we are sending T(A)T(B) = T(AB) because (left, right) = left right This is why I currently have to send view projection to the shader instead of projection view. To fix this, I think * needs to be:

* (left, right) = right * left instead of * (left, right) =  left * right.

I think that a user would get the desired result from projection * view without being aware of the transpositions being used. The attached program provides examples of these ideas. With correct multiplication, what I imagine is that the "What we have to do now" and "What is conventional" results should swap. src.zip

flyx commented 7 years ago

Why do we need to send transposed matrices to the vertex shader? I thought we eliminated the need for transposition by switching to a column-major format? (Forgive me that I do not have time to look up the details in books.)

Oh, and I meant orthogonal in metaphorical sense, i.e. I thought the matrix multiplication algorithm would be independent of whether we use row-major or column-major format.

rogermc2 commented 7 years ago

I probably haven't expressed myself clearly. Currently we ARE sending transposed matrices to the vertex shader. Switching to a column-major format has achieved this. The projection, view or MPV matrices that we send are in transposed form. Sending matrices that haven't been produced by multiplication works fine. The problem is with the multiplication of matrices that produces the result that we send (unless one of a pair is the identity matrix of course). To get the right result sending a MVP_Matrix I have to use MVP_Matrix := View_Matrix Projection_Matrix; because View_Matrix and Projection_Matrix, being column-major, are effectively transposed matrices but when multiplied produce the wrong result for the multiplication of transposed matrices which should obey the rule T(AB) = T(B)T(A). In effect, I think currently that what we are doing is T(AB) = T(A)T(B) which is "wrong" but which I correct for by MVP_Matrix := View_Matrix Projection_Matrix; that is swapping View_Matrix and Projection_Matrix from what they conventionally should be. I hope this makes sense. Its taken me quite a while to produce with quite a few failed attempts at making some sort of sense. Unfortunately explaining this sort of thing stretches my abilities to the limit! I'm pretty sure that

* (left, right) = right * left 

should fix it. Otherwise we would have to go back to the original row-major format and send Transpose (a matrix) to the shader which I originally did to satisfy the shader. I think that the rule T(B)T(A) = T(AB) indicates that the matrix multiplication algorithm is not independent of whether we use row-major or column-major format. I hope this is helpful.

flyx commented 7 years ago

Okay, I now understand the problem. I had difficulty because from my understanding, switching from row-major to column-major format is not transposing, but merely a representational change. However, as you explain, this means that the multiplication algorithm must be adapted to the new layout, because it does yield a different result now. So this issues boils down to fixing the matrix multiplication implementation, so that (A * B) * Vector yields the same vector as it did before.

rogermc2 commented 7 years ago

My original understanding was also that switching from row-major to column-major format is not transposing, but merely a representational change. However, I was also concerned about the T(AB) = T(B)T(A) rule which lead me to my original (unconventional) solution. T(AB) = T(B)T(A) led me to the solution but that doesn't mean that I understand what's going on completely! Hopefully the fine detail won't be too difficult. I realize, as always, that there may be other things to consider that cause what seems to be a simple solution not so simple.

flyx commented 7 years ago

I fixed the "*" (Matrix, Vector) as well as "*" (Matrix, Matrix). Please evaluate.

rogermc2 commented 7 years ago

Looks good! I tried the textured_cube_example (Tutorial 5) which worked correctly with

MVP_Matrix :=  Projection_Matrix * View_Matrix * Model_Matrix;

Other examples successfully updated in a separate branch.