-
Notifications
You must be signed in to change notification settings - Fork 369
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
base: master
Are you sure you want to change the base?
Replace Matrix3 with System.Numerics.Matrix3x2 #5078
Conversation
Instead of doing this refactor, which is quite likely to introduce bugs along the way, we should just finally switch to using |
I took a bit of time yesterday evening to try replacing Matrix3 with System.Numerics.Matrix3x2.
If you think this is the right way to go, I can find some more time to finish it up. |
This comment was marked as spam.
This comment was marked as spam.
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. |
de31dc5
to
c0fbe69
Compare
There we go. Tested everything I could think of; all seems to be good. |
var buf = new float[6]; | ||
for (int i = 0; i < 6; i++) |
There was a problem hiding this comment.
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?
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