fix(algo): merge overlapping deepened wall openings in the internal-loops splitter#1104
Merged
Conversation
…oops splitter A later cut can re-open a wall area an earlier cut already opened, offset slightly (the snapClip slot stack's 0.01 ledge margins). The cutter's sections form a closed internal loop on the wall face that overlaps the existing inner wire; the internal-loops splitter attached the loop as an independent second hole, double-covering the overlap band — both rims stayed unpaired, the collinear band pieces traced twice, the analytic gate rejected the cut, and the whole 44-hole snapClip join-edges chain fell back to mesh booleans (F=615 -> 8203 at op-cut-3, export nm=5). split_face_with_internal_loops now merges each overlapping pair: the frame keeps ONE union outline and the removable disc is bounded by the loop pieces outside the hole plus the hole pieces inside the loop. Gated to plane faces with all-Line loop+hole; every 2D test projects 3D endpoints through one locally built frame (stored hole-wire UVs can be fitted in a foreign frame); any chain failure bails to the previous behavior. The full snapClip join-edges chain now replays analytic and watertight natively (F=881 posBad=0, was F=8207 bnd=86).
Contributor
WASM Binary Size
|
There was a problem hiding this comment.
Pull request overview
Fixes a planar-face edge case in brepkit-algo’s internal-loops face splitter where a later cut’s internal section loop can overlap an existing inner wire (deepened/re-opened opening), previously producing double-covered rims, unpaired edges, and triggering mesh-fallback booleans downstream. Adds an in-memory IO fixture to lock the regression and updates the roadmap notes.
Changes:
- Add a planar “deepened-opening union” pre-pass in
split_face_with_internal_loopsto merge an overlapping internal loop with an existing hole wire into a single union opening (with a merged removable disc outline). - Add
deepened_wall_opening_inmemintegration test to verify watertight edge pairing, single merged wall hole, and expected volume. - Update roadmap documentation to mark the deepened-notch plane-face variant as closed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/io/tests/deepened_wall_opening_inmem.rs |
New regression fixture validating the deepened wall-opening union behavior (watertightness, single wall hole, volume). |
crates/algo/src/builder/face_splitter/special_cases.rs |
Implements the planar overlap-detection + union/chain logic inside split_face_with_internal_loops. |
.claude/skills/roadmap/SKILL.md |
Documents closure of the plane-face deepened-opening case and updates remaining work items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+96
to
+99
| let e = topo.edge(oe.edge()).unwrap(); | ||
| (topo.vertex(e.start()).unwrap().point().x() - 10.0).abs() < 1e-9 | ||
| && (topo.vertex(e.end()).unwrap().point().x() - 10.0).abs() < 1e-9 | ||
| }); |
Comment on lines
+1667
to
+1670
| let lp: Vec<Point2> = ls.iter().map(|s| s.0).collect(); | ||
| let hp: Vec<Point2> = hs.iter().map(|s| s.0).collect(); | ||
| let eps = 1e-5; | ||
| for &(a0, a1) in &ls { |
Comment on lines
+1747
to
+1751
| Out, | ||
| } | ||
| let eps = 1e-5; | ||
| let seg_of = |e: &OrientedPCurveEdge| (frame.project(e.start_3d), frame.project(e.end_3d)); | ||
| let lsegs: Vec<(Point2, Point2)> = loop_edges.iter().map(seg_of).collect(); |
Greptile SummaryThis PR fixes a non-manifold STL export failure in the snapClip join-edges chain by adding a union pre-pass in
Confidence Score: 5/5Safe to merge — the union pre-pass is correctly gated to all-Line plane faces and bails cleanly to the pre-existing independent-wires path on any chaining failure, so no new failure mode is introduced. All three new helpers carry explicit bail-to-None guards; the 3D-endpoint chain-closure check in No files require special attention; both P2 observations are edge cases beyond the target rectangular-overlap shape and are safely covered by the existing bail path. Important Files Changed
Reviews (2): Last reviewed commit: "fix(algo): derive union pre-pass epsilon..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Address review: the weld-band proximity threshold in the deepened-opening union is now tol*100 instead of a hardcoded 1e-5 (same value, tied to the kernel tolerance), the wire_pts parameter drops its stale underscore now that it drives the PlaneFrame construction, and the fixture's wall-face detection uses a tolerance-derived epsilon.
andymai
enabled auto-merge (squash)
July 17, 2026 15:17
brepkit Bot
added a commit
that referenced
this pull request
Jul 17, 2026
🤖 I have created a release *beep* *boop* --- ## [2.126.14](v2.126.13...v2.126.14) (2026-07-17) ### Bug Fixes * **algo:** merge overlapping deepened wall openings in the internal-loops splitter ([#1104](#1104)) ([ac46c58](ac46c58)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Release 2.126.14 for `brepkit-wasm`, fixing the internal-loops splitter to merge overlapping deepened wall openings. This prevents duplicated openings and keeps wall topology valid. <sup>Written for commit e887026. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/andymai/brepkit/pull/1105?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> Co-authored-by: brepkit[bot] <265643962+brepkit[bot]@users.noreply.github.com>
andymai
added a commit
that referenced
this pull request
Jul 17, 2026
… arcs (#1106) ## Summary Second half of the snapClip join-edges export root (follows #1104). The export baseplate builds with the simplified **tapered** pockets, so each deep relief cutter's back corner lands on a pocket corner **cone**. The cutter should contribute three sections to that cone face: two marched conics (cone × back wall, cone × side wall) and the cone × cutter-top arc that closes the chain. **Root cause.** A horizontal plane cuts a cone in an exact `EdgeCurve::Circle`. The exact-arc path that bypasses the sampling pre-filters (`trim_ellipse_to_boundary_crossings` — built for the tilted-tread × cylinder family) only accepted **Ellipse** sections, so the circle fell to the generic 16-sample in-both AABB filter, which cannot find a ~0.11-long in-both arc on an ~18-long circle. With the closing arc missing, the cone face received an open two-conic chain, the internal-loops splitter rejected it (open chains are dropped by design), and the face stayed unsplit. The resulting analytic-but-leaky solid (posBad=8) was accepted by the by-edge-id gate and poisoned the export chain into mesh fallback two cuts later (final bnd=111 nm=6 at export tolerance). **Fix.** The trimmer now dispatches over Circle and Ellipse sections through one angular parameterization (`SecCurve`); emitted arcs keep their exact curve type so downstream circle-calibrated machinery sees real Circle sections. No other behavior changes — Ellipse handling is byte-identical. **Result.** The minimal repro (`cut(plate-after-op-cut-2, deep-relief-cutter)`) goes posBad 8→0 on both the raw GFA and ops paths, and the full 44-hole export-variant join-edges chain replays analytic and watertight: final **F=418, posBad=0** (was F=4842, bnd=111, nm=6). ## Verification - New fixture `crates/io/tests/snapclip_export_corner_inmem.rs` (tool's serialized operands, 2026-07-17 export-variant capture): edge pairing + analytic-cone assertions, 10× flake gate clean - Full io suite green (including the tilted-tread/halfSockets ramp landscapes that calibrate the ellipse path) - d4 canary 27/27; `brepkit-algo` and `brepkit-operations` suites green - Full workspace suite green via pre-push hook Roadmap updated in the same PR. Known remaining snapClip residuals (out of scope, recorded with repro recipes): the 0.6mm-nozzle export chain's op-cut-3 (posBad=10, different landscape), the by-edge-id acceptance gate's blindness to position-duplicate leaks, and the bed-flat clip volume pin. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes trimming for plane×cone intersections by treating circle sections as exact boundary-crossing arcs. Restores the short cutter-top closing arcs, so cone faces split and the snapClip export chain stays analytic and watertight. - **Bug Fixes** - Updated `trim_ellipse_to_boundary_crossings` to handle both `EdgeCurve::Circle` and `EdgeCurve::Ellipse` via a shared angular parameterization (`SecCurve`); emitted arcs keep their exact type. - Avoids dropping ~0.11-length circle arcs in the 16-sample in-both filter that left open two-conic chains on cone faces. - Added `crates/io/tests/snapclip_export_corner_inmem.rs` with serialized operands to assert analytic cones and B-Rep edge pairing. - Result: minimal repro goes posBad 8→0; full 44-hole export variant replays analytic `F=418, posBad=0` (was `F=4842, bnd=111, nm=6`). <sup>Written for commit ca4ce75. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/andymai/brepkit/pull/1106?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
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
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
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.
Summary
The snapClip join-edges baseplate export (
baseplateGenerator.scenario.snapClip, 5×4 right/front/back join) failed with 5 non-manifold STL edges, and its 44-hole connector chain ran almost entirely on mesh-fallback booleans. Fresh operand capture on 2.126.13 localized the break toop-cut-3: the plate flips from 615 analytic faces to an 8203-face mesh fallback and never recovers.Root cause. The snap-slot cutter stack cuts the same seam wall twice: hole-2 opens a window in the x=105 join wall, and hole-3 (whose top floats 0.01 above hole-2's floor) re-opens an overlapping window in that same wall. Hole-3's sections form a closed internal loop on the wall face that OVERLAPS the existing inner wire.
plane_internal_line_loopsonly tests loops against the OUTER boundary, so the face routed tosplit_face_with_internal_loops, which attached the loop as an independent second hole. The two wires double-cover the 0.01 overlap band: both rims stay as unpaired edges, the collinear band pieces trace twice (use=3 micro verticals), the analytic gate rejects the cut, and every subsequent cut inherits the fallback. This is the plane-face twin of the cone stranded-rim case closed in #1102 — the roadmap's deepened-notch terminal row.Fix. A union pre-pass in
split_face_with_internal_loops, gated to plane faces with all-Line loop+hole: detect genuine geometric overlap (proper crossing, vertex containment, or collinear overlap span), split both edge sets at mutual crossings and vertex T-points, classify pieces by midpoint (In/On/Out), and emit ONE merged opening outline (collinear On-pieces contributed once, from the hole's copy so they pair with the pave-split neighbor faces) plus the removable disc bounded by loop-pieces-outside + hole-pieces-inside. All 2D tests project 3D endpoints through one locally builtPlaneFrame— stored hole-wire UVs can be fitted in a foreign frame (the pcurve-convention lesson). Any chaining failure bails to the previous behavior.Result. The full 44-hole join-edges chain now replays analytic and watertight natively: final F=881 (539 plane / 182 cylinder / 160 cone), position-quantized edge pairing clean — versus F=8207 with 86 boundary edges before. Synthetic 3-box fixture included.
Verification
crates/io/tests/deepened_wall_opening_inmem.rs: stranded-rim pairing, single union hole on the wall, exact volume — 10× flake gate cleancargo test -p brepkit-wasm --lib gridfinity)brepkit-algo157 +brepkit-operations765 green;check-boundaries.shcleanRoadmap updated in the same PR (terminal row retired; nozzle-chain residual re-scoped with its dig recipe).
Summary by cubic
Merges overlapping deepened wall openings in the internal-loops splitter so plane walls keep a single union hole and cuts stay analytic. Fixes the snapClip join-edges export by preventing double-covered rims and mesh fallback.
split_face_with_internal_loopsfor plane faces with all-Line loop+hole: detect overlap (crossing, containment, or collinear span), split at intersections/T-points, classify pieces, and emit one union hole plus the correct removable disc; epsilons derive from kernel tolerance (tol*100), and all 2D tests project 3D endpoints through one localPlaneFramebuilt fromwire_pts; bails to the old path on any failure.crates/io/tests/deepened_wall_opening_inmem.rsto guard the stranded-rim case; updated roadmap notes; all test suites pass.Written for commit c131cf1. Summary will update on new commits.