improved B2DHomMatrix

since we know that this is a matrix only used for 2D transforms,
we know that the last row of the matrix is always { 0, 0, 1 }.

Therefore, we don't need to store that information, and
we can simplify some of the computations.

Also remove operations like operator+ which are not legal for
such a matrix.

Change-Id: I482de9a45ebbedf79e3b6033575aab590e61c2d5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/151909
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

4 files changed, 102 insertions, 193 deletions

diff --git a/basegfx/source/matrix/b2dhommatrix.cxx b/basegfx/source/matrix/b2dhommatrix.cxx
index 4222c7a351c2..e4a9dda9e3c5 100644
--- a/basegfx/source/matrix/b2dhommatrix.cxx
+++ b/basegfx/source/matrix/b2dhommatrix.cxx
@@ -26,50 +26,56 @@
 
 namespace basegfx
 {
-
-    B2DHomMatrix::B2DHomMatrix(double f_0x0, double f_0x1, double f_0x2, double f_1x0, double f_1x1, double f_1x2)
-    {
-        maImpl.set(0, 0, f_0x0);
-        maImpl.set(0, 1, f_0x1);
-        maImpl.set(0, 2, f_0x2);
-        maImpl.set(1, 0, f_1x0);
-        maImpl.set(1, 1, f_1x1);
-        maImpl.set(1, 2, f_1x2);
-    }
-
-    void B2DHomMatrix::set(sal_uInt16 nRow, sal_uInt16 nColumn, double fValue)
-    {
-        maImpl.set(nRow, nColumn, fValue);
-    }
+    constexpr int RowSize = 3;
 
     void B2DHomMatrix::set3x2(double f_0x0, double f_0x1, double f_0x2, double f_1x0, double f_1x1, double f_1x2)
     {
-        maImpl.set(0, 0, f_0x0);
-        maImpl.set(0, 1, f_0x1);
-        maImpl.set(0, 2, f_0x2);
-        maImpl.set(1, 0, f_1x0);
-        maImpl.set(1, 1, f_1x1);
-        maImpl.set(1, 2, f_1x2);
-    }
-
-    bool B2DHomMatrix::isLastLineDefault() const
-    {
-        return maImpl.isLastLineDefault();
+        mfValues[0][0] = f_0x0;
+        mfValues[0][1] = f_0x1;
+        mfValues[0][2] = f_0x2;
+        mfValues[1][0] = f_1x0;
+        mfValues[1][1] = f_1x1;
+        mfValues[1][2] = f_1x2;
     }
 
     bool B2DHomMatrix::isIdentity() const
     {
-        return maImpl.isIdentity();
+        for(sal_uInt16 a(0); a < RowSize - 1; a++)
+        {
+            for(sal_uInt16 b(0); b < RowSize; b++)
+            {
+                const double fDefault(internal::implGetDefaultValue(a, b));
+                const double fValueAB(get(a, b));
+
+                if(!::basegfx::fTools::equal(fDefault, fValueAB))
+                {
+                    return false;
+                }
+            }
+        }
+
+        return true;
     }
 
     void B2DHomMatrix::identity()
     {
-        maImpl = Impl2DHomMatrix();
+        for(sal_uInt16 a(0); a < RowSize - 1; a++)
+        {
+            for(sal_uInt16 b(0); b < RowSize; b++)
+                mfValues[a][b] = internal::implGetDefaultValue(a, b);
+        }
     }
 
     bool B2DHomMatrix::isInvertible() const
     {
-        return maImpl.isInvertible();
+        double dst[6];
+        /* Compute adjoint: */
+        computeAdjoint(dst);
+        /* Compute determinant: */
+        double det = computeDeterminant(dst);
+        if (fTools::equalZero(det))
+            return false;
+        return true;
     }
 
     bool B2DHomMatrix::invert()
@@ -77,85 +83,43 @@ namespace basegfx
         if(isIdentity())
             return true;
 
-
-        double dst[9];
+        double dst[6];
 
         /* Compute adjoint: */
-
-        dst[0] = + get(1, 1) * get(2, 2) - get(1, 2) * get(2, 1);
-        dst[1] = - get(0, 1) * get(2, 2) + get(0, 2) * get(2, 1);
-        dst[2] = + get(0, 1) * get(1, 2) - get(0, 2) * get(1, 1);
-        dst[3] = - get(1, 0) * get(2, 2) + get(1, 2) * get(2, 0);
-        dst[4] = + get(0, 0) * get(2, 2) - get(0, 2) * get(2, 0);
-        dst[5] = - get(0, 0) * get(1, 2) + get(0, 2) * get(1, 0);
-        dst[6] = + get(1, 0) * get(2, 1) - get(1, 1) * get(2, 0);
-        dst[7] = - get(0, 0) * get(2, 1) + get(0, 1) * get(2, 0);
-        dst[8] = + get(0, 0) * get(1, 1) - get(0, 1) * get(1, 0);
+        computeAdjoint(dst);
 
         /* Compute determinant: */
-
-        double det = get(0, 0) * dst[0] + get(0, 1) * dst[3] + get(0, 2) * dst[6];
+        double det = computeDeterminant(dst);
         if (fTools::equalZero(det))
             return false;
 
         /* Multiply adjoint with reciprocal of determinant: */
-
         det = 1.0 / det;
-
-        maImpl.set(0, 0, dst[0] * det);
-        maImpl.set(0, 1, dst[1] * det);
-        maImpl.set(0, 2, dst[2] * det);
-        maImpl.set(1, 0, dst[3] * det);
-        maImpl.set(1, 1, dst[4] * det);
-        maImpl.set(1, 2, dst[5] * det);
-        maImpl.set(2, 0, dst[6] * det);
-        maImpl.set(2, 1, dst[7] * det);
-        maImpl.set(2, 2, dst[8] * det);
-
-        // The above algorithm is very slightly less accurate then the old one, so
-        // we need to round the last row to make sure that functions like decompose
-        // still do the same thing with existing data, otherwise testTdf109143
-        // in CppunitTest_vcl_pdfexport will fail.
-        if (fTools::equalZero(maImpl.get(2, 0)))
-            maImpl.set(2, 0, 0);
-        if (fTools::equalZero(maImpl.get(2, 1)))
-            maImpl.set(2, 1, 0);
-        if (fTools::equal(1.0, maImpl.get(2, 2)))
-            maImpl.set(2, 2, 1);
+        mfValues[0][0] = dst[0] * det;
+        mfValues[0][1] = dst[1] * det;
+        mfValues[0][2] = dst[2] * det;
+        mfValues[1][0] = dst[3] * det;
+        mfValues[1][1] = dst[4] * det;
+        mfValues[1][2] = dst[5] * det;
 
         return true;
     }
 
