From 57a1d99cf4e3488162c8ec2dd9783fe7a312dcca Mon Sep 17 00:00:00 2001 From: Bastiaan van der Plaat Date: Tue, 22 Aug 2023 09:46:02 +0200 Subject: [PATCH] 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. --- .../Libraries/LibWeb/Geometry/DOMMatrix.cpp | 26 +++++----- .../LibWeb/Geometry/DOMMatrixReadOnly.cpp | 48 ++++++++++--------- .../LibWeb/Geometry/DOMMatrixReadOnly.h | 26 +++++----- 3 files changed, 53 insertions(+), 47 deletions(-) diff --git a/Userland/Libraries/LibWeb/Geometry/DOMMatrix.cpp b/Userland/Libraries/LibWeb/Geometry/DOMMatrix.cpp index 35c38f3bd7..59ad0fd126 100644 --- a/Userland/Libraries/LibWeb/Geometry/DOMMatrix.cpp +++ b/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::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; } diff --git a/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.cpp b/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.cpp index 867de28b08..acc69aac2b 100644 --- a/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.cpp +++ b/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 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. diff --git a/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.h b/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.h index 64cd25d4bf..5e7ed1a5b7 100644 --- a/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.h +++ b/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>> 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: