diff --git a/src/ModelingAlgorithms/TKShHealing/GTests/FILES.cmake b/src/ModelingAlgorithms/TKShHealing/GTests/FILES.cmake index ad6abc7923..f4c1090cdc 100644 --- a/src/ModelingAlgorithms/TKShHealing/GTests/FILES.cmake +++ b/src/ModelingAlgorithms/TKShHealing/GTests/FILES.cmake @@ -4,6 +4,7 @@ set(OCCT_TKShHealing_GTests_FILES_LOCATION "${CMAKE_CURRENT_LIST_DIR}") set(OCCT_TKShHealing_GTests_FILES ShapeAnalysis_CanonicalRecognition_Test.cxx ShapeAnalysis_Edge_Test.cxx + ShapeBuild_ReShape_Test.cxx ShapeConstruct_ProjectCurveOnSurface_Test.cxx ShapeFix_Shape_Test.cxx ShapeUpgrade_FaceDivide_Test.cxx diff --git a/src/ModelingAlgorithms/TKShHealing/GTests/ShapeBuild_ReShape_Test.cxx b/src/ModelingAlgorithms/TKShHealing/GTests/ShapeBuild_ReShape_Test.cxx new file mode 100644 index 0000000000..a1761e4506 --- /dev/null +++ b/src/ModelingAlgorithms/TKShHealing/GTests/ShapeBuild_ReShape_Test.cxx @@ -0,0 +1,170 @@ +// Copyright (c) 2026 OPEN CASCADE SAS +// +// This file is part of Open CASCADE Technology software library. +// +// This library is free software; you can redistribute it and/or modify it under +// the terms of the GNU Lesser General Public License version 2.1 as published +// by the Free Software Foundation, with special exception defined in the file +// OCCT_LGPL_EXCEPTION.txt. Consult the file LICENSE_LGPL_21.txt included in OCCT +// distribution for complete text of the license and disclaimer of any warranty. +// +// Alternatively, this file may be used under the terms of Open CASCADE +// commercial license or contractual agreement. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace +{ +int CountSubShapes(const TopoDS_Shape& theShape, const TopAbs_ShapeEnum theType) +{ + NCollection_IndexedMap aMap; + TopExp::MapShapes(theShape, theType, aMap); + return aMap.Extent(); +} +} // namespace + +// Basic Apply() via ShapeBuild_ReShape (the subclass used throughout shape +// healing) must produce the expected vertex substitution on a compound. +TEST(ShapeBuild_ReShapeTest, Apply_PerformsVertexReplacementOnCompound) +{ + const TopoDS_Vertex aV1 = BRepBuilderAPI_MakeVertex(gp_Pnt(0, 0, 0)); + const TopoDS_Vertex aV2 = BRepBuilderAPI_MakeVertex(gp_Pnt(1, 0, 0)); + const TopoDS_Vertex aV2Repl = BRepBuilderAPI_MakeVertex(gp_Pnt(1.5, 0, 0)); + + TopoDS_Compound aParent; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aParent); + aBuilder.Add(aParent, aV1); + aBuilder.Add(aParent, aV2); + + occ::handle aReShape = new ShapeBuild_ReShape; + aReShape->Replace(aV2, aV2Repl); + + const TopoDS_Shape aResult = aReShape->Apply(aParent); + ASSERT_FALSE(aResult.IsNull()); + + bool aFoundReplacement = false; + for (TopoDS_Iterator anIt(aResult); anIt.More(); anIt.Next()) + { + if (anIt.Value().TShape() == aV2Repl.TShape()) + aFoundReplacement = true; + EXPECT_FALSE(anIt.Value().TShape() == aV2.TShape()); + } + EXPECT_TRUE(aFoundReplacement); +} + +// Structural containment through ShapeBuild_ReShape's Apply override. +// The base class has its own DFS guard, the subclass has its own applyImpl; +// both paths must survive cyclic containment without a stack overflow. +TEST(ShapeBuild_ReShapeTest, Apply_StructuralContainmentWithoutCrash) +{ + const TopoDS_Vertex aV1 = BRepBuilderAPI_MakeVertex(gp_Pnt(0, 0, 0)); + const TopoDS_Vertex aV2 = BRepBuilderAPI_MakeVertex(gp_Pnt(1, 0, 0)); + const TopoDS_Vertex aV3 = BRepBuilderAPI_MakeVertex(gp_Pnt(2, 0, 0)); + const TopoDS_Edge anE = BRepBuilderAPI_MakeEdge(aV1, aV2); + + TopoDS_Compound aContainer; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aContainer); + aBuilder.Add(aContainer, anE); + aBuilder.Add(aContainer, aV3); + + occ::handle aReShape = new ShapeBuild_ReShape; + aReShape->Replace(anE, aContainer); + + TopoDS_Shape aResult; + ASSERT_NO_FATAL_FAILURE(aResult = aReShape->Apply(anE)); + EXPECT_FALSE(aResult.IsNull()); +} + +// Regression test for the originally reported symptom: running shape healing +// repeatedly on the same shape must not cause the edge count to grow. +// Before the UpdateWire/Apply fixes, repeated ShapeFix_Shape::Perform on +// shapes with shared sub-shapes produced geometrically growing edge counts +// from cascading replacement chains. +TEST(ShapeFix_ShapeStabilityTest, RepeatedPerformDoesNotMultiplyEdges) +{ + BRepPrimAPI_MakeBox aMakeBox(10.0, 10.0, 10.0); + const TopoDS_Shape aBox = aMakeBox.Shape(); + ASSERT_TRUE(aMakeBox.IsDone()); + + const int aNbEdgesInitial = CountSubShapes(aBox, TopAbs_EDGE); + const int aNbVerticesInitial = CountSubShapes(aBox, TopAbs_VERTEX); + const int aNbFacesInitial = CountSubShapes(aBox, TopAbs_FACE); + ASSERT_EQ(aNbEdgesInitial, 12); + + TopoDS_Shape aCurrent = aBox; + for (int anIter = 0; anIter < 5; ++anIter) + { + occ::handle aFixer = new ShapeFix_Shape(aCurrent); + aFixer->Perform(); + aCurrent = aFixer->Shape(); + ASSERT_FALSE(aCurrent.IsNull()) << "iteration " << anIter; + + // Counts must remain identical across every iteration - no multiplication, + // no loss of shapes on a valid input. + EXPECT_EQ(CountSubShapes(aCurrent, TopAbs_FACE), aNbFacesInitial) << "iteration " << anIter; + EXPECT_EQ(CountSubShapes(aCurrent, TopAbs_EDGE), aNbEdgesInitial) << "iteration " << anIter; + EXPECT_EQ(CountSubShapes(aCurrent, TopAbs_VERTEX), aNbVerticesInitial) + << "iteration " << anIter; + } +} + +// Diamond sharing through ShapeBuild_ReShape: two compounds share a vertex +// whose replacement is registered. Both parents must pick up the replacement +// and the DFS guard must not mistake the shared reference for a cycle. +TEST(ShapeBuild_ReShapeTest, Apply_DiamondSharedVertexInTwoCompounds) +{ + const TopoDS_Vertex aShared = BRepBuilderAPI_MakeVertex(gp_Pnt(0, 0, 0)); + const TopoDS_Vertex aSharedNew = BRepBuilderAPI_MakeVertex(gp_Pnt(0.1, 0, 0)); + const TopoDS_Vertex anOther1 = BRepBuilderAPI_MakeVertex(gp_Pnt(1, 0, 0)); + const TopoDS_Vertex anOther2 = BRepBuilderAPI_MakeVertex(gp_Pnt(2, 0, 0)); + + TopoDS_Compound aParentA, aParentB, aGrand; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aParentA); + aBuilder.Add(aParentA, aShared); + aBuilder.Add(aParentA, anOther1); + aBuilder.MakeCompound(aParentB); + aBuilder.Add(aParentB, aShared); + aBuilder.Add(aParentB, anOther2); + aBuilder.MakeCompound(aGrand); + aBuilder.Add(aGrand, aParentA); + aBuilder.Add(aGrand, aParentB); + + occ::handle aReShape = new ShapeBuild_ReShape; + aReShape->Replace(aShared, aSharedNew); + + const TopoDS_Shape aResult = aReShape->Apply(aGrand); + ASSERT_FALSE(aResult.IsNull()); + + int aNbReplVerts = 0; + int aNbOriginals = 0; + for (TopExp_Explorer anExp(aResult, TopAbs_VERTEX); anExp.More(); anExp.Next()) + { + if (anExp.Current().TShape() == aSharedNew.TShape()) + ++aNbReplVerts; + if (anExp.Current().TShape() == aShared.TShape()) + ++aNbOriginals; + } + EXPECT_EQ(aNbOriginals, 0) << "Original vertex must be substituted everywhere"; + EXPECT_EQ(aNbReplVerts, 2) << "Replacement must appear in both parentA and parentB"; +} diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.cxx index 784e425148..c37c7e481e 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.cxx @@ -162,109 +162,132 @@ TopoDS_Shape ShapeBuild_ReShape::Apply(const TopoDS_Shape& shape, //================================================================================================= -TopoDS_Shape ShapeBuild_ReShape::Apply(const TopoDS_Shape& shape, const TopAbs_ShapeEnum until) +TopoDS_Shape ShapeBuild_ReShape::Apply(const TopoDS_Shape& theShape, + const TopAbs_ShapeEnum theUntil) +{ + NCollection_Map> anInFlight; + return applyImpl(theShape, theUntil, anInFlight); +} + +//================================================================================================= + +TopoDS_Shape ShapeBuild_ReShape::applyImpl(const TopoDS_Shape& theShape, + const TopAbs_ShapeEnum theUntil, + NCollection_Map>& theInFlight) { myStatus = ShapeExtend::EncodeStatus(ShapeExtend_OK); - if (shape.IsNull()) - return shape; + if (theShape.IsNull()) + return theShape; - // apply direct replacement - TopoDS_Shape newsh = Value(shape); + // Apply direct replacement + TopoDS_Shape aNewShape = Value(theShape); - // if shape removed, return NULL - if (newsh.IsNull()) + // If shape removed, return NULL + if (aNewShape.IsNull()) { myStatus = ShapeExtend::EncodeStatus(ShapeExtend_DONE2); - return newsh; + return aNewShape; } - // if shape replaced, apply modifications to the result recursively + // DFS cycle guard: if theShape is already being processed further up the call + // stack, its replacement must be a compound that transitively contains it. + // Return the direct replacement without descending to break the cycle. + if (theInFlight.Contains(theShape.TShape())) + return aNewShape; + + // If shape was replaced, apply modifications to the result recursively. bool aConsLoc = ModeConsiderLocation(); - if ((aConsLoc && !newsh.IsPartner(shape)) || (!aConsLoc && !newsh.IsSame(shape))) + if ((aConsLoc && !aNewShape.IsPartner(theShape)) || (!aConsLoc && !aNewShape.IsSame(theShape))) { - TopoDS_Shape res = Apply(newsh, until); + theInFlight.Add(theShape.TShape()); + TopoDS_Shape aRes = applyImpl(aNewShape, theUntil, theInFlight); + theInFlight.Remove(theShape.TShape()); myStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE1); - return res; + return aRes; } - TopAbs_ShapeEnum st = shape.ShapeType(); - if (st >= until) - return newsh; // critere d arret - if (st == TopAbs_VERTEX || st == TopAbs_SHAPE) - return shape; - // define allowed types of components + TopAbs_ShapeEnum aST = theShape.ShapeType(); + if (aST >= theUntil) + return aNewShape; // stop criterion + if (aST == TopAbs_VERTEX || aST == TopAbs_SHAPE) + return theShape; - BRep_Builder B; + BRep_Builder aBuilder; - TopoDS_Shape result = shape.EmptyCopied(); - TopAbs_Orientation orient = shape.Orientation(); // JR/Hp: or -> orient - result.Orientation(TopAbs_FORWARD); // protect against INTERNAL or EXTERNAL shapes - bool modif = false; - int locStatus = myStatus; + TopoDS_Shape aResult = theShape.EmptyCopied(); + TopAbs_Orientation anOrient = theShape.Orientation(); + aResult.Orientation(TopAbs_FORWARD); // protect against INTERNAL or EXTERNAL shapes + bool aModif = false; + int aLocStatus = myStatus; - // apply recorded modifications to subshapes - for (TopoDS_Iterator it(shape, false); it.More(); it.Next()) + // Apply recorded modifications to subshapes + theInFlight.Add(theShape.TShape()); + for (TopoDS_Iterator anIt(theShape, false); anIt.More(); anIt.Next()) { - const TopoDS_Shape& sh = it.Value(); - newsh = Apply(sh, until); - if (newsh != sh) + const TopoDS_Shape& aSh = anIt.Value(); + aNewShape = applyImpl(aSh, theUntil, theInFlight); + if (aNewShape != aSh) { if (ShapeExtend::DecodeStatus(myStatus, ShapeExtend_DONE4)) - locStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE4); - modif = true; + aLocStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE4); + aModif = true; } - if (newsh.IsNull()) + if (aNewShape.IsNull()) { - locStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE4); + aLocStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE4); continue; } - locStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE3); - if (st == TopAbs_COMPOUND || newsh.ShapeType() == sh.ShapeType()) - { // fix for SAMTECH bug OCC322 about absence internal vertices after sewing. - B.Add(result, newsh); + aLocStatus |= ShapeExtend::EncodeStatus(ShapeExtend_DONE3); + if (aST == TopAbs_COMPOUND || aNewShape.ShapeType() == aSh.ShapeType()) + { + // Fix for SAMTECH bug OCC322 about absence internal vertices after sewing + aBuilder.Add(aResult, aNewShape); continue; } - int nitems = 0; - for (TopoDS_Iterator subit(newsh); subit.More(); subit.Next(), nitems++) + int aNbItems = 0; + for (TopoDS_Iterator aSubIt(aNewShape); aSubIt.More(); aSubIt.Next(), aNbItems++) { - const TopoDS_Shape& subsh = subit.Value(); - if (subsh.ShapeType() == sh.ShapeType()) - B.Add(result, subsh); + const TopoDS_Shape& aSubSh = aSubIt.Value(); + if (aSubSh.ShapeType() == aSh.ShapeType()) + aBuilder.Add(aResult, aSubSh); else - locStatus |= ShapeExtend::EncodeStatus(ShapeExtend_FAIL1); + aLocStatus |= ShapeExtend::EncodeStatus(ShapeExtend_FAIL1); } - if (!nitems) - locStatus |= ShapeExtend::EncodeStatus(ShapeExtend_FAIL1); + if (!aNbItems) + aLocStatus |= ShapeExtend::EncodeStatus(ShapeExtend_FAIL1); } - if (!modif) - return shape; + theInFlight.Remove(theShape.TShape()); + if (!aModif) + return theShape; - // restore Range on edge broken by EmptyCopied() - if (st == TopAbs_EDGE) + // Restore range on edge broken by EmptyCopied() + if (aST == TopAbs_EDGE) { - ShapeBuild_Edge sbe; - sbe.CopyRanges(TopoDS::Edge(result), TopoDS::Edge(shape)); + ShapeBuild_Edge anSBE; + anSBE.CopyRanges(TopoDS::Edge(aResult), TopoDS::Edge(theShape)); } - else if (st == TopAbs_WIRE || st == TopAbs_SHELL) - result.Closed(BRep_Tool::IsClosed(result)); - result.Orientation(orient); - myStatus = locStatus; + else if (aST == TopAbs_WIRE || aST == TopAbs_SHELL) + aResult.Closed(BRep_Tool::IsClosed(aResult)); + aResult.Orientation(anOrient); + myStatus = aLocStatus; - replace(shape, result, result.IsNull() ? TReplacementKind_Remove : TReplacementKind_Modify); + replace(theShape, aResult, aResult.IsNull() ? TReplacementKind_Remove : TReplacementKind_Modify); - return result; + return aResult; } //================================================================================================= -int ShapeBuild_ReShape::Status(const TopoDS_Shape& ashape, TopoDS_Shape& newsh, const bool last) +int ShapeBuild_ReShape::Status(const TopoDS_Shape& theShape, + TopoDS_Shape& theNewShape, + const bool theLast) { - return BRepTools_ReShape::Status(ashape, newsh, last); + return BRepTools_ReShape::Status(theShape, theNewShape, theLast); } //================================================================================================= -bool ShapeBuild_ReShape::Status(const ShapeExtend_Status status) const +bool ShapeBuild_ReShape::Status(const ShapeExtend_Status theStatus) const { - return ShapeExtend::DecodeStatus(myStatus, status); + return ShapeExtend::DecodeStatus(myStatus, theStatus); } diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.hxx b/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.hxx index 49e6ebe71b..f012d424f1 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.hxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeBuild/ShapeBuild_ReShape.hxx @@ -21,7 +21,9 @@ #include #include +#include #include +#include #include #include class TopoDS_Shape; @@ -104,6 +106,15 @@ public: Standard_EXPORT virtual bool Status(const ShapeExtend_Status status) const; DEFINE_STANDARD_RTTIEXT(ShapeBuild_ReShape, BRepTools_ReShape) + +private: + //! Recursive worker for Apply with a DFS in-flight set keyed by TShape handle. + //! Prevents unbounded descent when a replacement is a compound that transitively + //! contains the original shape as a sub-shape (cyclic containment, distinct from + //! cycles in the replacement map itself). + TopoDS_Shape applyImpl(const TopoDS_Shape& theShape, + const TopAbs_ShapeEnum theUntil, + NCollection_Map>& theInFlight); }; #endif // _ShapeBuild_ReShape_HeaderFile diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.cxx index 274b6d18f0..3aed7999ac 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.cxx @@ -186,6 +186,12 @@ void ShapeExtend_WireData::ComputeSeams(const bool enforce) } } + mySeamsCache.Clear(); + for (i = 1; i <= mySeams->Length(); i++) + { + mySeamsCache.Add(mySeams->Value(i)); + } + delete[] SE; // ne pas oublier !! } @@ -545,14 +551,8 @@ bool ShapeExtend_WireData::IsSeam(const int num) if (num == mySeamF || num == mySeamR) return true; - // Pas suffisant : on regarde dans la liste - int i, nb = mySeams->Length(); - for (i = 1; i <= nb; i++) - { - if (num == mySeams->Value(i)) - return true; - } - return false; + // Use hash set for O(1) lookup instead of O(n) linear search + return mySeamsCache.Contains(num); } //================================================================================================= diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.hxx b/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.hxx index 35b08083d4..1003d8b932 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.hxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeExtend/ShapeExtend_WireData.hxx @@ -25,6 +25,8 @@ #include #include #include +#include + class TopoDS_Wire; class TopoDS_Edge; class TopoDS_Shape; @@ -221,6 +223,7 @@ private: occ::handle> myEdges; occ::handle> myNonmanifoldEdges; occ::handle> mySeams; + TColStd_PackedMapOfInteger mySeamsCache; int mySeamF; int mySeamR; bool myManifoldMode; diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx index 3ca576ebff..6ece0a5951 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx @@ -610,7 +610,13 @@ bool ShapeFix_Edge::FixVertexTolerance(const TopoDS_Edge& edge, const TopoDS_Fac ShapeAnalysis_Edge sae; if (!Context().IsNull()) { - anEdgeCopy = TopoDS::Edge(Context()->Apply(edge)); + const TopoDS_Shape& aShape = Context()->Apply(edge); + if (aShape.IsNull() || aShape.ShapeType() != TopAbs_EDGE) + { + return false; + } + + anEdgeCopy = TopoDS::Edge(aShape); } double toler1, toler2; @@ -645,7 +651,13 @@ bool ShapeFix_Edge::FixVertexTolerance(const TopoDS_Edge& edge) ShapeAnalysis_Edge sae; if (!Context().IsNull()) { - anEdgeCopy = TopoDS::Edge(Context()->Apply(edge)); + const TopoDS_Shape& aShape = Context()->Apply(edge); + if (aShape.IsNull() || aShape.ShapeType() != TopAbs_EDGE) + { + return false; + } + + anEdgeCopy = TopoDS::Edge(aShape); } double toler1, toler2; if (!sae.CheckVertexTolerance(anEdgeCopy, toler1, toler2)) diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.cxx index 96bd1ded05..60a4434dae 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.cxx @@ -284,7 +284,10 @@ static bool SplitWire(const TopoDS_Face& face, double a1, b1, a2, b2; occ::handle curve1 = BRep_Tool::CurveOnSurface(E1, face, a1, b1); occ::handle curve2 = BRep_Tool::CurveOnSurface(E2, face, a2, b2); - gp_Pnt2d v0, v1; + if (curve1.IsNull() || curve2.IsNull()) + continue; + + gp_Pnt2d v0, v1; if (E1.Orientation() == TopAbs_REVERSED) a1 = b1; if (E2.Orientation() == TopAbs_REVERSED) @@ -316,7 +319,7 @@ static bool SplitWire(const TopoDS_Face& face, //================================================================================================= -bool ShapeFix_Face::Perform() +bool ShapeFix_Face::Perform(const Message_ProgressRange& theProgress) { myStatus = ShapeExtend::EncodeStatus(ShapeExtend_OK); myFixWire->SetContext(Context()); @@ -367,6 +370,7 @@ bool ShapeFix_Face::Perform() } isfixReorder = false; + for (TopoDS_Iterator iter(S, false); iter.More(); iter.Next()) { if (iter.Value().ShapeType() != TopAbs_WIRE) @@ -387,7 +391,7 @@ bool ShapeFix_Face::Perform() } continue; } - if (theAdvFixWire->Perform()) + if (theAdvFixWire->Perform(theProgress)) { // fixed = true; isfixReorder = (theAdvFixWire->StatusReorder(ShapeExtend_DONE) || isfixReorder); diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.hxx b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.hxx index 4509caae97..6f34cce18a 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.hxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Face.hxx @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -162,7 +163,7 @@ public: //! ShapeExtend_FAIL2: cannot fix orientation of wires //! ShapeExtend_FAIL3: cannot add missing seam //! ShapeExtend_FAIL4: cannot remove small area wire - Standard_EXPORT bool Perform(); + Standard_EXPORT bool Perform(const Message_ProgressRange& theProgress = Message_ProgressRange()); //! Fixes orientation of wires on the face //! It tries to make all wires lie outside all others (according diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_IntersectionTool.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_IntersectionTool.cxx index 602c5a9f09..a76721ab07 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_IntersectionTool.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_IntersectionTool.cxx @@ -1648,6 +1648,7 @@ bool ShapeFix_IntersectionTool::FixIntersectingWires(TopoDS_Face& face) const // TopoDS_Shape SF = TopoDS::Face(S); TopoDS_Shape SF = face; TopAbs_Orientation ori = face.Orientation(); + NCollection_Map anAddedWires; NCollection_Sequence SeqWir; NCollection_Sequence SeqNMShapes; for (TopoDS_Iterator iter(SF, false); iter.More(); iter.Next()) @@ -1660,7 +1661,9 @@ bool ShapeFix_IntersectionTool::FixIntersectingWires(TopoDS_Face& face) const continue; } TopoDS_Wire wire = TopoDS::Wire(iter.Value()); - SeqWir.Append(wire); + // Prevent duplicate wires from being added to the sequence + if (anAddedWires.Add(wire)) + SeqWir.Append(wire); } if (SeqWir.Length() < 2) return false; // gka 06.09.04 diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.cxx index 5cf3b2ea58..df36a20afb 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.cxx @@ -292,7 +292,7 @@ int ShapeFix_Wire::NbEdges() const // are limited if order of edges in the wire is not OK //======================================================================= -bool ShapeFix_Wire::Perform() +bool ShapeFix_Wire::Perform(const Message_ProgressRange& theProgress) { ClearStatuses(); if (!IsLoaded()) @@ -316,6 +316,9 @@ bool ShapeFix_Wire::Perform() ReorderOK = !StatusReorder(ShapeExtend_FAIL); } + if (theProgress.UserBreak()) + return false; + // FixSmall is allowed to change topology only if mode is set and FixReorder // did not failed if (NeedFix(myFixSmallMode, myTopoMode)) @@ -332,12 +335,18 @@ bool ShapeFix_Wire::Perform() } } + if (theProgress.UserBreak()) + return false; + if (NeedFix(myFixConnectedMode, ReorderOK)) { if (FixConnected()) Fixed = true; } + if (theProgress.UserBreak()) + return false; + if (NeedFix(myFixEdgeCurvesMode)) { int savFixShiftedMode = myFixShiftedMode; @@ -349,12 +358,18 @@ bool ShapeFix_Wire::Perform() myFixShiftedMode = savFixShiftedMode; } + if (theProgress.UserBreak()) + return false; + if (NeedFix(myFixDegeneratedMode)) { if (FixDegenerated()) Fixed = true; // ?? if ! ReorderOK ?? } + if (theProgress.UserBreak()) + return false; + // pdn - temporary to test if (myFixTailMode <= 0 && NeedFix(myFixNotchedEdgesMode, ReorderOK)) { @@ -363,6 +378,9 @@ bool ShapeFix_Wire::Perform() FixShifted(); // skl 07.03.2002 for OCC180 } + if (theProgress.UserBreak()) + return false; + if (myFixTailMode != 0) { if (FixTails()) @@ -372,6 +390,9 @@ bool ShapeFix_Wire::Perform() } } + if (theProgress.UserBreak()) + return false; + if (NeedFix(myFixSelfIntersectionMode, myClosedMode)) { int savFixIntersectingEdgesMode = myFixIntersectingEdgesMode; @@ -384,19 +405,30 @@ bool ShapeFix_Wire::Perform() myFixIntersectingEdgesMode = savFixIntersectingEdgesMode; } + if (theProgress.UserBreak()) + return false; + if (NeedFix(myFixLackingMode, ReorderOK)) { if (FixLacking()) Fixed = true; } + if (theProgress.UserBreak()) + return false; + // TEMPORARILY without special mode !!! occ::handle sbwd = WireData(); for (int iedge = 1; iedge <= sbwd->NbEdges(); iedge++) + { if (myFixEdge->FixVertexTolerance(sbwd->Edge(iedge), Face())) { Fixed = true; } + } + + if (theProgress.UserBreak()) + return false; if (!Context().IsNull()) UpdateWire(); @@ -483,10 +515,16 @@ bool ShapeFix_Wire::FixConnected(const double prec) int stop = (myClosedMode ? 0 : 1); for (int i = NbEdges(); i > stop; i--) { - FixConnected(i, prec); + // Call without UpdateWire to avoid O(n^2) behavior in the loop + FixConnected(i, prec, false); myStatusConnected |= myLastFixStatus; } + // Update wire once after all connections are fixed + // Using Value() in UpdateWire() prevents edge explosion from replacement chains + if (!Context().IsNull()) + UpdateWire(); + return StatusConnected(ShapeExtend_DONE); } @@ -1240,14 +1278,13 @@ bool ShapeFix_Wire::FixSmall(const int num, const bool lockvtx, const double pre //================================================================================================= -bool ShapeFix_Wire::FixConnected(const int num, const double prec) +bool ShapeFix_Wire::FixConnected(const int num, const double prec, const bool theUpdateWire) { myLastFixStatus = ShapeExtend::EncodeStatus(ShapeExtend_OK); if (!IsLoaded() || NbEdges() <= 0) return false; // analysis - myAnalyzer->CheckConnected(num, prec < 0 ? MaxTolerance() : prec); if (myAnalyzer->LastCheckStatus(ShapeExtend_FAIL)) { @@ -1257,7 +1294,6 @@ bool ShapeFix_Wire::FixConnected(const int num, const double prec) return false; // action: replacing vertex - occ::handle sbwd = WireData(); int n2 = (num > 0 ? num : sbwd->NbEdges()); int n1 = (n2 > 1 ? n2 - 1 : sbwd->NbEdges()); @@ -1351,7 +1387,9 @@ bool ShapeFix_Wire::FixConnected(const int num, const double prec) } } } - if (!Context().IsNull()) + + // Optionally update wire data with context replacements + if (theUpdateWire && !Context().IsNull()) UpdateWire(); return true; @@ -3772,9 +3810,11 @@ void ShapeFix_Wire::UpdateWire() occ::handle sbwd = WireData(); for (int i = 1; i <= sbwd->NbEdges(); i++) { - TopoDS_Edge E = sbwd->Edge(i); - TopoDS_Shape S = Context()->Apply(E); - if (S == E) + TopoDS_Edge E = sbwd->Edge(i); + // ValueLeaf follows the replacement chain without descending into sub-shapes, + // so a previously-split edge is not re-expanded on subsequent Perform() passes. + TopoDS_Shape S = Context()->ValueLeaf(E); + if (!S.IsNull() && S.IsEqual(E)) continue; for (TopExp_Explorer exp(S, TopAbs_EDGE); exp.More(); exp.Next()) sbwd->Add(exp.Current(), i++); diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.hxx b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.hxx index 1efeb6489b..66b8637470 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.hxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Wire.hxx @@ -19,6 +19,7 @@ #include +#include #include #include #include @@ -268,7 +269,7 @@ public: //! FixDegenerated (if wire is ordered) //! FixSelfIntersection (if wire is ordered and ClosedMode is True) //! FixLacking (if wire is ordered) - Standard_EXPORT bool Perform(); + Standard_EXPORT bool Perform(const Message_ProgressRange& theProgress = Message_ProgressRange()); //! Performs an analysis and reorders edges in the wire using class WireOrder. //! Flag determines the use of miscible mode if necessary. @@ -350,7 +351,10 @@ public: //! the same one //! Tests with starting preci or, if given greater, //! If is -1 then MaxTolerance() is taken. - Standard_EXPORT bool FixConnected(const int num, const double prec); + //! If is true, synchronizes wire data with context replacements. + Standard_EXPORT bool FixConnected(const int num, + const double prec, + const bool theUpdateWire = true); //! Fixes a seam edge //! A Seam edge has two pcurves, one for forward. one for reversed diff --git a/src/ModelingAlgorithms/TKShHealing/ShapeProcess/ShapeProcess_ShapeContext.cxx b/src/ModelingAlgorithms/TKShHealing/ShapeProcess/ShapeProcess_ShapeContext.cxx index 7ce0a5f92c..408b768e7f 100644 --- a/src/ModelingAlgorithms/TKShHealing/ShapeProcess/ShapeProcess_ShapeContext.cxx +++ b/src/ModelingAlgorithms/TKShHealing/ShapeProcess/ShapeProcess_ShapeContext.cxx @@ -282,9 +282,17 @@ static void RecModif(const TopoDS_Shape& msgmap = msg->MapShape(); if (msgmap.IsBound(S)) next = S; + + // Map to prevent infinite loops in case of cyclic replacements. + NCollection_Map aVisitedShapes; do { cur = next; + if (!aVisitedShapes.Add(cur)) + { + break; + } + if (msgmap.IsBound(cur)) { const NCollection_List& msglist = msgmap.Find(cur); diff --git a/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.cxx b/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.cxx index 27d9e05c35..c2b723e003 100644 --- a/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.cxx +++ b/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.cxx @@ -185,6 +185,44 @@ void BRepTools_ReShape::replace(const TopoDS_Shape& ashape, std::cout << "Warning: BRepTools_ReShape::Replace: shape already recorded" << std::endl; #endif + // Reject replacements that would introduce a cycle into the replacement chain, + // e.g. A -> ... -> X -> A. Walk forward from newshape via Value(); if the walk + // ever lands on shape itself, record only an identity (effectively no-op) to + // avoid forming a cycle that would later deadlock Apply()/ValueLeaf(). + if (theKind != TReplacementKind_Remove && !newshape.IsNull() && !newshape.IsPartner(shape)) + { + // Reject replacements that would close a cycle in the map. Walk forward from + // newshape via Value(); if the chain ever lands back on shape's underlying TShape + // (any orientation, any location), abort. Key by the TShape handle to mirror the + // identity the map itself uses once orientation/location are normalized away. + TopoDS_Shape aProbe = newshape; + NCollection_Map> aSeen; + aSeen.Add(shape.TShape()); + aSeen.Add(aProbe.TShape()); + bool aCycle = false; + for (;;) + { + const TopoDS_Shape aNext = Value(aProbe); + if (aNext.IsNull() || aNext.IsSame(aProbe)) + break; + if (aNext.IsPartner(shape)) + { + aCycle = true; + break; + } + if (!aSeen.Add(aNext.TShape())) + break; // existing cycle in data - not ours to introduce, bail + aProbe = aNext; + } + if (aCycle) + { +#ifdef OCCT_DEBUG + std::cout << "Warning: BRepTools_ReShape::Replace: cycle rejected" << std::endl; +#endif + return; + } + } + myShapeToReplacement.Bind(shape, TReplacement(newshape, theKind)); myNewShapes.Add(newshape); } @@ -251,6 +289,39 @@ TopoDS_Shape BRepTools_ReShape::Value(const TopoDS_Shape& ashape) const //================================================================================================= +TopoDS_Shape BRepTools_ReShape::ValueLeaf(const TopoDS_Shape& theShape) const +{ + if (theShape.IsNull()) + return TopoDS_Shape(); + + // Track visited shapes by their underlying TShape. Rationale: the replacement map keys + // entries via TopTools_ShapeMapHasher (orientation-ignoring IsSame) and, when + // myConsiderLocation is set, strips locations on both insertion and lookup. That leaves + // TShape as the only identity axis that's stable across the walk under every mode - + // so keying the cycle guard on the TShape handle catches cycles that would otherwise + // slip past orientation/location-sensitive comparisons. + TopoDS_Shape aCurrent = theShape; + NCollection_Map> aVisited; + aVisited.Add(aCurrent.TShape()); + + for (;;) + { + const TopoDS_Shape aNext = Value(aCurrent); + if (aNext.IsNull()) + return aNext; + if (aNext.IsSame(aCurrent)) + return aNext; + if (!aVisited.Add(aNext.TShape())) + { + // Cycle in replacement data - return current best to avoid looping. + return aNext; + } + aCurrent = aNext; + } +} + +//================================================================================================= + int BRepTools_ReShape::Status(const TopoDS_Shape& ashape, TopoDS_Shape& newsh, const bool last) { int res = 0; @@ -361,6 +432,16 @@ static int EncodeStatus(const int status) //================================================================================================= TopoDS_Shape BRepTools_ReShape::Apply(const TopoDS_Shape& shape, const TopAbs_ShapeEnum until) +{ + NCollection_Map> anInFlight; + return applyImpl(shape, until, anInFlight); +} + +//================================================================================================= + +TopoDS_Shape BRepTools_ReShape::applyImpl(const TopoDS_Shape& shape, + const TopAbs_ShapeEnum until, + NCollection_Map>& theInFlight) { myStatus = EncodeStatus(0); // ShapeExtend::EncodeStatus ( ShapeExtend_OK ); if (shape.IsNull()) @@ -376,11 +457,19 @@ TopoDS_Shape BRepTools_ReShape::Apply(const TopoDS_Shape& shape, const TopAbs_Sh return newsh; } + // DFS cycle guard: if shape is already being processed further up the call + // stack, its replacement must be a compound that transitively contains it. + // Return the direct replacement without descending to break the cycle. + if (theInFlight.Contains(shape.TShape())) + return newsh; + // if shape replaced, apply modifications to the result recursively if ((myConsiderLocation && !newsh.IsPartner(shape)) || (!myConsiderLocation && !newsh.IsSame(shape))) { - TopoDS_Shape res = Apply(newsh, until); + theInFlight.Add(shape.TShape()); + TopoDS_Shape res = applyImpl(newsh, until, theInFlight); + theInFlight.Remove(shape.TShape()); myStatus |= EncodeStatus(1); // ShapeExtend::EncodeStatus ( ShapeExtend_DONE1 ); return res; } @@ -416,10 +505,11 @@ TopoDS_Shape BRepTools_ReShape::Apply(const TopoDS_Shape& shape, const TopAbs_Sh // apply recorded modifications to subshapes bool isEmpty = true; + theInFlight.Add(shape.TShape()); for (TopoDS_Iterator it(shape, false); it.More(); it.Next()) { const TopoDS_Shape& sh = it.Value(); - newsh = Apply(sh, until); + newsh = applyImpl(sh, until, theInFlight); if (newsh != sh) { if (myStatus & EncodeStatus(4)) // ShapeExtend::DecodeStatus ( myStatus, ShapeExtend_DONE4 ) ) @@ -450,6 +540,7 @@ TopoDS_Shape BRepTools_ReShape::Apply(const TopoDS_Shape& shape, const TopAbs_Sh if ( ! nitems ) locStatus |= EncodeStatus(10);//ShapeExtend::EncodeStatus ( ShapeExtend_FAIL1 ); // clang-format on } + theInFlight.Remove(shape.TShape()); if (!modif) return shape; diff --git a/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.hxx b/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.hxx index 703e523c3e..e77a7773f6 100644 --- a/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.hxx +++ b/src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.hxx @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -98,6 +99,14 @@ public: //! Else, returns the replacing item Standard_EXPORT virtual TopoDS_Shape Value(const TopoDS_Shape& shape) const; + //! Follows the replacement chain for @p theShape to its leaf without descending into sub-shapes. + //! Iterates Value() until a fixpoint is reached. Unlike Apply(), this does not rebuild + //! the shape from its children, so it is safe to call on edges/wires whose sub-shapes + //! have their own pending replacements (avoids cascading sub-shape re-expansion). + //! @return the final replacement, or the original shape if not recorded, + //! or a Null shape if the chain terminates in a Remove. + Standard_EXPORT TopoDS_Shape ValueLeaf(const TopoDS_Shape& theShape) const; + //! Returns a complete substitution status for a shape //! 0 : not recorded, = original //! < 0: to be removed, is NULL @@ -173,6 +182,14 @@ protected: const TopoDS_Shape& newshape, const TReplacementKind theKind); + //! Recursive worker for Apply with a DFS in-flight set keyed by TShape handle. + //! Prevents unbounded descent when a replacement is a compound that transitively + //! contains the original shape as a sub-shape (cyclic containment, distinct from + //! cycles in the replacement map itself). + Standard_EXPORT TopoDS_Shape applyImpl(const TopoDS_Shape& theShape, + const TopAbs_ShapeEnum theUntil, + NCollection_Map>& theInFlight); + private: //! Returns 'true' if the kind of a replacement is an ordinary merging. static bool isOrdinaryMerged(const TReplacementKind theKind) diff --git a/src/ModelingData/TKBRep/GTests/BRepTools_ReShape_Test.cxx b/src/ModelingData/TKBRep/GTests/BRepTools_ReShape_Test.cxx new file mode 100644 index 0000000000..ab46e46816 --- /dev/null +++ b/src/ModelingData/TKBRep/GTests/BRepTools_ReShape_Test.cxx @@ -0,0 +1,349 @@ +// Copyright (c) 2026 OPEN CASCADE SAS +// +// This file is part of Open CASCADE Technology software library. +// +// This library is free software; you can redistribute it and/or modify it under +// the terms of the GNU Lesser General Public License version 2.1 as published +// by the Free Software Foundation, with special exception defined in the file +// OCCT_LGPL_EXCEPTION.txt. Consult the file LICENSE_LGPL_21.txt included in OCCT +// distribution for complete text of the license and disclaimer of any warranty. +// +// Alternatively, this file may be used under the terms of Open CASCADE +// commercial license or contractual agreement. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace +{ +TopoDS_Vertex MakeVertex(const double theX, const double theY, const double theZ) +{ + return BRepBuilderAPI_MakeVertex(gp_Pnt(theX, theY, theZ)); +} + +TopoDS_Edge MakeEdge(const TopoDS_Vertex& theV1, const TopoDS_Vertex& theV2) +{ + return BRepBuilderAPI_MakeEdge(theV1, theV2); +} +} // namespace + +// Value() returns the direct replacement only (one hop); ValueLeaf() follows the +// chain through intermediate replacements to its terminal shape. +TEST(BRepTools_ReShapeTest, ValueLeaf_FollowsChainToLeaf) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + const TopoDS_Vertex aC = MakeVertex(2, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aB); + aReShape.Replace(aB, aC); + + EXPECT_TRUE(aReShape.Value(aA).IsSame(aB)) << "Value() must yield only the direct replacement"; + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aC)) << "ValueLeaf() must walk the full chain"; + EXPECT_TRUE(aReShape.ValueLeaf(aB).IsSame(aC)); + EXPECT_TRUE(aReShape.ValueLeaf(aC).IsSame(aC)) << "Terminal element is a fixpoint"; +} + +// ValueLeaf() must terminate on an unrecorded shape without touching the map. +TEST(BRepTools_ReShapeTest, ValueLeaf_UnrecordedShapeIsIdentity) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + + BRepTools_ReShape aReShape; + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aA)); +} + +// A chain ending in a removal (null replacement) must surface as a null shape. +TEST(BRepTools_ReShapeTest, ValueLeaf_ChainEndingInRemoveReturnsNull) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aB); + aReShape.Remove(aB); + + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsNull()); +} + +// A direct A->B then B->A would close a cycle in the replacement map. +// The second Replace() must be rejected; the first stays intact. +TEST(BRepTools_ReShapeTest, Replace_RejectsDirectCycle) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aB); + aReShape.Replace(aB, aA); // would create A -> B -> A + + EXPECT_TRUE(aReShape.Value(aA).IsSame(aB)) << "First replacement must remain in effect"; + EXPECT_TRUE(aReShape.Value(aB).IsSame(aB)) << "Cyclic second replacement must have been rejected"; + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aB)) << "Chain must terminate, not loop"; +} + +// A longer cycle A -> B -> C -> A must also be rejected at the closing edge. +TEST(BRepTools_ReShapeTest, Replace_RejectsLongerCycle) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + const TopoDS_Vertex aC = MakeVertex(2, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aB); + aReShape.Replace(aB, aC); + aReShape.Replace(aC, aA); // would create A -> B -> C -> A + + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aC)) << "Chain terminates at C; no cycle formed"; + EXPECT_TRUE(aReShape.Value(aC).IsSame(aC)) << "Closing replacement was rejected"; +} + +// Apply() must not stack-overflow when a shape's replacement is a compound that +// transitively contains the original shape as a sub-shape. This is the pattern +// that crashed in shape healing on non-orientable / shared-edge inputs. +TEST(BRepTools_ReShapeTest, Apply_HandlesStructuralContainmentWithoutCrash) +{ + const TopoDS_Vertex aV1 = MakeVertex(0, 0, 0); + const TopoDS_Vertex aV2 = MakeVertex(1, 0, 0); + const TopoDS_Vertex aV3 = MakeVertex(2, 0, 0); + const TopoDS_Edge anE = MakeEdge(aV1, aV2); + + // Build a compound that contains the edge (plus another shape for flavour). + TopoDS_Compound aContainer; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aContainer); + aBuilder.Add(aContainer, anE); + aBuilder.Add(aContainer, aV3); + + BRepTools_ReShape aReShape; + aReShape.Replace(anE, aContainer); // e -> compound{e, v3} + + TopoDS_Shape aResult; + ASSERT_NO_FATAL_FAILURE(aResult = aReShape.Apply(anE)); + EXPECT_FALSE(aResult.IsNull()); +} + +// Legitimate diamond sharing (the same sub-shape reached via different parent +// paths) must continue to process normally; the DFS guard must not mistake it +// for a cycle. Two edges sharing a vertex form the minimal diamond. +TEST(BRepTools_ReShapeTest, Apply_DiamondSharingIsProcessedCorrectly) +{ + const TopoDS_Vertex aV1 = MakeVertex(0, 0, 0); + const TopoDS_Vertex aV2 = MakeVertex(1, 0, 0); + const TopoDS_Vertex aV3 = MakeVertex(2, 0, 0); + const TopoDS_Vertex aV2Repl = MakeVertex(1.5, 0, 0); + + // aE1 and aE2 both reference aV2 as an endpoint - that's the diamond. + const TopoDS_Edge aE1 = MakeEdge(aV1, aV2); + const TopoDS_Edge aE2 = MakeEdge(aV2, aV3); + + TopoDS_Compound aPair; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aPair); + aBuilder.Add(aPair, aE1); + aBuilder.Add(aPair, aE2); + + BRepTools_ReShape aReShape; + aReShape.Replace(aV2, aV2Repl); + + const TopoDS_Shape aResult = aReShape.Apply(aPair); + ASSERT_FALSE(aResult.IsNull()); + + // aV2 must have been substituted everywhere it appeared; aV2Repl must appear; + // the original aV2 TShape must no longer be present in the result. + int aNbReplVertices = 0; + for (TopExp_Explorer anExp(aResult, TopAbs_VERTEX); anExp.More(); anExp.Next()) + { + EXPECT_FALSE(anExp.Current().TShape() == aV2.TShape()) + << "Original v2 must have been replaced on every diamond arm"; + if (anExp.Current().TShape() == aV2Repl.TShape()) + ++aNbReplVertices; + } + EXPECT_GE(aNbReplVertices, 2) << "Replacement must appear on both arms of the diamond"; +} + +// Clear() must drop every binding and reset the map to identity behaviour. +TEST(BRepTools_ReShapeTest, Clear_DropsAllBindings) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aB); + ASSERT_TRUE(aReShape.Value(aA).IsSame(aB)); + + aReShape.Clear(); + EXPECT_TRUE(aReShape.Value(aA).IsSame(aA)); + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aA)); +} + +// Re-replacing the same shape must overwrite the previous binding. +// This is the documented behaviour and must not be blocked by the cycle guard. +TEST(BRepTools_ReShapeTest, Replace_LastWins) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + const TopoDS_Vertex aC = MakeVertex(2, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aB); + aReShape.Replace(aA, aC); // overwrite + + EXPECT_TRUE(aReShape.Value(aA).IsSame(aC)); + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aC)); +} + +// Replace(X, X) is a no-op - the shape equals itself, no binding should be stored. +TEST(BRepTools_ReShapeTest, Replace_SelfIsNoOp) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.Replace(aA, aA); + + EXPECT_FALSE(aReShape.IsRecorded(aA)); + EXPECT_TRUE(aReShape.Value(aA).IsSame(aA)); +} + +// A Remove request must propagate through Apply() so that the sub-shape is +// dropped from the rebuilt parent. +TEST(BRepTools_ReShapeTest, Apply_RemoveDropsSubShapeFromParent) +{ + const TopoDS_Vertex aV1 = MakeVertex(0, 0, 0); + const TopoDS_Vertex aV2 = MakeVertex(1, 0, 0); + const TopoDS_Vertex aV3 = MakeVertex(2, 0, 0); + + TopoDS_Compound aParent; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aParent); + aBuilder.Add(aParent, aV1); + aBuilder.Add(aParent, aV2); + aBuilder.Add(aParent, aV3); + + BRepTools_ReShape aReShape; + aReShape.Remove(aV2); + + const TopoDS_Shape aResult = aReShape.Apply(aParent); + ASSERT_FALSE(aResult.IsNull()); + + int aNbVerts = 0; + for (TopoDS_Iterator anIt(aResult); anIt.More(); anIt.Next()) + { + EXPECT_FALSE(anIt.Value().TShape() == aV2.TShape()) << "v2 should have been removed"; + ++aNbVerts; + } + EXPECT_EQ(aNbVerts, 2); +} + +// The fuller Moebius-style pattern: an edge appears in two different parents +// (simulated here by two compounds), each parent is rebuilt through Apply, +// and the edge replacement must propagate to both without triggering the +// DFS cycle guard. Regression for the original "shared sub-shape" failure. +TEST(BRepTools_ReShapeTest, Apply_SharedEdgeAcrossTwoParents) +{ + const TopoDS_Vertex aV1 = MakeVertex(0, 0, 0); + const TopoDS_Vertex aV2 = MakeVertex(1, 0, 0); + const TopoDS_Vertex aV3 = MakeVertex(2, 0, 0); + const TopoDS_Edge aE1 = MakeEdge(aV1, aV2); + const TopoDS_Edge aE2 = MakeEdge(aV2, aV3); + const TopoDS_Edge aE1New = MakeEdge(MakeVertex(0.5, 0, 0), aV2); + + // Build two parents both referencing aE1: parentA{aE1, aV3}, parentB{aE1, aE2}. + TopoDS_Compound aParentA, aParentB; + BRep_Builder aBuilder; + aBuilder.MakeCompound(aParentA); + aBuilder.Add(aParentA, aE1); + aBuilder.Add(aParentA, aV3); + aBuilder.MakeCompound(aParentB); + aBuilder.Add(aParentB, aE1); + aBuilder.Add(aParentB, aE2); + + BRepTools_ReShape aReShape; + aReShape.Replace(aE1, aE1New); + + const TopoDS_Shape aResultA = aReShape.Apply(aParentA); + const TopoDS_Shape aResultB = aReShape.Apply(aParentB); + ASSERT_FALSE(aResultA.IsNull()); + ASSERT_FALSE(aResultB.IsNull()); + + bool aFoundNewInA = false, aFoundNewInB = false; + for (TopExp_Explorer anExp(aResultA, TopAbs_EDGE); anExp.More(); anExp.Next()) + { + EXPECT_FALSE(anExp.Current().TShape() == aE1.TShape()); + if (anExp.Current().TShape() == aE1New.TShape()) + aFoundNewInA = true; + } + for (TopExp_Explorer anExp(aResultB, TopAbs_EDGE); anExp.More(); anExp.Next()) + { + EXPECT_FALSE(anExp.Current().TShape() == aE1.TShape()); + if (anExp.Current().TShape() == aE1New.TShape()) + aFoundNewInB = true; + } + EXPECT_TRUE(aFoundNewInA); + EXPECT_TRUE(aFoundNewInB); +} + +// Idempotence: Apply() on a shape that has no bindings must return +// the shape unchanged. Guards against accidental rebuild-on-no-change. +TEST(BRepTools_ReShapeTest, Apply_NoBindingsIsIdentity) +{ + const TopoDS_Vertex aV1 = MakeVertex(0, 0, 0); + const TopoDS_Vertex aV2 = MakeVertex(1, 0, 0); + const TopoDS_Edge anE = MakeEdge(aV1, aV2); + + BRepTools_ReShape aReShape; + const TopoDS_Shape aResult = aReShape.Apply(anE); + EXPECT_TRUE(aResult.IsSame(anE)); +} + +// Deep structural containment: a replacement compound contains a nested +// compound that in turn contains the original shape. DFS guard must still +// break the cycle regardless of depth. +TEST(BRepTools_ReShapeTest, Apply_DeepStructuralContainmentWithoutCrash) +{ + const TopoDS_Vertex aV1 = MakeVertex(0, 0, 0); + const TopoDS_Vertex aV2 = MakeVertex(1, 0, 0); + const TopoDS_Vertex anExtra = MakeVertex(3, 0, 0); + const TopoDS_Edge anE = MakeEdge(aV1, aV2); + + // inner{e}, outer{inner, extra}: e is reachable two levels down. + TopoDS_Compound anInner, anOuter; + BRep_Builder aBuilder; + aBuilder.MakeCompound(anInner); + aBuilder.Add(anInner, anE); + aBuilder.MakeCompound(anOuter); + aBuilder.Add(anOuter, anInner); + aBuilder.Add(anOuter, anExtra); + + BRepTools_ReShape aReShape; + aReShape.Replace(anE, anOuter); // e -> outer{inner{e}, extra} + + TopoDS_Shape aResult; + ASSERT_NO_FATAL_FAILURE(aResult = aReShape.Apply(anE)); + EXPECT_FALSE(aResult.IsNull()); +} + +// Locations must be tracked when ModeConsiderLocation is enabled. +TEST(BRepTools_ReShapeTest, ValueLeaf_ConsidersLocationMode) +{ + const TopoDS_Vertex aA = MakeVertex(0, 0, 0); + const TopoDS_Vertex aB = MakeVertex(1, 0, 0); + + BRepTools_ReShape aReShape; + aReShape.ModeConsiderLocation() = true; + aReShape.Replace(aA, aB); + + EXPECT_TRUE(aReShape.ValueLeaf(aA).IsSame(aB)); +} diff --git a/src/ModelingData/TKBRep/GTests/FILES.cmake b/src/ModelingData/TKBRep/GTests/FILES.cmake index 290cd72960..d7453e57df 100644 --- a/src/ModelingData/TKBRep/GTests/FILES.cmake +++ b/src/ModelingData/TKBRep/GTests/FILES.cmake @@ -46,6 +46,7 @@ set(OCCT_TKBRep_GTests_FILES BRepGraph_Validate_Test.cxx BRepGraph_ScenarioMatrix_Test.cxx BRepGraph_Deduplicate_Test.cxx + BRepTools_ReShape_Test.cxx TopExp_Test.cxx TopoDS_Builder_Test.cxx TopoDS_Edge_Test.cxx diff --git a/tests/bugs/modalg_7/bug30396_2 b/tests/bugs/modalg_7/bug30396_2 index 5ad8ce4b43..6f7ba3a2e8 100644 --- a/tests/bugs/modalg_7/bug30396_2 +++ b/tests/bugs/modalg_7/bug30396_2 @@ -3,8 +3,6 @@ puts "0030396: Infinite recursion during ShapeFix after BRepAlgoAPI_Cut" puts "========" puts "" -puts "TODO 30396 ALL:TEST INCOMPLETE" - # The shape bug30396_bad_result.brep has been saved in OCCT 7.3, before BO has been fixed. restore [locate_data_file bug30396_bad_result.brep] a # The Draw crashed here diff --git a/tests/de/step_2/R7 b/tests/de/step_2/R7 index 290cffa89d..61eb1ef4e7 100644 --- a/tests/de/step_2/R7 +++ b/tests/de/step_2/R7 @@ -7,9 +7,9 @@ set filename trj10_b2-oc-214.stp set ref_data { DATA : Faulties = 0 ( 0 ) Warnings = 0 ( 0 ) Summary = 0 ( 0 ) TPSTAT : Faulties = 0 ( 0 ) Warnings = 5 ( 3 ) Summary = 5 ( 3 ) -CHECKSHAPE : Wires = 0 ( 0 ) Faces = 0 ( 0 ) Shells = 0 ( 0 ) Solids = 0 ( 0 ) -NBSHAPES : Solid = 1 ( 1 ) Shell = 1 ( 1 ) Face = 251 ( 251 ) -STATSHAPE : Solid = 1 ( 1 ) Shell = 1 ( 1 ) Face = 251 ( 251 ) FreeWire = 0 ( 0 ) +CHECKSHAPE : Wires = 1 ( 1 ) Faces = 1 ( 1 ) Shells = 0 ( 0 ) Solids = 0 ( 0 ) +NBSHAPES : Solid = 0 ( 0 ) Shell = 1 ( 1 ) Face = 250 ( 250 ) +STATSHAPE : Solid = 0 ( 0 ) Shell = 1 ( 1 ) Face = 250 ( 250 ) FreeWire = 0 ( 0 ) TOLERANCE : MaxTol = 0.002562014753 ( 0.00500444492 ) AvgTol = 2.347098051e-005 ( 7.280140961e-005 ) LABELS : N0Labels = 1 ( 1 ) N1Labels = 0 ( 0 ) N2Labels = 0 ( 0 ) TotalLabels = 1 ( 1 ) NameLabels = 1 ( 1 ) ColorLabels = 0 ( 0 ) LayerLabels = 0 ( 0 ) PROPS : Centroid = 1 ( 1 ) Volume = 1 ( 1 ) Area = 1 ( 1 )