Shape Headling - Crash and Stack with Moebius#998
Open
dpasukhi wants to merge 8 commits intoOpen-Cascade-SAS:IRfrom
Open
Shape Headling - Crash and Stack with Moebius#998dpasukhi wants to merge 8 commits intoOpen-Cascade-SAS:IRfrom
dpasukhi wants to merge 8 commits intoOpen-Cascade-SAS:IRfrom
Conversation
Modifications in historical order: /Users/dpasukhi/work/OCCT/src/ModelingAlgorithms/TKShHealing/GTests/ShapeUpgrade_UnifySameDomain_Test.cxx.cxx, BasicTest() - entry point ShapeBuild_ReShape - added method ApplyImpl(). This is to prevent initial stackoverflow. Same implementation as original Apply() but accepts map of visited shapes. If shape is found in the map, doesn't perform the recursive call. ShapeProcess_ShapeContext.cxx, function RecModif() Added map of visited shapes to prevent infinite loop in do/while cycle. It exits the loop when encounters already visited shape. ShapeExtend_WireData - added std::unordered set that duplicates the content of mySeams. This is to prevent quase-infinite loop when calling ShapeExtend_WireData::IsSeam(const int num). It was simply searching for the 'num' in the list mySeams that took enormous amount of time. ShapeFix_Edge::FixVertexTolerance(const TopoDS_Edge& edge, const TopoDS_Face& face) ShapeFix_Edge::FixVertexTolerance(const TopoDS_Edge& edge) Added check that object returned by Context()->Apply(edge) is actually an edge. Due to presumably incorrect previous processing it could return Wire which caused a crush. ShapeFix_Wire, ShapeFix_Face Latest attempts to add Message_ProgressRange. Didn't work for some reason. Current state: Stuck in quase-infinite (so long so it might as well be considered infinite) loop inside ShapeFix_Wire::Perform() trying to fix the wire with more then 1'000'000 edges.
…hape processing and connection handling
…fInteger for improved performance
dpasukhi
added
the
1. Shape Healing
There was a problem hiding this comment.
Pull request overview
This PR addresses crashes and stack overflow issues when processing shapes with non-orientable surfaces like Moebius strips. The changes prevent infinite loops from cyclic replacements and exponential edge growth from shared context.
Changes:
- Added cycle detection in shape replacement chains to prevent infinite loops
- Optimized wire update logic to prevent O(n²) behavior and edge explosion
- Added progress tracking support to enable cancellation of long-running operations
- Improved performance of seam detection with hash-based lookups
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ShapeProcess_ShapeContext.cxx | Adds visited shapes tracking to prevent infinite loops in replacement chains |
| ShapeFix_Wire.hxx | Adds progress tracking parameter and new wire update control parameter |
| ShapeFix_Wire.cxx | Implements progress checks, optimizes UpdateWire to use Value() instead of Apply(), and prevents duplicate edge expansion |
| ShapeFix_IntersectionTool.cxx | Prevents duplicate wires from being added to processing sequence |
| ShapeFix_Face.hxx | Adds progress tracking parameter |
| ShapeFix_Face.cxx | Propagates progress tracking to wire fixing |
| ShapeFix_Edge.cxx | Adds validation for shape type after context application |
| ShapeExtend_WireData.hxx | Adds cache for seam edge lookups |
| ShapeExtend_WireData.cxx | Implements O(1) seam lookup using hash set |
| ShapeBuild_ReShape.cxx | Refactors Apply() to use helper structure that defers replacement registration until after recursion completes |
| { | ||
| FixConnected(i, prec); | ||
| // Call without UpdateWire to avoid O(n^2) behavior in the loop | ||
| FixConnected(i, prec, false); |
There was a problem hiding this comment.
Corrected spelling of 'Headling' to 'Healing' in PR title.
Suggested change
| FixConnected(i, prec, false); | |
| (void)FixConnected(i, prec, false); |
Comment on lines
+54
to
+57
| //! Recursively applies substitutions to a shape. | ||
| //! @param theShape the shape to process | ||
| //! @param theUntil the shape type at which to stop recursion | ||
| //! @return the processed shape (original or replacement) |
There was a problem hiding this comment.
The comment refers to 'theUntil' but the parameter is named 'theUntil' with capital U. The documentation should match the actual parameter name for consistency.
Suggested change
| //! Recursively applies substitutions to a shape. | |
| //! @param theShape the shape to process | |
| //! @param theUntil the shape type at which to stop recursion | |
| //! @return the processed shape (original or replacement) | |
| //! @param theShape the shape to process | |
| //! @param theUntil the shape type at which to stop recursion | |
| //! @return the processed shape (original or replacement) | |
| //! @return the processed shape (original or replacement) |
Comment on lines
+28
to
+29
| #include <TColStd_PackedMapOfInteger.hxx> | ||
|
|
There was a problem hiding this comment.
The blank line after the include directive is inconsistent with the surrounding include block style. Consider removing the extra blank line for consistency with the rest of the header.
AtheneNoctuaPt
approved these changes
Jan 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.