Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Matrix3 with System.Numerics.Matrix3x2 #5078

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eoineoineoin
Copy link
Contributor

@eoineoineoin eoineoineoin commented Apr 28, 2024

Matrix3 multiplication multiplies matrices in the wrong order:

Matrix3.Multiply(in Matrix3 left, in Matrix3 right, out Matrix3 result) actually calculated result = (right)(left)
Matrix3.operator *(in Matrix3 left, in Matrix3 right) actually calculated result = (right)(left)

Matrix4 code does not have this issue. Existing code using these methods actually had the arguments in the wrong order, in order to achieve the correct result. This causes the code to be difficult to read - I've repeatedly read code and thought "surely these are being multiplied in the wrong order" - and hard to write, as the documentation is incorrect.

To fix the problem and identify calling sites of the offending functions, I disabled the operator*, and renamed Multiply(), then fixed up the compile errors. Then, I fixed the functions, restored operator*, renamed Multiply() back to the original name and fixed up the compile errors again, changing the argument order.

Changes to content are actually quite limited, but not sure how to communicate this change to forks.
PR with content changes: space-wizards/space-station-14#27443

@ElectroJr
Copy link
Contributor

Instead of doing this refactor, which is quite likely to introduce bugs along the way, we should just finally switch to using System.Numerics.Matrix3x2. We've already made that switch for vectors and (IIRC) have agreed to do the same for matrices.

@eoineoineoin
Copy link
Contributor Author

I took a bit of time yesterday evening to try replacing Matrix3 with System.Numerics.Matrix3x2.
There's a ton of compile errors, but most of them are pretty benign, lots of m.Transform(v) -> Vector2.Transform(v, m). Couple of hairy ones around Robust.Client.Graphics as the matrices are the transpose of what they previously were; my main concerns are:

  1. Matrix3x2 transforms vectors on the left (vM), while the existing code and the GL shaders transform vectors on the right (Mv); maybe a bit weird to have two different conventions in the codebase.
  2. Matrix3x2 really represents a 3x3 matrix with an implicit third column of (0,0,1) - I'm not sure if this is always true in the existing code.

If you think this is the right way to go, I can find some more time to finish it up.

DannyFranklin

This comment was marked as spam.

@DannyFranklin

This comment was marked as spam.

@PJB3005
Copy link
Member

PJB3005 commented May 6, 2024

If you think this is the right way to go, I can find some more time to finish it up.

It is the right way to go. I already started doing it when I was doing renderer rewrite a year ago, but that has not born any fruit yet.

@eoineoineoin
Copy link
Contributor Author

There we go. Tested everything I could think of; all seems to be good.

Comment on lines 49 to 50
var buf = new float[6];
for (int i = 0; i < 6; i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stackalloc or maybe just explicitly read out the floats?

@ElectroJr ElectroJr changed the title Fix Matrix3 multiplication Replace Matrix3 with System.Numerics.Matrix3x2 May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants