PartDesign: Revolution - add FuseOrder

In older versions of FreeCAD the boolean order was base + result. After
TNP mitigation the order was changed to be result + base. For the
resulting shape that does not matter, but order of edges can differ if
arguments are in a different order.

This can impact refine algorithm which may pick other face as the base
one and result in a differnt shape after refining. This commit restores
previous order. For most files it should not make any difference, but it
may fix some older files.

To support all cases we introduce FuseOrder compatibility property that
will be set to FeatureFirst for files saved with 1.0 to preserve
behavior.

src/Mod/PartDesign/App/FeatureRevolution.cpp

@@ -36,6 +36,8 @@
 #include "FeatureRevolution.h"
 #include "Mod/Part/App/TopoShapeOpCode.h"
 
+#include <Base/ProgramVersion.h>
+
 using namespace PartDesign;
 
 namespace PartDesign
@@ -46,6 +48,8 @@ namespace PartDesign
 const char* Revolution::TypeEnums[]
     = {"Angle", "UpToLast", "UpToFirst", "UpToFace", "TwoAngles", nullptr};
 
+const char* Revolution::FuseOrderEnums[] = {"BaseFirst", "FeatureFirst", nullptr};
+
 PROPERTY_SOURCE(PartDesign::Revolution, PartDesign::ProfileBased)
 
 const App::PropertyAngle::Constraints Revolution::floatAngle = {0.0, 360.0, 1.0};
@@ -82,12 +86,21 @@ Revolution::Revolution()
         (App::Prop_None),
         "Reference axis of revolution"
     );
+
+    ADD_PROPERTY_TYPE(
+        FuseOrder,
+        (BaseFirst),
+        "Compatibility",
+        App::Prop_Hidden,
+        "Order of fuse operation to preserve compatibility with filest created using FreeCAD 1.0"
+    );
+    FuseOrder.setEnums(FuseOrderEnums);
 }
 
 short Revolution::mustExecute() const
 {
     if (Placement.isTouched() || ReferenceAxis.isTouched() || Axis.isTouched() || Base.isTouched()
-        || UpToFace.isTouched() || Angle.isTouched() || Angle2.isTouched()) {
+        || UpToFace.isTouched() || Angle.isTouched() || Angle2.isTouched() || FuseOrder.isTouched()) {
         return 1;
     }
     return ProfileBased::mustExecute();
@@ -254,7 +267,16 @@ App::DocumentObjectExecReturn* Revolution::execute()
             this->AddSubShape.setValue(result);
 
             if (!base.isNull()) {
-                result = result.makeElementFuse(base);
+                // In 1.0 there was a bug that caused the order of operations to be reversed.
+                // Changing the order may impact geometry order and the results of refine operation,
+                // hence we need to support both ways to ensure compatibility.
+                if (FuseOrder.getValue() == FeatureFirst) {
+                    result = result.makeElementFuse(base);
+                }
+                else {
+                    result = base.makeElementFuse(result);
+                }
+
                 // store shape before refinement
                 this->rawShape = result;
                 result = refineShapeIfActive(result);
@@ -297,6 +319,16 @@ App::DocumentObjectExecReturn* Revolution::execute()
     }
 }
 
+void Revolution::Restore(Base::XMLReader& reader)
+{
+    ProfileBased::Restore(reader);
+
+    // For 1.0 and 1.0 only the order was feature first due to a bug
+    if (Base::getVersion(reader.ProgramVersion) == Base::Version::v1_0) {
+        FuseOrder.setValue(FeatureFirst);
+    }
+}
+
 bool Revolution::suggestReversed()
 {
     try {

src/Mod/PartDesign/App/FeatureRevolution.h

@@ -30,6 +30,12 @@
 namespace PartDesign
 {
 
+enum FuseOrder : std::uint8_t
+{
+    BaseFirst,
+    FeatureFirst,
+};
+
 class PartDesignExport Revolution: public ProfileBased
 {
     PROPERTY_HEADER_WITH_OVERRIDE(PartDesign::Revolution);
@@ -48,6 +54,12 @@ public:
      */
     App::PropertyLinkSub ReferenceAxis;
 
+    /**
+     * Compatibility property that is required to preserve behavior from 1.0, that while incorrect
+     * may have an impact over user files.
+     */
+    App::PropertyEnumeration FuseOrder;
+
     /** @name methods override feature */
     //@{
     /** Recalculate the feature
@@ -66,6 +78,8 @@ public:
     }
     //@}
 
+    void Restore(Base::XMLReader& reader) override;
+
     /// suggests a value for Reversed flag so that material is always added to the support
     bool suggestReversed();
 
@@ -132,6 +146,7 @@ protected:
 
 private:
     static const char* TypeEnums[];
+    static const char* FuseOrderEnums[];
 };
 
 }  // namespace PartDesign

src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py

@@ -569,7 +569,7 @@ class TestTopologicalNamingProblem(unittest.TestCase):
         #  Pad -> Extrusion -> makes compounds and does booleans, thus the resulting newName element maps
         #  See if we can turn those off, or try them on the other types?
 
-    def testPartDesignElementMapRevolution(self):
+    def _testPartDesignElementMapRevolution(self, order, vertex, face):
         # App.KeepTestDoc = True    # Uncomment this if you want to keep the test document to examine
         self.Doc.UseHasher = False
         # Arrange
@@ -596,6 +596,7 @@ class TestTopologicalNamingProblem(unittest.TestCase):
         revolution.Profile = sketch2
         revolution.Angle = 180
         revolution.Refine = True
+        revolution.FuseOrder = order
         body.addObject(revolution)
         volume = (math.pi * 3 * 3 - math.pi * 2 * 2) * 2 / 2
         padVolume = 3 * 3 * 2  # 50.26548245743668
@@ -611,8 +612,8 @@ class TestTopologicalNamingProblem(unittest.TestCase):
         self.assertEqual(
             self.countFacesEdgesVertexes(revolution.Shape.ElementReverseMap), (9, 21, 14)
         )
-        self.assertEqual(revolution.Shape.ElementReverseMap["Vertex9"][1].count(";"), 3)
-        self.assertEqual(revolution.Shape.ElementReverseMap["Face9"].count(";"), 19)
+        self.assertEqual(revolution.Shape.ElementReverseMap[vertex][1].count(";"), 3)
+        self.assertEqual(revolution.Shape.ElementReverseMap[face].count(";"), 19)
 
         ### This test has been removed because FeatureRevolution generates improper element maps when the user select the
         #   UpToFace mode. That behavior seems to be the fault of OpenCASCADE itself, and we need to rewrite that section
@@ -642,6 +643,12 @@ class TestTopologicalNamingProblem(unittest.TestCase):
         # self.assertEqual( revolution.Shape.ElementReverseMap["Face8"].count("Face8"), 3)
         # self.assertEqual( revolution.Shape.ElementReverseMap["Face8"].count("Face10"), 3)
 
+    def testPartDesignElementMapRevolutionFuseFeatureFirst(self):
+        self._testPartDesignElementMapRevolution("FeatureFirst", "Vertex9", "Face9")
+
+    def testPartDesignElementMapRevolutionWithDefaultFuseOrder(self):
+        self._testPartDesignElementMapRevolution("BaseFirst", "Vertex8", "Face8")
+
     def testPartDesignBinderRevolution(self):
         doc = self.Doc
         body = doc.addObject("PartDesign::Body", "Body")