-    B2DHomMatrix& B2DHomMatrix::operator+=(const B2DHomMatrix& rMat)
+    /* Compute adjoint, optimised for the case where the last (not stored) row is { 0, 0, 1 } */
+    void B2DHomMatrix::computeAdjoint(double (&dst)[6]) const
     {
-        maImpl.doAddMatrix(rMat.maImpl);
-        return *this;
-    }
-
-    B2DHomMatrix& B2DHomMatrix::operator-=(const B2DHomMatrix& rMat)
-    {
-        maImpl.doSubMatrix(rMat.maImpl);
-        return *this;
+        dst[0] = + get(1, 1);
+        dst[1] = - get(0, 1);
+        dst[2] = + get(0, 1) * get(1, 2) - get(0, 2) * get(1, 1);
+        dst[3] = - get(1, 0);
+        dst[4] = + get(0, 0);
+        dst[5] = - get(0, 0) * get(1, 2) + get(0, 2) * get(1, 0);
     }
 
-    B2DHomMatrix& B2DHomMatrix::operator*=(double fValue)
+    /* Compute the determinant, given the adjoint matrix */
+    double B2DHomMatrix::computeDeterminant(double (&dst)[6]) const
     {
-        const double fOne(1.0);
-
-        if(!fTools::equal(fOne, fValue))
-            maImpl.doMulMatrix(fValue);
-
-        return *this;
-    }
-
-    B2DHomMatrix& B2DHomMatrix::operator/=(double fValue)
-    {
-        const double fOne(1.0);
-
-        if(!fTools::equal(fOne, fValue))
-            maImpl.doMulMatrix(1.0 / fValue);
-
-        return *this;
+        return mfValues[0][0] * dst[0] + mfValues[0][1] * dst[3];
     }
 
     B2DHomMatrix& B2DHomMatrix::operator*=(const B2DHomMatrix& rMat)
