Fix legacy mmTangentPlane orientation in 1.1.0dev (resolves #24254) (#24529)

* Add legacy TangentPlane orientation compatibility layer

* Refactor: extract lambda for object identifier construction

* Refactor: axis calculation using std::ranges::max_element and std::distance

* Apply suggestions from code review

Co-authored-by: Pieter Hijma <pieterhijma@users.noreply.github.com>

* Part: Add regression test for issue 24254

---------

Co-authored-by: Pieter Hijma <pieterhijma@users.noreply.github.com>
Co-authored-by: Chris Hennes <chennes@gmail.com>

src/Mod/Part/App/AttachExtension.cpp

@@ -26,6 +26,12 @@
 #include <Base/Console.h>
 #include <Base/Tools.h>
 
+#include <App/Document.h>
+#include <App/Datums.h>
+#include <App/ObjectIdentifier.h>
+#include <App/Expression.h>
+#include <App/ExpressionParser.h>
+
 #include "AttachExtension.h"
 #include "AttachExtensionPy.h"
 
@@ -479,6 +485,162 @@ bool AttachExtension::extensionHandleChangedPropertyName(Base::XMLReader& reader
     return App::DocumentObjectExtension::extensionHandleChangedPropertyName(reader, TypeName, PropName);
 }
 
+void AttachExtension::handleLegacyTangentPlaneOrientation()
+{
+    // check attachment mode
+    if (_props.attacher->mapMode != mmTangentPlane) {
+        return;
+    }
+
+    // check stored document program version (applies to v1.0 and earlier only)
+    int major, minor;
+    if (sscanf(getExtendedObject()->getDocument()->getProgramVersion(), "%d.%d", &major, &minor) != 2) {
+        return;
+    }
+    if (major > 1 || (major == 1 && minor > 0)) {
+        return;
+    }
+
+    // check for an App::Plane support object exists
+    App::GeoFeature* geof = nullptr;
+    for (auto obj : this->AttachmentSupport.linkedObjects()) {
+        if (obj->isDerivedFrom<App::Plane>()) {
+            geof = freecad_cast<App::GeoFeature*>(obj);
+            break;
+        }
+    }
+    if (!geof) {
+        return;
+    }
+
+    /*
+     * determine which global axis (X/Y/Z) is closest to the plane normal
+     */
+
+    // extract the plane normal
+    Base::Vector3d norm;
+    geof->Placement.getValue().getRotation().multVec(Base::Vector3d(0.0, 0.0, 1.0), norm);
+    gp_Dir normal(norm.x, norm.y, norm.z);
+
+    // determine the dominant global axis
+    std::array<double, 3> cosXYZ = {
+        fabs(normal.Dot(gp_Dir(1, 0, 0))),  // normal dot global X axis
+        fabs(normal.Dot(gp_Dir(0, 1, 0))),  // normal dot global Y axis
+        fabs(normal.Dot(gp_Dir(0, 0, 1)))   // normal dot global Z axis
+    };
+    std::size_t axis = std::distance(cosXYZ.begin(), std::ranges::max_element(cosXYZ));
+
+    // if Z is dominant, no changes needed
+    if (axis == 2) {
+        return;
+    }
+
+    /*
+     * correct attachment offset position and rotation based on dominant axis
+     */
+
+    App::DocumentObject* owner = getExtendedObject();
+    Base::Console().message("Converting attachment offset of %s\n",
+                            owner->getNameInDocument());
+
+    try {
+        // extract current attachment offset values
+        Base::Placement placement = this->AttachmentOffset.getValue();
+        Base::Vector3d position = placement.getPosition();
+        Base::Rotation rotation = placement.getRotation();
+        double yaw, pitch, roll;
+        rotation.getYawPitchRoll(yaw, pitch, roll);
+
+        // extract existing expressions
+        auto buildOID  = [owner](const std::vector<std::string>& path) {
+            App::ObjectIdentifier oid(owner, true);
+            for (const auto& name : path) {
+                oid.addComponent(App::ObjectIdentifier::SimpleComponent(name));
+            }
+            return oid;
+        };
+        App::ObjectIdentifier oidX   = buildOID ({"AttachmentOffset", "Base", "x"});
+        App::ObjectIdentifier oidY   = buildOID ({"AttachmentOffset", "Base", "y"});
+        App::ObjectIdentifier oidYaw = buildOID ({"AttachmentOffset", "Rotation", "Yaw"});
+        const App::Expression* exprX = nullptr;
+        const App::Expression* exprY = nullptr;
+        const App::Expression* exprYaw = nullptr;
+        for (const auto& [oid, expr] : owner->ExpressionEngine.getExpressions()) {
+            if (oid == oidX) {
+                exprX = expr;
+            } else if (oid == oidY) {
+                exprY = expr;
+            } else if (oid == oidYaw) {
+                exprYaw = expr;
+            }
+        }
+
+        // convert placement and expressions according to the dominant axis
+        App::Expression* newExprX = nullptr;
+        App::Expression* newExprY = nullptr;
+        App::Expression* newExprYaw = nullptr;
+        if (axis == 0) { // normal mostly X
+            // values
+            std::swap(position.x, position.y);
+            position.x = -position.x;
+            rotation.setYawPitchRoll(yaw + 90, pitch, roll);
+
+            // expressions
+            if (exprX) {
+                newExprY = App::ExpressionParser::parse(owner, exprX->toString().c_str());
+            }
+            if (exprY) {
+                std::string expr = "-1 * (" + exprY->toString() + ")";
+                newExprX = App::ExpressionParser::parse(owner, expr.c_str());
+            }
+            if (exprYaw) {
+                std::string expr = "(" + exprYaw->toString() + ") + 90 deg";
+                newExprYaw = App::ExpressionParser::parse(owner, expr.c_str());
+            }
+        }
+        else if (axis == 1) { // normal mostly Y
+            // values
+            std::swap(position.x, position.y);
+            position.y = -position.y;
+            rotation.setYawPitchRoll(yaw - 90, pitch, roll);
+
+            // expressions
+            if (exprX) {
+                std::string expr = "-1 * (" + exprX->toString() + ")";
+                newExprY = App::ExpressionParser::parse(owner, expr.c_str());
+            }
+            if (exprY) {
+                newExprX = App::ExpressionParser::parse(owner, exprY->toString().c_str());
+            }
+            if (exprYaw) {
+                std::string expr = "(" + exprYaw->toString() + ") - 90 deg";
+                newExprYaw = App::ExpressionParser::parse(owner, expr.c_str());
+            }
+        }
+        else {
+            // should not happen
+            return;
+        }
+
+        // store updated placement and expressions back to the document object
+
+        // expressions
+        owner->ExpressionEngine.setValue(oidX, App::ExpressionPtr(newExprX));
+        owner->ExpressionEngine.setValue(oidY, App::ExpressionPtr(newExprY));
+        owner->ExpressionEngine.setValue(oidYaw, App::ExpressionPtr(newExprYaw));
+
+        // values
+        placement.setPosition(position);
+        placement.setRotation(rotation);
+        this->AttachmentOffset.setValue(placement);
+
+    } catch (const Base::Exception& e) {
+        Base::Console().error("Error converting legacy attachment offset of %s: %s\n",
+                              owner->getNameInDocument(),
+                              e.what());
+    }
+}
+
 void AttachExtension::onExtendedDocumentRestored()
 {
     try {
@@ -494,6 +656,10 @@ void AttachExtension::onExtendedDocumentRestored()
 
         restoreAttacherEngine(this);
 
+        if (_props.attacher->mapMode == mmTangentPlane) {
+            handleLegacyTangentPlaneOrientation();
+        }
+
         bool bAttached = positionBySupport();
 
         updateSinglePropertyStatus(bAttached);

src/Mod/Part/App/AttachExtension.h

@@ -147,6 +147,8 @@ protected:
     App::PropertyPlacement& getPlacement() const;
     void initBase(bool force);
 
+    void handleLegacyTangentPlaneOrientation();
+
 public:
     void updateAttacherVals(bool base = false) const;
     // This update both _props and _baseProps if base = false

src/Mod/Part/CMakeLists.txt

@@ -78,6 +78,7 @@ set(Part_tests
     parttests/ColorPerFaceTest.py
     parttests/ColorTransparencyTest.py
     parttests/TopoShapeTest.py
+    parttests/TestTangentMode3-0.21.FCStd
 )
 
 add_custom_target(PartScripts ALL SOURCES

src/Mod/Part/parttests/regression_tests.py

@@ -3,6 +3,10 @@
 import FreeCAD
 from FreeCAD import Vector, Base, newDocument, closeDocument
 import Part
+
+import math
+import os
+
 if "BUILD_SKETCHER" in FreeCAD.__cmake__:
     import Sketcher
 
@@ -123,6 +127,65 @@ class RegressionTests(unittest.TestCase):
         cube.recompute()
         assert cube.Placement.Base.isEqual(v1,1e-6)
 
+    def test_TangentMode3_Issue24254(self):
+        """In FreeCAD 1.1 we changed the behavior of the mmTangentPlane, but added code to handle old files
+        that relied upon the original tangent plane behavior. This test ensures that old files open with the expected
+        tangent plane setup."""
+
+        location = os.path.dirname(os.path.realpath(__file__))
+        FreeCAD.openDocument(os.path.join(location,"TestTangentMode3-0.21.FCStd"), True)
+
+        def check_plane(name, expected_pos, expected_normal, expected_xaxis):
+            obj = FreeCAD.ActiveDocument.getObject(name)
+            if not obj:
+                FreeCAD.Console.PrintError(f"{name}: object not found\n")
+                return
+
+            pos = obj.Placement.Base
+            rot = obj.Placement.Rotation
+            normal = rot.multVec(FreeCAD.Vector(0,0,1))
+            xaxis  = rot.multVec(FreeCAD.Vector(1,0,0))
+
+            # position
+            self.assertAlmostEqual((pos - expected_pos).Length, 0)
+
+            # normal
+            self.assertAlmostEqual(normal.getAngle(expected_normal), 0)
+
+            # tangent x-axis
+            self.assertAlmostEqual(xaxis.getAngle(expected_xaxis), 0)
+
+        r = 10.0
+        rad30 = math.radians(30)
+        cos30 = math.cos(rad30)
+        sin30 = math.sin(rad30)
+
+        expected_planes = [
+            ("Sketch001",
+             FreeCAD.Vector(-1 * r, 0, 0), # position
+             FreeCAD.Vector(1, 0, 0),      # normal
+             FreeCAD.Vector(0, -1, 0)),    # tangent x-axis
+            ("Sketch002",
+             FreeCAD.Vector(sin30 * r,
+                            cos30 * cos30 * (-r),
+                            cos30 * sin30 * r),
+             FreeCAD.Vector(sin30,
+                            cos30 * cos30 * (-1),
+                            cos30 * sin30),
+             FreeCAD.Vector(0, sin30, cos30)),
+            ("Sketch003",
+             FreeCAD.Vector(cos30 * cos30 * r,
+                            sin30 * r,
+                            cos30 * sin30 * r),
+             FreeCAD.Vector(cos30 * cos30,
+                            sin30,
+                            cos30 * sin30),
+             FreeCAD.Vector(sin30, 0, -cos30)),
+        ]
+
+        for name, pos, normal, xaxis in expected_planes:
+            check_plane(name, pos, normal, xaxis)
+
     def tearDown(self):
         """Clean up our test, optionally preserving the test document"""
         # This flag allows doing something like this: