Jelajahi Sumber

LibWeb: Fix DOMMatrix Gfx::Matrix row/column ordering

The matrix used in the spec is column-major but Gfx::Matrix4x4 is
row-major so we need to transpose the values. This will fix internal
operations on that matrix. Because we also transposed the readonly
matrix property getters the matrix is again transposed when reading
so the JavaScript world only sees a column-major matrix.
Bastiaan van der Plaat 1 tahun lalu
induk
melakukan
57a1d99cf4

+ 13 - 13
Userland/Libraries/LibWeb/Geometry/DOMMatrix.cpp

@@ -84,14 +84,14 @@ void DOMMatrix::set_m11(double value)
 void DOMMatrix::set_m12(double value)
 {
     // For the DOMMatrix interface, setting the m12 or the b attribute must set the m12 element to the new value.
-    m_matrix.elements()[0][1] = value;
+    m_matrix.elements()[1][0] = value;
 }
 
 // https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-m13
 void DOMMatrix::set_m13(double value)
 {
     // For the DOMMatrix interface, setting the m13 attribute must set the m13 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[0][2] = value;
+    m_matrix.elements()[2][0] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -100,7 +100,7 @@ void DOMMatrix::set_m13(double value)
 void DOMMatrix::set_m14(double value)
 {
     // For the DOMMatrix interface, setting the m14 attribute must set the m14 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[0][3] = value;
+    m_matrix.elements()[3][0] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -109,7 +109,7 @@ void DOMMatrix::set_m14(double value)
 void DOMMatrix::set_m21(double value)
 {
     // For the DOMMatrix interface, setting the m21 or the c attribute must set the m21 element to the new value.
-    m_matrix.elements()[1][0] = value;
+    m_matrix.elements()[0][1] = value;
 }
 
 // https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-m22
@@ -123,7 +123,7 @@ void DOMMatrix::set_m22(double value)
 void DOMMatrix::set_m23(double value)
 {
     // For the DOMMatrix interface, setting the m23 attribute must set the m23 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[1][2] = value;
+    m_matrix.elements()[2][1] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -132,7 +132,7 @@ void DOMMatrix::set_m23(double value)
 void DOMMatrix::set_m24(double value)
 {
     // For the DOMMatrix interface, setting the m24 attribute must set the m24 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[1][3] = value;
+    m_matrix.elements()[3][1] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -141,7 +141,7 @@ void DOMMatrix::set_m24(double value)
 void DOMMatrix::set_m31(double value)
 {
     // For the DOMMatrix interface, setting the m31 attribute must set the m31 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[2][0] = value;
+    m_matrix.elements()[0][2] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -150,7 +150,7 @@ void DOMMatrix::set_m31(double value)
 void DOMMatrix::set_m32(double value)
 {
     // For the DOMMatrix interface, setting the m32 attribute must set the m32 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[2][1] = value;
+    m_matrix.elements()[1][2] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -168,7 +168,7 @@ void DOMMatrix::set_m33(double value)
 void DOMMatrix::set_m34(double value)
 {
     // For the DOMMatrix interface, setting the m34 attribute must set the m34 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[2][3] = value;
+    m_matrix.elements()[3][2] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -177,21 +177,21 @@ void DOMMatrix::set_m34(double value)
 void DOMMatrix::set_m41(double value)
 {
     // For the DOMMatrix interface, setting the m41 or the e attribute must set the m41 element to the new value.
-    m_matrix.elements()[3][0] = value;
+    m_matrix.elements()[0][3] = value;
 }
 
 // https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-m42
 void DOMMatrix::set_m42(double value)
 {
     // For the DOMMatrix interface, setting the m42 or the f attribute must set the m42 element to the new value.
-    m_matrix.elements()[3][1] = value;
+    m_matrix.elements()[1][3] = value;
 }
 
 // https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-m43
 void DOMMatrix::set_m43(double value)
 {
     // For the DOMMatrix interface, setting the m43 attribute must set the m43 element to the new value and, if the new value is not 0 or -0, set is 2D to false.
-    m_matrix.elements()[3][2] = value;
+    m_matrix.elements()[2][3] = value;
     if (value != 0.0 && value != -0.0)
         m_is_2d = false;
 }
@@ -260,7 +260,7 @@ JS::NonnullGCPtr<DOMMatrix> DOMMatrix::invert_self()
     if (!is_invertible) {
         for (u8 i = 0; i < 4; i++) {
             for (u8 j = 0; j < 4; j++)
-                m_matrix.elements()[i][j] = NAN;
+                m_matrix.elements()[j][i] = NAN;
         }
         m_is_2d = false;
     }

+ 26 - 22
Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.cpp

@@ -117,25 +117,27 @@ void DOMMatrixReadOnly::initialize(JS::Realm& realm)
 // https://drafts.fxtf.org/geometry/#create-a-2d-matrix
 void DOMMatrixReadOnly::initialize_from_create_2d_matrix(double m11, double m12, double m21, double m22, double m41, double m42)
 {
+    // NOTE: The matrix used in the spec is column-major (https://drafts.fxtf.org/geometry/#4x4-abstract-matrix) but Gfx::Matrix4x4 is row-major so we need to transpose the values.
+
     // 1. Let matrix be a new instance of type.
     // 2. Set m11 element, m12 element, m21 element, m22 element, m41 element and m42 element to the values of init in order starting with the first value.
     auto* elements = m_matrix.elements();
     elements[0][0] = m11;
-    elements[0][1] = m12;
-    elements[1][0] = m21;
+    elements[1][0] = m12;
+    elements[0][1] = m21;
     elements[1][1] = m22;
-    elements[3][0] = m41;
-    elements[3][1] = m42;
+    elements[0][3] = m41;
+    elements[1][3] = m42;
 
     // 3. Set m13 element, m14 element, m23 element, m24 element, m31 element, m32 element, m34 element, and m43 element to 0.
-    elements[0][2] = 0.0;
-    elements[0][3] = 0.0;
-    elements[1][2] = 0.0;
-    elements[1][3] = 0.0;
     elements[2][0] = 0.0;
+    elements[3][0] = 0.0;
     elements[2][1] = 0.0;
-    elements[2][3] = 0.0;
+    elements[3][1] = 0.0;
+    elements[0][2] = 0.0;
+    elements[1][2] = 0.0;
     elements[3][2] = 0.0;
+    elements[2][3] = 0.0;
 
     // 4. Set m33 element and m44 element to 1.
     elements[2][2] = 1.0;
@@ -150,24 +152,26 @@ void DOMMatrixReadOnly::initialize_from_create_2d_matrix(double m11, double m12,
 // https://drafts.fxtf.org/geometry/#create-a-3d-matrix
 void DOMMatrixReadOnly::initialize_from_create_3d_matrix(double m11, double m12, double m13, double m14, double m21, double m22, double m23, double m24, double m31, double m32, double m33, double m34, double m41, double m42, double m43, double m44)
 {
+    // NOTE: The matrix used in the spec is column-major (https://drafts.fxtf.org/geometry/#4x4-abstract-matrix) but Gfx::Matrix4x4 is row-major so we need to transpose the values.
+
     // 1. Let matrix be a new instance of type.
     // 2. Set m11 element to m44 element to the values of init in column-major order.
     auto* elements = m_matrix.elements();
     elements[0][0] = m11;
-    elements[0][1] = m12;
-    elements[0][2] = m13;
-    elements[0][3] = m14;
-    elements[1][0] = m21;
+    elements[1][0] = m12;
+    elements[2][0] = m13;
+    elements[3][0] = m14;
+    elements[0][1] = m21;
     elements[1][1] = m22;
-    elements[1][2] = m23;
-    elements[1][3] = m24;
-    elements[2][0] = m31;
-    elements[2][1] = m32;
+    elements[2][1] = m23;
+    elements[3][1] = m24;
+    elements[0][2] = m31;
+    elements[1][2] = m32;
     elements[2][2] = m33;
-    elements[2][3] = m34;
-    elements[3][0] = m41;
-    elements[3][1] = m42;
-    elements[3][2] = m43;
+    elements[3][2] = m34;
+    elements[0][3] = m41;
+    elements[1][3] = m42;
+    elements[2][3] = m43;
     elements[3][3] = m44;
 
     // 3. Set is 2D to false.
@@ -263,7 +267,7 @@ JS::NonnullGCPtr<DOMPoint> DOMMatrixReadOnly::transform_point(DOMPointReadOnly c
 
     // 6. Set pointVector to pointVector pre-multiplied by matrix.
     // This is really a post multiply because of the transposed m_matrix.
-    point_vector = m_matrix.transpose() * point_vector;
+    point_vector = m_matrix * point_vector;
 
     // 7. Let transformedPoint be a new DOMPoint object.
     // 8. Set transformedPoint’s x coordinate to pointVector’s first element.

+ 14 - 12
Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.h

@@ -55,20 +55,20 @@ public:
 
     // https://drafts.fxtf.org/geometry/#dommatrix-attributes
     double m11() const { return m_matrix.elements()[0][0]; }
-    double m12() const { return m_matrix.elements()[0][1]; }
-    double m13() const { return m_matrix.elements()[0][2]; }
-    double m14() const { return m_matrix.elements()[0][3]; }
-    double m21() const { return m_matrix.elements()[1][0]; }
+    double m12() const { return m_matrix.elements()[1][0]; }
+    double m13() const { return m_matrix.elements()[2][0]; }
+    double m14() const { return m_matrix.elements()[3][0]; }
+    double m21() const { return m_matrix.elements()[0][1]; }
     double m22() const { return m_matrix.elements()[1][1]; }
-    double m23() const { return m_matrix.elements()[1][2]; }
-    double m24() const { return m_matrix.elements()[1][3]; }
-    double m31() const { return m_matrix.elements()[2][0]; }
-    double m32() const { return m_matrix.elements()[2][1]; }
+    double m23() const { return m_matrix.elements()[2][1]; }
+    double m24() const { return m_matrix.elements()[3][1]; }
+    double m31() const { return m_matrix.elements()[0][2]; }
+    double m32() const { return m_matrix.elements()[1][2]; }
     double m33() const { return m_matrix.elements()[2][2]; }
-    double m34() const { return m_matrix.elements()[2][3]; }
-    double m41() const { return m_matrix.elements()[3][0]; }
-    double m42() const { return m_matrix.elements()[3][1]; }
-    double m43() const { return m_matrix.elements()[3][2]; }
+    double m34() const { return m_matrix.elements()[3][2]; }
+    double m41() const { return m_matrix.elements()[0][3]; }
+    double m42() const { return m_matrix.elements()[1][3]; }
+    double m43() const { return m_matrix.elements()[2][3]; }
     double m44() const { return m_matrix.elements()[3][3]; }
 
     double a() const { return m11(); }
@@ -95,7 +95,9 @@ protected:
     DOMMatrixReadOnly(JS::Realm&, Optional<Variant<String, Vector<double>>> const& init);
     DOMMatrixReadOnly(JS::Realm&, DOMMatrixReadOnly const& other);
 
+    // NOTE: The matrix used in the spec is column-major (https://drafts.fxtf.org/geometry/#4x4-abstract-matrix) but Gfx::Matrix4x4 is row-major so we need to transpose the values.
     Gfx::DoubleMatrix4x4 m_matrix { Gfx::DoubleMatrix4x4::identity() };
+
     bool m_is_2d { true };
 
 private: