RobustToolbox icon indicating copy to clipboard operation
RobustToolbox copied to clipboard

Fix Matrix3 multiplication

Open eoineoineoin opened this issue 1 year ago • 5 comments
trafficstars

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

eoineoineoin avatar Apr 28 '24 15:04 eoineoineoin

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.

ElectroJr avatar Apr 28 '24 16:04 ElectroJr

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.

eoineoineoin avatar Apr 30 '24 16:04 eoineoineoin

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);
}

}

DannyFranklin avatar May 02 '24 00:05 DannyFranklin

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.

PJB3005 avatar May 06 '24 21:05 PJB3005

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

eoineoineoin avatar May 18 '24 11:05 eoineoineoin

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

ElectroJr avatar Jun 01 '24 08:06 ElectroJr

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.

eoineoineoin avatar Jun 01 '24 12:06 eoineoineoin