Correct mistaken Helix patch; use MakePipe for frenet for speed; add tests (#11598)

* Cleanup and switch to using MakePipe

* Cleanup unused include

* Use Frenet mode in building pipe

* Restore original code, add a control property

* Hide property, and init on new objects

* Restore prior behavior for legacy objects

* Clean up git blame; add missing move to keep helix same with new makePipe call

* Remove debug include

* Use doxygen syntax for deprecation comment

* Add unit tests; restore code for angled helixes; optimize zero angle helixes

* Lower test precision to accept platform variations

* Loosen more

* Correct restoration

* Tweak test volumes to match restored helix creation

* Restore helix generation parameter

* Fix test; delint

.git-blame-ignore-revs

@@ -98,3 +98,5 @@ db24eeec535f1f43fb3d5b63d24c5171af637880 # RE: Final application of pre-commit
 b8f8b232cb0882d171cb299e6f6279a516cdd6eb # Inspection: Final application of pre-commit
 c5c2ea3498f402c0c89916c46ddb071e22756622 # Assembly: Final application of pre-commit
 592c992b863549fde52741fd8830418168387695 # Assembly: Apply pre-commit to Assembly files
+94ca51b7799b2ac60a2a56d7e6e753bb97a73671 # PartDesign: Fix #9377 - issue with accuracy in AdditiveHelix (#11312)
+d472927bba7b2d8d151c99fb29cf1d8dd099ea7d # Correct PartDesign Helix feature negative angles (#11399)

src/Mod/PartDesign/App/FeatureHelix.cpp

@@ -31,6 +31,7 @@
 # include <BRepBuilderAPI_MakeSolid.hxx>
 # include <BRepBuilderAPI_Sewing.hxx>
 # include <BRepClass3d_SolidClassifier.hxx>
+# include <BRepOffsetAPI_MakePipe.hxx>
 # include <BRepOffsetAPI_MakePipeShell.hxx>
 # include <BRepPrimAPI_MakeRevol.hxx>
 # include <Precision.hxx>
@@ -214,10 +215,6 @@ App::DocumentObjectExecReturn* Helix::execute()
 
         base.Move(invObjLoc);
 
-        // generate the helix path
-        TopoDS_Shape path = generateHelixPath();
-        TopoDS_Shape auxpath = generateHelixPath(1.0);
-
 
         std::vector<TopoDS_Wire> wires;
         try {
@@ -226,44 +223,84 @@ App::DocumentObjectExecReturn* Helix::execute()
         catch (const Base::Exception& e) {
             return new App::DocumentObjectExecReturn(e.what());
         }
+        TopoDS_Shape result;
 
-        //build all shells
+        // generate the helix path
-        BRepOffsetAPI_MakePipeShell mkPS(TopoDS::Wire(path));
+        TopoDS_Shape path = generateHelixPath();
+        TopoDS_Shape auxpath = generateHelixPath(1.0);
 
-        mkPS.SetTolerance(Precision::Confusion());
+        // Use MakePipe for frenet ( Angle is 0 ) calculations, faster than MakePipeShell
-        mkPS.SetTransitionMode(BRepBuilderAPI_Transformed);
+        if ( Angle.getValue() == 0 ) {
-
+            TopoDS_Shape face = Part::FaceMakerCheese::makeFace(wires);
-        mkPS.SetMode(TopoDS::Wire(auxpath), true);  // this is for auxiliary
+            face.Move(invObjLoc);
-
+            BRepOffsetAPI_MakePipe mkPS(TopoDS::Wire(path), face, GeomFill_Trihedron::GeomFill_IsFrenet, Standard_False);
-        if (Angle.getValue() == 0) {
-            mkPS.SetMode(true);  // This is for frenet, quicker than auxiliary
-                                 // but can introduce to much error, checked below
-        } else {
-            mkPS.SetMode(TopoDS::Wire(auxpath), true);  // this is for auxiliary
-        }
-
-        for (TopoDS_Wire& wire : wires) {
-            wire.Move(invObjLoc);
-            mkPS.Add(wire);
-        }
-
-        if (!mkPS.IsReady())
-            return new App::DocumentObjectExecReturn(QT_TRANSLATE_NOOP("Exception", "Error: Could not build"));
-
-        mkPS.Build();
-
-        //check for error value at pipe creation and re-build if needed
-        if (Angle.getValue() == 0 && mkPS.ErrorOnSurface() < Precision::Confusion() / 2.0 ) {
-            Base::Console().Log("PartDesign_Helix : Fall back to auxiliary mode\n");
-            mkPS.SetMode(TopoDS::Wire(auxpath), true);
             mkPS.Build();
+            result = mkPS.Shape();
+        } else {
+            std::vector<std::vector<TopoDS_Wire>> wiresections;
+            for (TopoDS_Wire& wire : wires)
+                wiresections.emplace_back(1, wire);
+
+            //build all shells
+            std::vector<TopoDS_Shape> shells;
+            std::vector<TopoDS_Wire> frontwires, backwires;
+            for (std::vector<TopoDS_Wire>& wires : wiresections) {
+
+                BRepOffsetAPI_MakePipeShell mkPS(TopoDS::Wire(path));
+
+                // Frenet mode doesn't place the face quite right on an angled helix, so
+                // use the auxiliary spine to force that.
+                mkPS.SetMode(TopoDS::Wire(auxpath), true);  // this is for auxiliary
+
+                for (TopoDS_Wire& wire : wires) {
+                    wire.Move(invObjLoc);
+                    mkPS.Add(wire);
+                }
+
+                if (!mkPS.IsReady())
+                    return new App::DocumentObjectExecReturn(QT_TRANSLATE_NOOP("Exception", "Error: Could not build"));
+                mkPS.Build();
+
+                shells.push_back(mkPS.Shape());
+
+                if (!mkPS.Shape().Closed()) {
+                    // // shell is not closed - use simulate to get the end wires
+                    TopTools_ListOfShape sim;
+                    mkPS.Simulate(2, sim);
+
+                    frontwires.push_back(TopoDS::Wire(sim.First()));
+                    backwires.push_back(TopoDS::Wire(sim.Last()));
+                }
+                BRepBuilderAPI_MakeSolid mkSolid;
+
+                if (!frontwires.empty()) {
+                    // build the end faces, sew the shell and build the final solid
+                    TopoDS_Shape front = Part::FaceMakerCheese::makeFace(frontwires);
+                    TopoDS_Shape back = Part::FaceMakerCheese::makeFace(backwires);
+
+                    BRepBuilderAPI_Sewing sewer;
+                    sewer.SetTolerance(Precision::Confusion());
+                    sewer.Add(front);
+                    sewer.Add(back);
+
+                    for (TopoDS_Shape& s : shells)
+                        sewer.Add(s);
+                    sewer.Perform();
+                    mkSolid.Add(TopoDS::Shell(sewer.SewedShape()));
+                }
+                else {
+                    // shells are already closed - add them directly
+                    for (TopoDS_Shape& s : shells) {
+                        mkSolid.Add(TopoDS::Shell(s));
+                    }
+                }
+                if (!mkSolid.IsDone())
+                    return new App::DocumentObjectExecReturn(QT_TRANSLATE_NOOP("Exception", "Error: Result is not a solid"));
+
+                result = mkSolid.Shape();
+            }
         }
 
-        if (!mkPS.MakeSolid())
-            return new App::DocumentObjectExecReturn(QT_TRANSLATE_NOOP("Exception", "Error: Could not make solid helix with open wire"));
-
-        TopoDS_Shape result = mkPS.Shape();
-
         BRepClass3d_SolidClassifier SC(result);
         SC.PerformInfinitePoint(Precision::Confusion());
         if (SC.State() == TopAbs_IN)
@@ -425,7 +462,7 @@ TopoDS_Shape Helix::generateHelixPath(double startOffset0)
 
     //build the helix path
     //TopoShape helix = TopoShape().makeLongHelix(pitch, height, radius, angle, leftHanded);
-    TopoDS_Shape path = TopoShape().makeSpiralHelix(radius, radiusTop, height, turns, 0, leftHanded);
+    TopoDS_Shape path = TopoShape().makeSpiralHelix(radius, radiusTop, height, turns, 1, leftHanded);
 
     /*
      * The helix wire is created with the axis coinciding with z-axis and the start point at (radius, 0, 0)

src/Mod/PartDesign/CMakeLists.txt

@@ -53,6 +53,7 @@ set(PartDesign_TestScripts
     PartDesignTests/TestDraft.py
     PartDesignTests/TestThickness.py
     PartDesignTests/TestInvoluteGear.py
+    PartDesignTests/TestHelix.py
 )
 
 set(PartDesign_TestFixtures

src/Mod/PartDesign/PartDesignTests/TestHelix.py

@@ -0,0 +1,143 @@
+#***************************************************************************
+#*   Copyright (c) 2023 <bgbsww@gmail.com>                                 *
+#*                                                                         *
+#*   This program is free software; you can redistribute it and/or modify  *
+#*   it under the terms of the GNU Lesser General Public License (LGPL)    *
+#*   as published by the Free Software Foundation; either version 2 of     *
+#*   the License, or (at your option) any later version.                   *
+#*   for detail see the LICENCE text file.                                 *
+#*                                                                         *
+#*   This program is distributed in the hope that it will be useful,       *
+#*   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+#*   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+#*   GNU Library General Public License for more details.                  *
+#*                                                                         *
+#*   You should have received a copy of the GNU Library General Public     *
+#*   License along with this program; if not, write to the Free Software   *
+#*   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  *
+#*   USA                                                                   *
+#*                                                                         *
+#***************************************************************************
+
+from math import pi
+import unittest
+
+import FreeCAD
+import Part
+import Sketcher
+import TestSketcherApp
+
+""" Test various helixes """
+class TestHelix(unittest.TestCase):
+    """ Test various helixes """
+    def setUp(self):
+        self.Doc = FreeCAD.newDocument("PartDesignTestHelix")
+
+    def testCircleQ1(self):
+        """ Test helix based on circle in Quadrant 1 """
+        body = self.Doc.addObject('PartDesign::Body','Body')
+        profileSketch = self.Doc.addObject('Sketcher::SketchObject', 'ProfileSketch')
+        body.addObject(profileSketch)
+        TestSketcherApp.CreateCircleSketch(profileSketch, (2, 0), 1)
+        self.Doc.recompute()
+        helix = self.Doc.addObject("PartDesign::AdditiveHelix","AdditiveHelix")
+        body.addObject(helix)
+        helix.Profile = profileSketch
+        helix.ReferenceAxis = (profileSketch,"V_Axis")
+        helix.Placement = FreeCAD.Placement(FreeCAD.Vector(0,0,0), 
+                                            FreeCAD.Rotation(FreeCAD.Vector(0,0,1),0),
+                                            FreeCAD.Vector(0,0,0))
+        helix.Pitch = 3
+        helix.Height = 9
+        helix.Turns = 2
+        helix.Angle = 0
+        helix.Mode = 1
+        self.Doc.recompute()
+        self.assertAlmostEqual(helix.Shape.Volume, 78.95687956849457,places=5)
+
+        helix.Angle = 25
+        self.Doc.recompute()
+        self.assertAlmostEqual(helix.Shape.Volume, 134.17450779511307,places=5)
+
+        profileSketch.addGeometry(Part.Circle(FreeCAD.Vector(2, 0, 0), FreeCAD.Vector(0,0,1), 0.5) )
+        self.Doc.recompute()
+        self.assertAlmostEqual(helix.Shape.Volume, 100.63088079046352,places=5)
+
+
+    def testRectangle(self):
+        """ Test helix based on a rectangle """
+        body = self.Doc.addObject('PartDesign::Body','GearBody')
+        gearSketch = self.Doc.addObject('Sketcher::SketchObject', 'GearSketch')
+        body.addObject(gearSketch)
+        TestSketcherApp.CreateRectangleSketch(gearSketch, (0, 0), (5, 5))
+        self.Doc.recompute()
+
+        # xz_plane = body.Origin.OriginFeatures[4]
+        # coneSketch.Support = xz_plane
+        # coneSketch.MapMode = 'FlatFace'
+        helix = self.Doc.addObject("PartDesign::AdditiveHelix","AdditiveHelix")
+        body.addObject(helix)
+        helix.Profile = gearSketch
+        helix.ReferenceAxis = (gearSketch,"V_Axis")
+        helix.Placement = FreeCAD.Placement(FreeCAD.Vector(0,0,0), FreeCAD.Rotation(FreeCAD.Vector(0,0,1),0), FreeCAD.Vector(0,0,0))
+
+        helix.Pitch = 50
+        helix.Height = 150
+        helix.Turns = 3
+        helix.Angle = 0
+        helix.Mode = 0 
+        self.Doc.recompute()
+        bbox = helix.Shape.BoundBox
+        self.assertAlmostEqual(bbox.YMin,0)
+        self.assertAlmostEqual(helix.Shape.Volume, 1178.0961742825648,places=5)
+
+
+    def testCone(self):
+        """ Test helix following a cone """
+        body = self.Doc.addObject('PartDesign::Body','ConeBody')
+        coneSketch = self.Doc.addObject('Sketcher::SketchObject', 'ConeSketch')
+        body.addObject(coneSketch)
+
+        geoList = []
+        geoList.append(Part.LineSegment(FreeCAD.Vector(-5, -5, 0), FreeCAD.Vector(-3, 0, 0)) )
+        geoList.append(Part.LineSegment(FreeCAD.Vector(-3, 0, 0), FreeCAD.Vector(-2, 0, 0)) )
+        geoList.append(Part.LineSegment(FreeCAD.Vector(-2, 0, 0), FreeCAD.Vector(-4, -5, 0)) )
+        geoList.append(Part.LineSegment(FreeCAD.Vector(-4, -5, 0), FreeCAD.Vector(-5, -5, 0)))
+        (l1, l2, l3, l4) = coneSketch.addGeometry(geoList)
+
+        conList = []
+        conList.append(Sketcher.Constraint("Coincident", 0, 2, 1, 1))
+        conList.append(Sketcher.Constraint("Coincident", 1, 2, 2, 1))
+        conList.append(Sketcher.Constraint("Coincident", 2, 2, 3, 1))
+        conList.append(Sketcher.Constraint("Coincident", 3, 2, 0, 1))
+        conList.append(Sketcher.Constraint("Horizontal", 1))
+        conList.append(Sketcher.Constraint("Angle", l3, 1, -2, 2, FreeCAD.Units.Quantity("30.000000 deg")))
+        conList.append(Sketcher.Constraint("DistanceX", 1, 2, -5))
+        conList.append(Sketcher.Constraint("DistanceY", 1, 2, 0))
+        conList.append(Sketcher.Constraint("Equal", 0, 2))
+        conList.append(Sketcher.Constraint("Equal", 1, 3))
+        conList.append(Sketcher.Constraint("DistanceY", 0, 50))
+        conList.append(Sketcher.Constraint("DistanceX", 1, 10))
+        coneSketch.addConstraint(conList)
+
+        xz_plane = body.Origin.OriginFeatures[4]
+        coneSketch.Support = xz_plane
+        coneSketch.MapMode = 'FlatFace'
+        helix = self.Doc.addObject("PartDesign::AdditiveHelix","AdditiveHelix")
+        body.addObject(helix)
+        helix.Profile = coneSketch
+        helix.ReferenceAxis = (coneSketch,"V_Axis")
+        helix.Placement = FreeCAD.Placement(FreeCAD.Vector(0,0,0), FreeCAD.Rotation(FreeCAD.Vector(0,0,1),0), FreeCAD.Vector(0,0,0))
+
+        helix.Pitch = 50
+        helix.Height = 110
+        helix.Turns = 2.2
+        helix.Angle = 30
+        helix.Mode = 0 
+        helix.Reversed = True
+        self.Doc.recompute()
+        self.assertAlmostEqual(helix.Shape.Volume, 388285.4117047924,places=5)
+
+    def tearDown(self):
+        FreeCAD.closeDocument("PartDesignTestHelix")
+

src/Mod/PartDesign/PartDesignTests/__init__.py

@@ -3,6 +3,7 @@ from . import TestChamfer
 from . import TestDatum
 from . import TestDraft
 from . import TestFillet
+from . import TestHelix
 from . import TestHole
 from . import TestInvoluteGear
 from . import TestLinearPattern

src/Mod/PartDesign/TestPartDesignApp.py

@@ -37,6 +37,7 @@ from PartDesignTests.TestRevolve import TestRevolve
 from PartDesignTests.TestPipe import TestPipe
 from PartDesignTests.TestLoft import TestLoft
 from PartDesignTests.TestPrimitive import TestPrimitive
+from PartDesignTests.TestHelix import TestHelix
 
 # transformations and boolean
 from PartDesignTests.TestMirrored import TestMirrored