RobustToolbox
RobustToolbox copied to clipboard
Fix Matrix3 multiplication
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: https://github.com/space-wizards/space-station-14/pull/27443
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.
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:
- 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.
- 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.
for (int i = 0; i < m; i++) { for (int j = 0; j < n; j++) { resultMatrix[i][j] = 0; } }
// Perform matrix multiplication
for (int i = 0; i < m; i++) {
for (int j = 0; j < n; j++) {
for (int k = 0; k < p; k++) {
resultMatrix[i][j] += firstMatrix[i][k] * secondMatrix[k][j];
}
}
}
}
// Function to display a matrix
static void displayMatrix(int[][] matrix) {
for (int[] row : matrix) {
for (int element : row) {
System.out.print(element + " ");
}
System.out.println();
}
}
public static void main(String[] args) {
int[][] firstMatrix = {{1, 2, 3}, {4, 5, 6}};
int[][] secondMatrix = {{7, 8}, {9, 10}, {11, 12}};
int[][] resultMatrix = new int[firstMatrix.length][secondMatrix[0].length];
multiplyMatrices(firstMatrix, secondMatrix, resultMatrix);
System.out.println("First Matrix:");
displayMatrix(firstMatrix);
System.out.println("Second Matrix:");
displayMatrix(secondMatrix);
System.out.println("Result Matrix:");
displayMatrix(resultMatrix);
}
}
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.
There we go. Tested everything I could think of; all seems to be good.
Is there a specific reason its still marked as a draft? Because other than making sure it all works I'd be happy to fix conflicts & transition to matrix3x2 this weekend
Is there a specific reason its still marked as a draft? Because other than making sure it all works I'd be happy to fix conflicts & transition to matrix3x2 this weekend
No reason; just leftover from before I had the confirmation that System.Numerics.Matrix3x2 was the way to go. I've merged the latest and updated my branches.