search

giawa / opengl4csharp

OpenGL 4 Bindings (partially based on OpenTK) for C#
Other
232 stars 61 forks source link

Fixed matrix4 multiplication #66

Closed RDPBooooooom closed 2 years ago

RDPBooooooom commented 2 years ago

Matrix multiplication - Matrix4 x Matrix4 x Matrix4

While doing a small project I noticed that Matrix4 Matrix4 multiplication wasn't working as I would expect. When I was multiplying my model, view and projection Matrix I got some weird results. Normally you read matrix multiplication from right to the left. Like this for example "projection view * model". But this wasn't the case with the current implementation. In my opinion this is unexpected and wrong behaviour. I corrected that.

Tests

Additionally I added a test case to make sure this behaviour can be tested. Besides that I tried to fix the tests because they seemed abandoned? I added a solution configuration for "Use_numerics" and added additional preprocessor directives. You can use the normal solution configuration to test without System.Numerics. All tests are passing with both configurations. (Note: Vector3StaticMethods seems to fail sometimes. But that behaviour was already there before my changes)

For determining the test results I used a online calculator. See the following links for the result: m3 result(m1 m2) m2 * m1 MatrixMultiplication

Let me know what you think! Thanks for the Library btw.

giawa commented 2 years ago

Hi there, thanks for submitting a PR! I'm happy to hear you're using this library.

DirectX and OpenGL differ in their matrix multiplication order due to column major order versus row major order. The order of operations in this OpenGL library matches the expected order of OpenGL itself, and it is a deliberate design decision.

That being said, I do think it's great to update the unit tests, so I would still be happy to move forward with this PR with the breaking change of matrix multiplication removed and the whitespace issues addressed. Let me know what you think.

RDPBooooooom commented 2 years ago

Hi, I reverted the matrix multiplication changes. Wasn't aware of the column major and row major order. Still learning a lot about OpenGl and related subjects. I believe I fixed all the whitespace and indentation issues. I also adjusted the Matrix4 tests to run with the reverted change to multiplication. Let me know if you see anything else!

giawa commented 2 years ago

Thanks for submitting those changes! I've run an auto format on the code and fixed up the using statements for both USE_NUMERICS and without. Do you mind taking a quick look to make sure you're okay with those changes, and then I'll merge this?

RDPBooooooom commented 2 years ago

lgtm, seems like everything is working! Although I'm not sure why 6dc043b is necessary. Code was compiling for me without those changes in both configurations. It's fine for me if you merge it like this, but I would be interested in an explanation on this issue.

giawa commented 2 years ago

When you were trying it out were you also modifying USE_NUMERICS in the opengl4csharp library as well?