From a5e561b31dd11e0c2e6b886ae677a5c679f8f889 Mon Sep 17 00:00:00 2001 From: wmayer Date: Tue, 15 Feb 2022 21:05:11 +0100 Subject: [PATCH] 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 | 9 ++++ src/Base/Matrix.cpp | 74 ++++++++++++++++----------- src/Base/Matrix.h | 14 ++++- src/Base/MatrixPy.xml | 6 ++- src/Base/MatrixPyImp.cpp | 5 +- src/Mod/Draft/draftutils/gui_utils.py | 2 +- src/Mod/Part/App/TopoShape.cpp | 3 +- src/Mod/Test/BaseTests.py | 37 ++++++++++++-- 8 files changed, 110 insertions(+), 40 deletions(-) diff --git a/src/App/FreeCADInit.py b/src/App/FreeCADInit.py index b56e18c56a..4d96cd093d 100644 --- a/src/App/FreeCADInit.py +++ b/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) diff --git a/src/Base/Matrix.cpp b/src/Base/Matrix.cpp index 0b2fa86191..297502a5de 100644 --- a/src/Base/Matrix.cpp +++ b/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(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; } diff --git a/src/Base/Matrix.h b/src/Base/Matrix.h index 97abecb3b7..e56c0f752b 100644 --- a/src/Base/Matrix.h +++ b/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 diff --git a/src/Base/MatrixPy.xml b/src/Base/MatrixPy.xml index d6f4cd52cc..ea6d965794 100644 --- a/src/Base/MatrixPy.xml +++ b/src/Base/MatrixPy.xml @@ -37,8 +37,10 @@ Scale the matrix with the vector -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 diff --git a/src/Base/MatrixPyImp.cpp b/src/Base/MatrixPyImp.cpp index 69003eddd2..ecdc5eea71 100644 --- a/src/Base/MatrixPyImp.cpp +++ b/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(type))))); } PyObject* MatrixPy::nullify(PyObject * args) diff --git a/src/Mod/Draft/draftutils/gui_utils.py b/src/Mod/Draft/draftutils/gui_utils.py index 0799ddc7cd..681d762d8c 100644 --- a/src/Mod/Draft/draftutils/gui_utils.py +++ b/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") diff --git a/src/Mod/Part/App/TopoShape.cpp b/src/Mod/Part/App/TopoShape.cpp index 171c3831cd..4884b00277 100644 --- a/src/Mod/Part/App/TopoShape.cpp +++ b/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; } diff --git a/src/Mod/Test/BaseTests.py b/src/Mod/Test/BaseTests.py index 78dbd1e46b..c7c0a0b460 100644 --- a/src/Mod/Test/BaseTests.py +++ b/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())