Clean code and add tests

src/Mod/Part/App/TopoShape.h

@@ -37,11 +37,17 @@
 #include <BRepBuilderAPI_Sewing.hxx>
 #include <BRepOffsetAPI_ThruSections.hxx>
 #include <BRepOffsetAPI_MakePipeShell.hxx>
-#include <BRepPrimAPI_MakeHalfSpace.hxx>
 #include <BRepFeat_MakePrism.hxx>
 #include <BRepPrimAPI_MakeHalfSpace.hxx>
 
+// FIXME?
+// including <BRepPrimAPI_MakeHalfSpace.hxx> instead of the incomplete class declaration
+// below results in a broken link on Windows but builds on other platforms.  I suspect
+// that's why these class declarations appear in the RT branch.
+// Something about how that compiler is mangling the names?  Maybe we're missing a
+// magic windows specific qualifier on the declarations.
 
+class BRepPrimAPI_MakeHalfSpace;
 class gp_Ax1;
 class gp_Ax2;
 class gp_Pln;
@@ -56,6 +62,7 @@ namespace Part
 {
 
 class TopoShapeCache;
+class TopoShape;
 
 /* A special sub-class to indicate null shapes
  */

src/Mod/Part/App/TopoShapeExpansion.cpp

@@ -39,6 +39,7 @@
 #include "TopoShapeCache.h"
 #include "FaceMaker.h"
 
+#include "TopoShapeOpCode.h"
 
 FC_LOG_LEVEL_INIT("TopoShape", true, true)  // NOLINT
 
@@ -551,8 +552,6 @@ TopoShape::makeElementCompound(const std::vector<TopoShape>& shapes, const char*
     return *this;
 }
 
-<<<<<<< HEAD
-=======
 struct MapperSewing: Part::TopoShape::Mapper
 {
     BRepBuilderAPI_Sewing& maker;
@@ -690,7 +689,6 @@ TopoShape& TopoShape::makeElementShape(BRepPrimAPI_MakeHalfSpace& mkShape,
     return makeShapeWithElementMap(mkShape.Solid(), MapperMaker(mkShape), {source}, op);
 }
 
->>>>>>> ad521d6a23 (Missing method)
 TopoShape& TopoShape::makeElementFace(const TopoShape& shape,
                                       const char* op,
                                       const char* maker,
@@ -950,117 +948,104 @@ TopoShape& TopoShape::makeElementShape(BRepBuilderAPI_MakeShape& mkShape,
     return makeShapeWithElementMap(mkShape.Shape(), MapperMaker(mkShape), shapes, op);
 }
 
-TopoShape &TopoShape::makeElementShape(BRepOffsetAPI_ThruSections &mk, const TopoShape &source,
-        const char *op)
+TopoShape&
+TopoShape::makeElementShape(BRepOffsetAPI_ThruSections& mk, const TopoShape& source, const char* op)
 {
-    if(!op) op = Part::OpCodes::ThruSections;
-    return makeElementShape(mk,std::vector<TopoShape>(1,source),op);
+    if (!op) {
+        op = Part::OpCodes::ThruSections;
+    }
+    return makeElementShape(mk, std::vector<TopoShape>(1, source), op);
 }
 
-TopoShape &TopoShape::makeElementShape(BRepOffsetAPI_ThruSections &mk, const std::vector<TopoShape> &sources,
-        const char *op)
+TopoShape& TopoShape::makeElementShape(BRepOffsetAPI_ThruSections& mk,
+                                       const std::vector<TopoShape>& sources,
+                                       const char* op)
 {
-    if(!op) op = Part::OpCodes::ThruSections;
-    return makeShapeWithElementMap(mk.Shape(),MapperThruSections(mk,sources),sources,op);
+    if (!op) {
+        op = Part::OpCodes::ThruSections;
+    }
+    return makeShapeWithElementMap(mk.Shape(), MapperThruSections(mk, sources), sources, op);
 }
 
-TopoShape &TopoShape::makeElementShape(BRepBuilderAPI_Sewing &mk, const std::vector<TopoShape> &shapes,
-        const char *op)
+TopoShape& TopoShape::makeElementShape(BRepBuilderAPI_Sewing& mk,
+                                       const std::vector<TopoShape>& shapes,
+                                       const char* op)
 {
-    if(!op) op = Part::OpCodes::Sewing;
-    return makeShapeWithElementMap(mk.SewedShape(),MapperSewing(mk),shapes,op);
+    if (!op) {
+        op = Part::OpCodes::Sewing;
+    }
+    return makeShapeWithElementMap(mk.SewedShape(), MapperSewing(mk), shapes, op);
 }
 
-TopoShape &TopoShape::makeElementShape(BRepBuilderAPI_Sewing &mkShape,
-            const TopoShape &source, const char *op)
+TopoShape&
+TopoShape::makeElementShape(BRepBuilderAPI_Sewing& mkShape, const TopoShape& source, const char* op)
 {
-    if(!op) op = Part::OpCodes::Sewing;
-    return makeElementShape(mkShape,std::vector<TopoShape>(1,source),op);
+    if (!op) {
+        op = Part::OpCodes::Sewing;
+    }
+    return makeElementShape(mkShape, std::vector<TopoShape>(1, source), op);
 }
 
-struct MapperSewing: Part::TopoShape::Mapper {
-    BRepBuilderAPI_Sewing &maker;
-    MapperSewing(BRepBuilderAPI_Sewing &maker)
-        :maker(maker)
+struct MapperSewing: Part::TopoShape::Mapper
+{
+    BRepBuilderAPI_Sewing& maker;
+    MapperSewing(BRepBuilderAPI_Sewing& maker)
+        : maker(maker)
     {}
-    virtual const std::vector<TopoDS_Shape> &modified(const TopoDS_Shape &s) const override {
+    virtual const std::vector<TopoDS_Shape>& modified(const TopoDS_Shape& s) const override
+    {
         _res.clear();
         try {
-            const auto &shape = maker.Modified(s);
-            if(!shape.IsNull() && !shape.IsSame(s))
+            const auto& shape = maker.Modified(s);
+            if (!shape.IsNull() && !shape.IsSame(s)) {
                 _res.push_back(shape);
-            else {
-                const auto &sshape = maker.ModifiedSubShape(s);
-                if(!sshape.IsNull() && !sshape.IsSame(s))
-                    _res.push_back(sshape);
             }
-        } catch (const Standard_Failure & e) {
-            if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG))
+            else {
+                const auto& sshape = maker.ModifiedSubShape(s);
+                if (!sshape.IsNull() && !sshape.IsSame(s)) {
+                    _res.push_back(sshape);
+                }
+            }
+        }
+        catch (const Standard_Failure& e) {
+            if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) {
                 FC_WARN("Exception on shape mapper: " << e.GetMessageString());
+            }
         }
         return _res;
     }
 };
 
-struct MapperThruSections: MapperMaker {
-    TopoShape firstProfile;
-    TopoShape lastProfile;
-
-    MapperThruSections(BRepOffsetAPI_ThruSections &tmaker,
-            const std::vector<TopoShape> &profiles)
-        :MapperMaker(tmaker)
-    {
-        if(!tmaker.FirstShape().IsNull())
-            firstProfile = profiles.front();
-        if(!tmaker.LastShape().IsNull())
-            lastProfile = profiles.back();
-    }
-    virtual const std::vector<TopoDS_Shape> &generated(const TopoDS_Shape &s) const override {
-        MapperMaker::generated(s);
-        if(_res.size()) return _res;
-        try {
-            auto &tmaker = static_cast<BRepOffsetAPI_ThruSections&>(maker);
-            auto shape = tmaker.GeneratedFace(s);
-            if(!shape.IsNull())
-                _res.push_back(shape);
-            if(firstProfile.getShape().IsSame(s) || firstProfile.findShape(s))
-                _res.push_back(tmaker.FirstShape());
-            else if(lastProfile.getShape().IsSame(s) || lastProfile.findShape(s))
-                _res.push_back(tmaker.LastShape());
-        } catch (const Standard_Failure & e) {
-            if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG))
-                FC_WARN("Exception on shape mapper: " << e.GetMessageString());
-        }
-        return _res;
-    }
-};
-
-const std::vector<TopoDS_Shape> &
-MapperMaker::modified(const TopoDS_Shape &s) const
+const std::vector<TopoDS_Shape>& MapperMaker::modified(const TopoDS_Shape& s) const
 {
     _res.clear();
     try {
         TopTools_ListIteratorOfListOfShape it;
-        for (it.Initialize(maker.Modified(s)); it.More(); it.Next())
+        for (it.Initialize(maker.Modified(s)); it.More(); it.Next()) {
             _res.push_back(it.Value());
-    } catch (const Standard_Failure & e) {
-        if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG))
+        }
+    }
+    catch (const Standard_Failure& e) {
+        if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) {
             FC_WARN("Exception on shape mapper: " << e.GetMessageString());
+        }
     }
     return _res;
 }
 
