Modeling Algorithms - Speed up wire classification for many-hole faces#1375
Open
bwross wants to merge 2 commits into
Open
Modeling Algorithms - Speed up wire classification for many-hole faces#1375bwross wants to merge 2 commits into
bwross wants to merge 2 commits into
Conversation
…urves Parameter() always ran through GeomAdaptor_Curve and Extrema_ExtPC, an iterative numerical search, even for curve types where ElCLib already has an O(1) closed-form parameter computation: Line, Circle, Ellipse, Hyperbola, Parabola. That gets expensive when a shape has a lot of circular edges, since BRepCheck_Face::ClassifyWires calls into this once per wire edge to build a pcurve. Added a fast path that unwraps Geom_TrimmedCurve down to its basis curve, computes the closed-form parameter with ElCLib::Parameter, and only accepts it when the result actually falls within the curve's own range, handling periodicity via InPeriod. A trimmed arc whose closest point on the full curve isn't on the trimmed portion still falls through to the existing generic search unchanged. Verified with a small standalone harness covering full and trimmed circles, lines, and the out-of-range trimmed arc case. Combined with the BRepCheck_Face fix in the next commit, building and validating a many-hole face went from 0.57s/2.26s/9.21s at 500/1000/2000 holes down to 0.077s/0.29s/1.11s, about 8x faster at 2000 holes.
bwross
force-pushed
the
perf/geomlib-brepcheck-manyhole-fastpath
branch
2 times, most recently
from
July 19, 2026 02:45
b06b9c9 to
6e1b368
Compare
ClassifyWires() checks every pair of a face's wires against each other, calling IsInside() to classify a representative point of one wire against the other's region. For a face with a lot of wires, like a plate with thousands of holes, this dominates BRepCheck_Analyzer's runtime. IsInside() only needs to classify a single point, so if two wires' 2D bounding boxes don't overlap at all, the answer is already known without running the classifier: the point can't be inside the other wire's finite region. Following the same pattern IntersectWires() already uses above, this precomputes each wire's parametric bounding box once and skips the expensive test whenever the boxes are disjoint. The loop is still a check over every pair of wires, but each pair now costs an O(1) box test instead of a real point classification, which is what actually costs time when the wires are spatially separated. The box for a wire is built per edge from BRep_Tool::CurveOnSurface's trim range, clamped to the underlying curve's own parametric domain (needed to avoid an out-of-domain exception building the box). IsInside() picks its representative point from that same trim range, but without the clamp. For STEP-imported BSpline pcurves whose trim range slightly overshoots the curve's own domain, that mismatch let the box come out tighter than the point IsInside() would actually evaluate, so the disjoint-box fast path could wrongly report "false" for a point that was, in fact, inside. Caught this via a real regression test (bugs/mesh/bug27453) that started reporting a bad shape after this change. Fixed by tracking whether any edge's box needed clamping and, if so, not trusting that wire's box for the pre-filter at all, falling through to the exact classification for any pair involving it. This only gives up the fast path for wires that hit this edge case, which is rare. Ported from a fix originally written against OCCT 7.9.3, updated for the current src/ModelingAlgorithms/TKTopAlgo layout and the current bool/double/occ::handle<> style. The original version also had a bug where it returned WireBienOriente directly on the disjoint-box path, which is only correct when WireBienOriente is false; this port keeps the fixed version, which always returns false on that path regardless. Verified on current master with a harness that builds a plane face with N non-overlapping circular holes and checks it with BRepCheck_Analyzer. With this commit and the GeomLib_Tool one both reverted, the harness measured 0.57s/2.26s/9.21s at 500/1000/2000 holes. With both applied, 0.077s/0.28s/1.12s, still about 8x faster at 2000 holes (the clamp-tracking fix doesn't trigger for these synthetic, non-imported wires). Also verified directly against bugs/mesh/bug27453: a standalone harness reading the test's STEP file and running BRepCheck_Analyzer on face Open-Cascade-SAS#13 reproduced the failure with this commit alone applied (BRepCheck_Analyzer::IsValid() false, matching CI), confirmed GeomLib_Tool was not responsible (still valid with only that commit applied), and confirmed this fix resolves it (valid again with both commits and the fix applied).
bwross
force-pushed
the
perf/geomlib-brepcheck-manyhole-fastpath
branch
from
July 19, 2026 03:42
6e1b368 to
5360886
Compare
dpasukhi
self-requested a review
July 22, 2026 14:12
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
1 participant
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.
ClassifyWires()currently checks every pair of a face's wires against eachother. For a plate with thousands of non-overlapping holes, that nested loop
dominates
BRepCheck_Analyzer's runtime, and each check was going through afull point classification plus an iterative curve-parameter search.
This PR has been split into two commits to make it easier to review. The two
fixes that together make validating a face with many hole wires about 8x
faster:
1176043
GeomLib_Tool::Parameter()now uses ElCLib's closed-formparameter computation for analytic curve types (Line, Circle, Ellipse,
Hyperbola, Parabola) instead of always running the iterative Extrema_ExtPC
search.
BRepCheck_Face::ClassifyWirescalls this once per wire edge to builda pcurve, so it adds up fast on a face with a lot of circular holes.
b06b9c9
BRepCheck_Face::ClassifyWires()now precomputes a 2D boundingbox per wire and skips the exact point-classification test whenever two
wires' boxes don't overlap, since a point outside a wire's box can't be
inside its finite bounded region. This is the same pre-filter pattern
IntersectWires()already uses above it in the same file.Verified against current master with a standalone test file.
Timing at 500/1000/2000 holes: 0.57s/2.26s/9.21s before, down to
0.077s/0.29s/1.11s with both fixes applied. It's about 8x faster at 2000
holes.
CLA ID is 1145.