Base: fix Matrix4D::hasScale

* If all column vectors of the 3x3 sub-matrix are equal the function incorrectly claims that it's uniform scaling.
* Detect also non-uniform scaling and if was applied from the left or right side
* Replace the int with an enum and expose it to Python
* Add several new unit tests

src/App/FreeCADInit.py

@@ -888,6 +888,15 @@ class Scheme(IntEnum):
 
 App.Units.Scheme = Scheme
 
+class ScaleType(IntEnum):
+    Other = -1
+    NoScaling = 0
+    NonUniformRight = 1
+    NonUniformLeft = 2
+    Uniform = 3
+
+App.ScaleType = ScaleType
+
 # clean up namespace
 del(InitApplications)
 del(test_ascii)

src/Base/Matrix.cpp

@@ -155,6 +155,18 @@ double Matrix4D::determinant() const
     return fDet;
 }
 
+double Matrix4D::determinant3() const
+{
+    double a = dMtrx4D[0][0] * dMtrx4D[1][1] * dMtrx4D[2][2];
+    double b = dMtrx4D[0][1] * dMtrx4D[1][2] * dMtrx4D[2][0];
+    double c = dMtrx4D[1][0] * dMtrx4D[2][1] * dMtrx4D[0][2];
+    double d = dMtrx4D[0][2] * dMtrx4D[1][1] * dMtrx4D[2][0];
+    double e = dMtrx4D[1][0] * dMtrx4D[0][1] * dMtrx4D[2][2];
+    double f = dMtrx4D[0][0] * dMtrx4D[2][1] * dMtrx4D[1][2];
+    double det = (a + b + c) - (d + e + f);
+    return det;
+}
+
 void Matrix4D::move (const Vector3f& rclVct)
 {
     move(convertTo<Vector3d>(rclVct));
@@ -821,42 +833,46 @@ Matrix4D& Matrix4D::Hat(const Vector3d& rV)
     return *this;
 }
 