@@ -172,15 +136,50 @@ namespace basegfx
         else
         {
             // multiply
-            maImpl.doMulMatrix(rMat.maImpl);
+            doMulMatrix(rMat);
         }
 
         return *this;
     }
 
+    void B2DHomMatrix::doMulMatrix(const B2DHomMatrix& rMat)
+    {
+        // create a copy as source for the original values
+        const B2DHomMatrix aCopy(*this);
+
+        for(sal_uInt16 a(0); a < 2; ++a)
+        {
+            for(sal_uInt16 b(0); b < 3; ++b)
+            {
+                double fValue = 0.0;
+
+                for(sal_uInt16 c(0); c < 2; ++c)
+                    fValue += aCopy.mfValues[c][b] * rMat.mfValues[a][c];
+
+                mfValues[a][b] = fValue;
+            }
+            mfValues[a][2] += rMat.mfValues[a][2];
+        }
+    }
+
     bool B2DHomMatrix::operator==(const B2DHomMatrix& rMat) const
     {
-        return &rMat == this || maImpl.isEqual(rMat.maImpl);
+        if (&rMat == this)
+            return true;
+        for(sal_uInt16 a(0); a < 2; a++)
+        {
+            for(sal_uInt16 b(0); b < 3; b++)
+            {
+                const double fValueA(mfValues[a][b]);
+                const double fValueB(rMat.mfValues[a][b]);
+
+                if(!::basegfx::fTools::equal(fValueA, fValueB))
+                {
+                    return false;
+                }
+            }
+        }
+        return true;
     }
 
     bool B2DHomMatrix::operator!=(const B2DHomMatrix& rMat) const
@@ -197,26 +196,26 @@ namespace basegfx
         double fCos(1.0);
 
         utils::createSinCosOrthogonal(fSin, fCos, fRadiant);
-        Impl2DHomMatrix aRotMat;
+        B2DHomMatrix aRotMat;
 
         aRotMat.set(0, 0, fCos);
         aRotMat.set(1, 1, fCos);
         aRotMat.set(1, 0, fSin);
         aRotMat.set(0, 1, -fSin);
 
-        maImpl.doMulMatrix(aRotMat);
+        doMulMatrix(aRotMat);
     }
 
     void B2DHomMatrix::translate(double fX, double fY)
     {
         if(!fTools::equalZero(fX) || !fTools::equalZero(fY))
         {
-            Impl2DHomMatrix aTransMat;
+            B2DHomMatrix aTransMat;
 
             aTransMat.set(0, 2, fX);
             aTransMat.set(1, 2, fY);
 
-            maImpl.doMulMatrix(aTransMat);
+            doMulMatrix(aTransMat);
         }
     }
 
@@ -231,12 +230,12 @@ namespace basegfx
 
         if(!fTools::equal(fOne, fX) || !fTools::equal(fOne, fY))
         {
-            Impl2DHomMatrix aScaleMat;
+            B2DHomMatrix aScaleMat;
 
             aScaleMat.set(0, 0, fX);
             aScaleMat.set(1, 1, fY);
 
-            maImpl.doMulMatrix(aScaleMat);
+            doMulMatrix(aScaleMat);
         }
     }
 