-const std::vector<TopoDS_Shape> &
-MapperMaker::generated(const TopoDS_Shape &s) const
+const std::vector<TopoDS_Shape>& MapperMaker::generated(const TopoDS_Shape& s) const
 {
     _res.clear();
     try {
         TopTools_ListIteratorOfListOfShape it;
-        for (it.Initialize(maker.Generated(s)); it.More(); it.Next())
+        for (it.Initialize(maker.Generated(s)); it.More(); it.Next()) {
             _res.push_back(it.Value());
-    } catch (const Standard_Failure & e) {
-        if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG))
+        }
+    }
+    catch (const Standard_Failure& e) {
+        if (FC_LOG_INSTANCE.isEnabled(FC_LOGLEVEL_LOG)) {
             FC_WARN("Exception on shape mapper: " << e.GetMessageString());
+        }
     }
     return _res;
 }

tests/src/Mod/Part/App/CMakeLists.txt

@@ -16,4 +16,5 @@ target_sources(
             ${CMAKE_CURRENT_SOURCE_DIR}/TopoShapeCache.cpp
             ${CMAKE_CURRENT_SOURCE_DIR}/TopoShapeExpansion.cpp
             ${CMAKE_CURRENT_SOURCE_DIR}/TopoShapeMapper.cpp
+            ${CMAKE_CURRENT_SOURCE_DIR}/TopoShapeMakeShape.cpp
 )