-int Matrix4D::hasScale(double tol) const
+ScaleType Matrix4D::hasScale(double tol) const
 {
     // check for uniform scaling
     //
-    // scaling factors are the column vector length. We use square distance and
-    // ignore the actual scaling signess
-    //
-    // Note: In general using the column vectors to get the scaling factors makes
-    // sense if a scaling matrix was multiplied from the left side. If a scaling
-    // matrix was multiplied from the right side then the row vectors must be used.
-    // However, since this function checks for _uniform_ scaling it doesn't make a
-    // difference if row or column vectors are used.
-
-    // TODO:
-    // The int should be replaced with an enum class that tells the calling
-    // instance whether:
-    // * a uniform scaling was applied
-    // * a non-uniform scaling from the right side was applied
-    // * a non-uniform scaling from the left side was applied
-    // * no scaling at all
-
+    // For a scaled rotation matrix it matters whether
+    // the scaling was applied from the left or right side.
+    // Only in case of uniform scaling it doesn't make a difference.
     if (tol == 0.0)
         tol = 1e-9;
+
+    // get column vectors
     double dx = Vector3d(dMtrx4D[0][0],dMtrx4D[1][0],dMtrx4D[2][0]).Sqr();
     double dy = Vector3d(dMtrx4D[0][1],dMtrx4D[1][1],dMtrx4D[2][1]).Sqr();
-    if (fabs(dx-dy) > tol) {
-        return -1;
-    }
-    else {
-        double dz = Vector3d(dMtrx4D[0][2],dMtrx4D[1][2],dMtrx4D[2][2]).Sqr();
-        if (fabs(dy-dz) > tol)
-            return -1;
+    double dz = Vector3d(dMtrx4D[0][2],dMtrx4D[1][2],dMtrx4D[2][2]).Sqr();
+    double dxyz = sqrt(dx * dy * dz);
+
+    // get row vectors
+    double du = Vector3d(dMtrx4D[0][0],dMtrx4D[0][1],dMtrx4D[0][2]).Sqr();
+    double dv = Vector3d(dMtrx4D[1][0],dMtrx4D[1][1],dMtrx4D[1][2]).Sqr();
+    double dw = Vector3d(dMtrx4D[2][0],dMtrx4D[2][1],dMtrx4D[2][2]).Sqr();
+    double duvw = sqrt(du * dv * dw);
+
+    double d3 = determinant3();
+
+    // This could be e.g. a projection, a shearing,... matrix
+    if (fabs(dxyz - d3) > tol && fabs(duvw - d3) > tol) {
+        return ScaleType::Other;
     }
 
-    if (fabs(dx-1.0) > tol)
-        return 1;
-    else
-        return 0;
+    if (fabs(duvw - d3) <= tol && (fabs(du - dv) > tol || fabs(dv - dw) > tol)) {
+        return ScaleType::NonUniformLeft;
+    }
+
+    if (fabs(dxyz - d3) <= tol && (fabs(dx - dy) > tol || fabs(dy - dz) > tol)) {
+        return ScaleType::NonUniformRight;
+    }
+
+    if (fabs(dx - 1.0) > tol) {
+        return ScaleType::Uniform;
+    }
+
+    return ScaleType::NoScaling;
 }

src/Base/Matrix.h

@@ -37,6 +37,14 @@
 
 namespace Base {
 
+enum class ScaleType {
+    Other = -1,
+    NoScaling = 0,
+    NonUniformRight = 1,
+    NonUniformLeft = 2,
+    Uniform = 3
+};
+
 /**
  * The Matrix4D class.
  */
@@ -112,6 +120,8 @@ public:
   inline void setTrace(const Vector3d&);
   /// Compute the determinant of the matrix
   double determinant() const;
+  /// Compute the determinant of the 3x3 sub-matrix
+  double determinant3() const;
   /// Analyse the transformation
   std::string analyse() const;
   /// Outer product (Dyadic product)
@@ -162,8 +172,8 @@ public:
   { scale(Vector3f(scalexyz, scalexyz, scalexyz)); }
   void scale        (double scalexyz)
   { scale(Vector3d(scalexyz, scalexyz, scalexyz)); }
-  /// Check for scaling factor, 0: not scale, 1: uniform scale, or else -1
-  int hasScale(double tol=0.0) const;
+  /// Check for scaling factor
+  ScaleType hasScale(double tol=0.0) const;
   /// Rotate around the X axis (in transformed space) for the given value in radians
   void rotX         (double fAngle);
   /// Rotate around the Y axis (in transformed space) for the given value in radians

src/Base/MatrixPy.xml

@@ -37,8 +37,10 @@ Scale the matrix with the vector
     <Methode Name="hasScale" Const="true">
       <Documentation>
         <UserDocu>
-hasScale(tol=None)
-Return 0 is no scale factor, 1 for uniform scaling, -1 for non-uniform scaling.
+hasScale(tol=0.0)
+Return an enum value of ScaleType. Possible values are:
+Uniform, NonUniformLeft, NonUniformRight, NoScaling or Other
+if it's not a scale matrix
         </UserDocu>
       </Documentation>
     </Methode>

src/Base/MatrixPyImp.cpp

@@ -351,7 +351,10 @@ PyObject* MatrixPy::hasScale(PyObject * args)
     double tol=0;
     if (!PyArg_ParseTuple(args, "|d", &tol))
         return nullptr;
-    return Py::new_reference_to(Py::Int(getMatrixPtr()->hasScale(tol)));
+
+    ScaleType type = getMatrixPtr()->hasScale(tol);
+    Py::Module mod("FreeCAD");
+    return Py::new_reference_to(mod.callMemberFunction("ScaleType", Py::TupleN(Py::Int(static_cast<int>(type)))));
 }
 
 PyObject* MatrixPy::nullify(PyObject * args)

src/Mod/Draft/draftutils/gui_utils.py

@@ -154,7 +154,7 @@ def autogroup(obj):
                 # do not autogroup if obj points to active_part to prevent cyclic references
                 return
             matrix = parent.getSubObject(sub, retType=4)
-            if matrix.hasScale() == 1:
+            if matrix.hasScale() == App.ScaleType.Uniform:
                 err = translate("draft",
                                 "Unable to insert new object into "
                                 "a scaled part")

src/Mod/Part/App/TopoShape.cpp

@@ -4297,7 +4297,8 @@ bool TopoShape::_makETransform(const TopoShape &shape,
         const Base::Matrix4D &rclTrf, const char *op, bool checkScale, bool copy)
 {
     if(checkScale) {
-        if(rclTrf.hasScale()<0) {
+        auto type = rclTrf.hasScale();
+        if (type != Base::ScaleType::Uniform && type != Base::ScaleType::NoScaling) {
             makEGTransform(shape,rclTrf,op,copy);
             return true;
         }

src/Mod/Test/BaseTests.py

@@ -422,13 +422,42 @@ class MatrixTestCase(unittest.TestCase):
     def testScale(self):
         self.mat.scale(1., 2., 3.)
         self.assertEqual(self.mat.determinant(), 6.0)
-        self.assertEqual(self.mat.hasScale(), -1)
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.NonUniformLeft)
         self.mat.unity()
-        self.assertEqual(self.mat.hasScale(), 0)
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.NoScaling)
         self.mat.scale(2., 2., 2.)
-        self.assertEqual(self.mat.hasScale(), 1)
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.Uniform)
         self.mat.rotateX(1.0)
-        self.assertEqual(self.mat.hasScale(), 1)
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.Uniform)
+        self.mat.scale(1., 2., 3.)
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.NonUniformLeft)
+        self.mat.unity()
+        self.mat.scale(1., 2., 3.)
+        self.mat.rotateX(1.0)
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.NonUniformRight)
+        self.mat.unity()
+        self.mat.setCol(0, FreeCAD.Vector(1,2,3))
+        self.mat.setCol(1, FreeCAD.Vector(1,2,3))
+        self.mat.setCol(2, FreeCAD.Vector(1,2,3))
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.Other)
+        self.mat.unity()
+        self.mat.setRow(0, FreeCAD.Vector(1,2,3))
+        self.mat.setRow(1, FreeCAD.Vector(1,2,3))
+        self.mat.setRow(2, FreeCAD.Vector(1,2,3))
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.Other)
+
+    def testShearing(self):
+        self.mat.setRow(1, FreeCAD.Vector(0,1,1))
+        self.assertEqual(self.mat.hasScale(), FreeCAD.ScaleType.Other)
+
+    def testMultLeftOrRight(self):
+        mat1 = FreeCAD.Matrix()
+        mat1.rotateX(1.0)
+
+        mat2 = FreeCAD.Matrix()
+        mat2.scale(1., 2., 3.)
+        self.assertEqual((mat1 * mat2).hasScale(), FreeCAD.ScaleType.NonUniformRight)
+        self.assertEqual((mat2 * mat1).hasScale(), FreeCAD.ScaleType.NonUniformLeft)
 
     def testNull(self):
         self.assertFalse(self.mat.isNull())