@@ -250,11 +249,11 @@ namespace basegfx
         // #i76239# do not test against 1.0, but against 0.0. We are talking about a value not on the diagonal (!)
         if(!fTools::equalZero(fSx))
         {
-            Impl2DHomMatrix aShearXMat;
+            B2DHomMatrix aShearXMat;
 
             aShearXMat.set(0, 1, fSx);
 
-            maImpl.doMulMatrix(aShearXMat);
+            doMulMatrix(aShearXMat);
         }
     }
 
@@ -263,11 +262,11 @@ namespace basegfx
         // #i76239# do not test against 1.0, but against 0.0. We are talking about a value not on the diagonal (!)
         if(!fTools::equalZero(fSy))
         {
-            Impl2DHomMatrix aShearYMat;
+            B2DHomMatrix aShearYMat;
 
             aShearYMat.set(1, 0, fSy);
 
-            maImpl.doMulMatrix(aShearYMat);
+            doMulMatrix(aShearYMat);
         }
     }
 
@@ -280,12 +279,6 @@ namespace basegfx
     */
     bool B2DHomMatrix::decompose(B2DTuple& rScale, B2DTuple& rTranslate, double& rRotate, double& rShearX) const
     {
-        // when perspective is used, decompose is not made here
-        if(!maImpl.isLastLineDefault())
-        {
-            return false;
-        }
-
         // reset rotate and shear and copy translation values in every case
         rRotate = rShearX = 0.0;
         rTranslate.setX(get(0, 2));
diff --git a/basegfx/source/point/b2dpoint.cxx b/basegfx/source/point/b2dpoint.cxx
index 02e6711ac44c..1d7f75158526 100644
--- a/basegfx/source/point/b2dpoint.cxx
+++ b/basegfx/source/point/b2dpoint.cxx
@@ -34,21 +34,6 @@ namespace basegfx
             rMat.get(1, 1) * mfY +
             rMat.get(1, 2));
 
-        if(!rMat.isLastLineDefault())
-        {
-            const double fOne(1.0);
-            const double fTempM(
-                rMat.get(2, 0) * mfX +
-                rMat.get(2, 1) * mfY +
-                rMat.get(2, 2));
-
-            if(!fTools::equalZero(fTempM) && !fTools::equal(fOne, fTempM))
-            {
-                fTempX /= fTempM;
-                fTempY /= fTempM;
-            }
-        }
-
         mfX = fTempX;
         mfY = fTempY;
 
diff --git a/basegfx/source/point/b2ipoint.cxx b/basegfx/source/point/b2ipoint.cxx
index 7b43ebc860e2..2790fe3a18a8 100644
--- a/basegfx/source/point/b2ipoint.cxx
+++ b/basegfx/source/point/b2ipoint.cxx
@@ -41,21 +41,6 @@ namespace basegfx
             rMat.get(1, 1) * mnY +
             rMat.get(1, 2));
 
-        if(!rMat.isLastLineDefault())
-        {
-            const double fOne(1.0);
-            const double fTempM(
-                rMat.get(2, 0) * mnX +
-                rMat.get(2, 1) * mnY +
-                rMat.get(2, 2));
-
-            if(!fTools::equalZero(fTempM) && !fTools::equal(fOne, fTempM))
-            {
-                fTempX /= fTempM;
-                fTempY /= fTempM;
-            }
-        }
-
         mnX = fround(fTempX);
         mnY = fround(fTempY);
 
diff --git a/basegfx/test/B2DHomMatrixTest.cxx b/basegfx/test/B2DHomMatrixTest.cxx
index bd6b1f8c8029..aace816d3e3a 100644
--- a/basegfx/test/B2DHomMatrixTest.cxx
+++ b/basegfx/test/B2DHomMatrixTest.cxx
@@ -79,9 +79,6 @@ public:
         maPerspective.set(1, 0, 4.0);
         maPerspective.set(1, 1, 5.0);
         maPerspective.set(1, 2, 6.0);