tests/src/Mod/Part/App/TopoShapeMakeShape.cpp

@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+
+// Tests for the makeShape methods, extracted from the main set of tests for TopoShape
+// due to length and complexity.
+
+#include "gtest/gtest.h"
+#include "src/App/InitApplication.h"
+#include "PartTestHelpers.h"
+#include <Mod/Part/App/TopoShape.h>
+
+#include <BRepBuilderAPI_MakeVertex.hxx>
+
+using namespace Data;
+using namespace Part;
+using namespace PartTestHelpers;
+
+class TopoShapeMakeShapeTests: public ::testing::Test
+{
+protected:
+    static void SetUpTestSuite()
+    {
+        tests::initApplication();
+    }
+
+    void SetUp() override
+    {
+        _docName = App::GetApplication().getUniqueDocumentName("test");
+        App::GetApplication().newDocument(_docName.c_str(), "testUser");
+        _sids = &_sid;
+    }
+
+    void TearDown() override
+    {
+        App::GetApplication().closeDocument(_docName.c_str());
+    }
+
+    Part::TopoShape* Shape()
+    {
+        return &_shape;
+    }
+
+    Part::TopoShape::Mapper* Mapper()
+    {
+        return &_mapper;
+    }
+
+private:
+    std::string _docName;
+    Data::ElementIDRefs _sid;
+    QVector<App::StringIDRef>* _sids = nullptr;
+    Part::TopoShape _shape;
+    Part::TopoShape::Mapper _mapper;
+};
+
+TEST_F(TopoShapeMakeShapeTests, nullShapeThrows)
+{
+    // Arrange
+    auto [cube1, cube2] = CreateTwoCubes();
+    std::vector<Part::TopoShape> sources {cube1, cube2};
+    TopoDS_Vertex nullShape;
+
+    // Act and assert
+    EXPECT_THROW(Shape()->makeShapeWithElementMap(nullShape, *Mapper(), sources),
+                 Part::NullShapeException);
+}
+
+TEST_F(TopoShapeMakeShapeTests, shapeVertex)
+{
+    // Arrange
+    BRepBuilderAPI_MakeVertex vertexMaker = BRepBuilderAPI_MakeVertex(gp_Pnt(10, 10, 10));
+    TopoShape topoShape(vertexMaker.Vertex(), 1L);
+    // Act
+    TopoShape& result = topoShape.makeElementShape(vertexMaker, topoShape);
+    auto elements = elementMap(result);
+    // Assert
+    EXPECT_EQ(elements.size(), 1);
+    EXPECT_EQ(elements.count(IndexedName("Vertex", 1)), 1);
+    EXPECT_EQ(elements[IndexedName("Vertex", 1)], MappedName("Vertex1;MAK;:H:4,V"));
+    EXPECT_EQ(getArea(result.getShape()), 0);
+}
+
+TEST_F(TopoShapeMakeShapeTests, thruSections)
+{
+    // Arrange
+    auto [face1, wire1, edge1, edge2, edge3, edge4] = CreateRectFace();
+    TopoDS_Wire wire2 = wire1;
+    auto transform {gp_Trsf()};
+    transform.SetTranslation(gp_Pnt(0.0, 0.0, 0.0), gp_Pnt(0.0, 0.5, 1.0));
+    wire2.Move(TopLoc_Location(transform));
+    TopoShape wire1ts {wire1, 1L};
+    TopoShape wire2ts {wire2, 2L};
+    BRepOffsetAPI_ThruSections thruMaker;
+    thruMaker.AddWire(wire1);
+    thruMaker.AddWire(wire2);
+    TopoShape topoShape {};
+    // Act
+    TopoShape& result = topoShape.makeElementShape(thruMaker, {wire1ts, wire2ts});
+    auto elements = elementMap(result);
+    // Assert
+    EXPECT_EQ(elements.size(), 24);
+    EXPECT_EQ(elements.count(IndexedName("Vertex", 1)), 1);
+    EXPECT_EQ(elements[IndexedName("Vertex", 1)], MappedName("Vertex1;TRU;:H1:4,V"));
+    EXPECT_EQ(getVolume(result.getShape()), 4);
+}
+
+TEST_F(TopoShapeMakeShapeTests, sewing)
+{
+    // Arrange
+    auto [face1, wire1, edge1, edge2, edge3, edge4] = CreateRectFace();
+    auto face2 = face1;
+    auto transform {gp_Trsf()};
+    transform.SetTranslation(gp_Pnt(0.0, 0.0, 0.0), gp_Pnt(0.5, 0.5, 0.0));
+    face2.Move(TopLoc_Location(transform));
+    BRepBuilderAPI_Sewing sewer;
+    sewer.Add(face1);
+    sewer.Add(face2);
+    sewer.Perform();
+    std::vector<TopoShape> sources {{face1, 1L}, {face2, 2L}};
+    TopoShape topoShape {};
+    // Act
+    TopoShape& result = topoShape.makeElementShape(sewer, sources);
+    auto elements = elementMap(result);
+    // Assert
+    EXPECT_EQ(&result, &topoShape);
+    EXPECT_EQ(elements.size(), 18);  // Now a single cube
+    EXPECT_EQ(elements.count(IndexedName("Vertex", 1)), 1);
+    EXPECT_EQ(elements[IndexedName("Vertex", 1)], MappedName("Vertex1;SEW;:H1:4,V"));
+    EXPECT_EQ(getArea(result.getShape()), 12);
+}