Shape Healing - Refactor shape replacement logic to handle cycles (#1247)

Apply some fixes where aaa82fc4de introduce as a regressions
This commit is contained in:
Pasukhin Dmitry
2026-04-30 00:01:00 +01:00
committed by GitHub
parent 91be8c4c71
commit 883cece8e3
7 changed files with 65 additions and 85 deletions
@@ -261,6 +261,9 @@ static bool SplitWire(const TopoDS_Face& face,
occ::handle<ShapeExtend_WireData> sewd1 = new ShapeExtend_WireData;
sewd1->Add(E1);
bool IsConnectedEdge = true;
// Set to true when null pcurves prevent the 2D closure verification:
// the partial wire collected so far is discarded for this start edge.
bool aIsAbandonedSplit = false;
for (j = 2; j <= sewd->NbEdges() && IsConnectedEdge; j++)
{
TopoDS_Edge E2;
@@ -294,9 +297,9 @@ static bool SplitWire(const TopoDS_Face& face,
occ::handle<Geom2d_Curve> curve2 = BRep_Tool::CurveOnSurface(E2, face, a2, b2);
if (curve1.IsNull() || curve2.IsNull())
{
continue;
aIsAbandonedSplit = true;
break;
}
gp_Pnt2d v0, v1;
if (E1.Orientation() == TopAbs_REVERSED)
{
@@ -319,6 +322,10 @@ static bool SplitWire(const TopoDS_Face& face,
}
}
}
if (aIsAbandonedSplit)
{
continue;
}
if (!IsConnectedEdge)
{
// create new notclosed wire
@@ -1842,7 +1842,6 @@ 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<TopoDS_Shape> anAddedWires;
NCollection_Sequence<TopoDS_Shape> SeqWir;
NCollection_Sequence<TopoDS_Shape> SeqNMShapes;
for (TopoDS_Iterator iter(SF, false); iter.More(); iter.Next())
@@ -1855,11 +1854,7 @@ bool ShapeFix_IntersectionTool::FixIntersectingWires(TopoDS_Face& face) const
continue;
}
TopoDS_Wire wire = TopoDS::Wire(iter.Value());
// Prevent duplicate wires from being added to the sequence
if (anAddedWires.Add(wire))
{
SeqWir.Append(wire);
}
SeqWir.Append(wire);
}
if (SeqWir.Length() < 2)
{
@@ -562,16 +562,31 @@ bool ShapeFix_Wire::FixConnected(const double prec)
return false;
}
int stop = (myClosedMode ? 0 : 1);
for (int i = NbEdges(); i > stop; i--)
const int aStop = (myClosedMode ? 0 : 1);
occ::handle<ShapeExtend_WireData> aWireSBWD = WireData();
for (int aI = NbEdges(); aI > aStop; aI--)
{
// Call without UpdateWire to avoid O(n^2) behavior in the loop
FixConnected(i, prec, false);
FixConnected(aI, prec, false);
myStatusConnected |= myLastFixStatus;
// Refresh the edge that the next iteration will analyze as n1.
if (Context().IsNull() || aI - 1 <= aStop)
{
continue;
}
const int aN1Next = (aI - 1 > 1) ? aI - 2 : aWireSBWD->NbEdges();
if (aN1Next == aI || aN1Next == aI - 1)
{
continue;
}
const TopoDS_Edge aEPrev = aWireSBWD->Edge(aN1Next);
const TopoDS_Shape aRefresh = Context()->Apply(aEPrev);
if (aRefresh.IsNull() || aRefresh.IsSame(aEPrev) || aRefresh.ShapeType() != TopAbs_EDGE)
{
continue;
}
aWireSBWD->Set(TopoDS::Edge(aRefresh), aN1Next);
}
// Update wire once after all connections are fixed
// Using Value() in UpdateWire() prevents edge explosion from replacement chains
if (!Context().IsNull())
{
UpdateWire();
@@ -4280,11 +4295,9 @@ void ShapeFix_Wire::UpdateWire()
occ::handle<ShapeExtend_WireData> sbwd = WireData();
for (int i = 1; i <= sbwd->NbEdges(); i++)
{
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))
TopoDS_Edge E = sbwd->Edge(i);
TopoDS_Shape S = Context()->Apply(E);
if (S == E)
{
continue;
}
@@ -314,17 +314,9 @@ static void RecModif(const TopoDS_Shape&
{
next = S;
}
// Map to prevent infinite loops in case of cyclic replacements.
NCollection_Map<TopoDS_Shape> aVisitedShapes;
do
{
cur = next;
if (!aVisitedShapes.Add(cur))
{
break;
}
if (msgmap.IsBound(cur))
{
const NCollection_List<Message_Msg>& msglist = msgmap.Find(cur);
@@ -200,48 +200,10 @@ 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<occ::handle<TopoDS_TShape>> 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;
}
}
// Cycle handling: cycles in the replacement map (A -> B -> A or longer) are
// accepted at insertion time. The DFS in-flight guard inside Apply()/applyImpl()
// breaks any cycle at traversal time, returning the immediate Value() instead
// of recursing.
myShapeToReplacement.Bind(shape, TReplacement(newshape, theKind));
myNewShapes.Add(newshape);
}
@@ -78,24 +78,30 @@ TEST(BRepTools_ReShapeTest, ValueLeaf_ChainEndingInRemoveReturnsNull)
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)
// A direct A->B then B->A creates a cycle in the replacement map; both bindings
// are recorded, and the DFS in-flight guard inside Apply() handles termination.
TEST(BRepTools_ReShapeTest, Replace_DirectCycleHandledByApply)
{
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
aReShape.Replace(aB, aA);
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";
EXPECT_TRUE(aReShape.Value(aA).IsSame(aB));
EXPECT_TRUE(aReShape.Value(aB).IsSame(aA));
// Apply() must terminate (DFS in-flight guard) and return a defined shape.
const TopoDS_Shape aResult = aReShape.Apply(aA);
EXPECT_FALSE(aResult.IsNull()) << "Apply must terminate via the DFS guard";
}
// A longer cycle A -> B -> C -> A must also be rejected at the closing edge.
TEST(BRepTools_ReShapeTest, Replace_RejectsLongerCycle)
// Longer cycles A -> B -> C -> A are accepted at Replace() time and broken at
// traversal time by the DFS in-flight guard inside Apply(). This avoids
// over-rejection in IGES/STEP healing pipelines where shapes legitimately
// alias TShapes across stages.
TEST(BRepTools_ReShapeTest, Replace_LongerCycleHandledByApply)
{
const TopoDS_Vertex aA = MakeVertex(0, 0, 0);
const TopoDS_Vertex aB = MakeVertex(1, 0, 0);
@@ -104,10 +110,15 @@ TEST(BRepTools_ReShapeTest, Replace_RejectsLongerCycle)
BRepTools_ReShape aReShape;
aReShape.Replace(aA, aB);
aReShape.Replace(aB, aC);
aReShape.Replace(aC, aA); // would create A -> B -> C -> A
aReShape.Replace(aC, aA);
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";
EXPECT_TRUE(aReShape.Value(aA).IsSame(aB));
EXPECT_TRUE(aReShape.Value(aB).IsSame(aC));
EXPECT_TRUE(aReShape.Value(aC).IsSame(aA));
// Apply() must terminate (DFS in-flight guard) and return a defined shape.
const TopoDS_Shape aResult = aReShape.Apply(aA);
EXPECT_FALSE(aResult.IsNull()) << "Apply must terminate via the DFS guard";
}
// Apply() must not stack-overflow when a shape's replacement is a compound that
+4 -4
View File
@@ -7,10 +7,10 @@ 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 = 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 )
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 )
TOLERANCE : MaxTol = 0.002562014753 ( 0.00500444492 ) AvgTol = 2.347098051e-05 ( 7.280140961e-05 )
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 )
NCOLORS : NColors = 0 ( 0 )