-        maPerspective.set(2, 0, 7.0);
-        maPerspective.set(2, 1, 8.0);
-        maPerspective.set(2, 2, 9.0);
     }
 
     void equal()
@@ -129,9 +126,6 @@ public:
         aPerspective.set(1, 0, 4.0);
         aPerspective.set(1, 1, 5.0);
         aPerspective.set(1, 2, 6.0);
-        aPerspective.set(2, 0, 7.0);
-        aPerspective.set(2, 1, 8.0);
-        aPerspective.set(2, 2, 9.0);
 
         CPPUNIT_ASSERT_MESSAGE("operator==: identity matrix", aIdentity.operator==(maIdentity));
         CPPUNIT_ASSERT_MESSAGE("operator==: scale matrix", aScale.operator==(maScale));
@@ -239,59 +233,11 @@ public:
         affineAffineProd.set(1, 1, 33);
         affineAffineProd.set(1, 2, 48);
 
-        B2DHomMatrix affinePerspectiveProd;
-
-        affinePerspectiveProd.set(0, 0, 30);
-        affinePerspectiveProd.set(0, 1, 36);
-        affinePerspectiveProd.set(0, 2, 42);
-        affinePerspectiveProd.set(1, 0, 66);
-        affinePerspectiveProd.set(1, 1, 81);
-        affinePerspectiveProd.set(1, 2, 96);
-        affinePerspectiveProd.set(2, 0, 7);
-        affinePerspectiveProd.set(2, 1, 8);
-        affinePerspectiveProd.set(2, 2, 9);
-
-        B2DHomMatrix perspectiveAffineProd;
-
-        perspectiveAffineProd.set(0, 0, 9);
-        perspectiveAffineProd.set(0, 1, 12);
-        perspectiveAffineProd.set(0, 2, 18);
-        perspectiveAffineProd.set(1, 0, 24);
-        perspectiveAffineProd.set(1, 1, 33);
-        perspectiveAffineProd.set(1, 2, 48);
-        perspectiveAffineProd.set(2, 0, 39);
-        perspectiveAffineProd.set(2, 1, 54);
-        perspectiveAffineProd.set(2, 2, 78);
-
-        B2DHomMatrix perspectivePerspectiveProd;
-
-        perspectivePerspectiveProd.set(0, 0, 30);
-        perspectivePerspectiveProd.set(0, 1, 36);
-        perspectivePerspectiveProd.set(0, 2, 42);
-        perspectivePerspectiveProd.set(1, 0, 66);
-        perspectivePerspectiveProd.set(1, 1, 81);
-        perspectivePerspectiveProd.set(1, 2, 96);
-        perspectivePerspectiveProd.set(2, 0, 102);
-        perspectivePerspectiveProd.set(2, 1, 126);
-        perspectivePerspectiveProd.set(2, 2, 150);
-
         B2DHomMatrix temp;
 
         temp = maAffine;
         temp *= maAffine;
         CPPUNIT_ASSERT_EQUAL_MESSAGE("multiply: both compact", affineAffineProd, temp);
-
-        temp = maPerspective;
-        temp *= maAffine;
-        CPPUNIT_ASSERT_EQUAL_MESSAGE("multiply: first compact", affinePerspectiveProd, temp);
-
-        temp = maAffine;
-        temp *= maPerspective;
-        CPPUNIT_ASSERT_EQUAL_MESSAGE("multiply: second compact", perspectiveAffineProd, temp);
-
-        temp = maPerspective;
-        temp *= maPerspective;
-        CPPUNIT_ASSERT_EQUAL_MESSAGE("multiply: none compact", perspectivePerspectiveProd, temp);
     }
 
     void impFillMatrix(B2DHomMatrix& rSource, double fScaleX, double fScaleY, double fShearX,