diff --git a/src/Mod/PartDesign/App/FeatureRevolution.cpp b/src/Mod/PartDesign/App/FeatureRevolution.cpp index 2df41a8d47..9d7dc2a92d 100644 --- a/src/Mod/PartDesign/App/FeatureRevolution.cpp +++ b/src/Mod/PartDesign/App/FeatureRevolution.cpp @@ -36,6 +36,8 @@ #include "FeatureRevolution.h" #include "Mod/Part/App/TopoShapeOpCode.h" +#include + 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 { diff --git a/src/Mod/PartDesign/App/FeatureRevolution.h b/src/Mod/PartDesign/App/FeatureRevolution.h index a1bbf7ce99..ceee1f3615 100644 --- a/src/Mod/PartDesign/App/FeatureRevolution.h +++ b/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 diff --git a/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py b/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py index 2a91412723..1f76f47124 100644 --- a/src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py +++ b/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")