fix(algo): adopt existing boundary vertices as seams for closed section curves#755
Conversation
Wires assembled by the boolean engine store edges in loop order but with arbitrary per-edge orientation flags; face_polygon trusted the flags and produced folded polygons (a 4x2 rectangle integrated as area 4). Polygons are now chained by vertex connectivity. Planar faces now subtract inner-wire (hole) contributions instead of over-integrating the hole region. Closed boundary circles are sampled starting at their seam vertex in the traversal direction, so periodic-face UV trimming polygons close into the seam corners instead of cutting diagonals across the domain.
…on curves A closed section circle (e.g. the wall-plane intersection of a cylinder whose caps are coplanar with the other solid's faces) previously got a fresh seam vertex at an arbitrary angle. The coincident existing boundary circle keeps its own seam, so link_existing's endpoint-pair key never matched, duplicate coincident circle edges survived, edge-set SD pairing found nothing, and the result was non-manifold (3-face circle edges, orphan cap disc) with fall-through to the mesh fallback. - phase_ff: before allocating a seam vertex for a closed section circle, scan both faces' boundary vertices for one lying on the curve, adopt it and re-anchor t_range there. Gated to circles that do not properly cross either face's boundary (line-segment and coplanar circle-circle checks) so partially-overlapping rims keep the legacy fresh-seam path. - link_existing: secondary index for full closed circle blocks keyed by quantized center/radius/axis (up to sign) so geometric matching links coincident full circles even when seam adoption misses. - fill_images_faces: skip closed section curves that coincide with one of the face's own closed boundary edges — they lie entirely on the boundary and would corrupt the wire if fed to the splitter. - builder: interior-point sampling for unsplit periodic faces bounded by closed curves uses the closed edge's u at the v-range midpoint. - regression test: box(4,4,2) minus r=0.3 cylinder with caps coplanar at z=0/z=2 asserts closed manifold, Euler 2, no coincident full-circle edges, and volume within 0.1% of the analytic value.
The relative compound-vs-sequential oracle was vacuous: both paths agreed at ~27% below the true volume because cuts after the first fall back to the mesh path. The oracle now checks both paths against the analytic volume; the test stays ignored with the actual remaining gap documented (face splitter drops faces carrying pre-existing internal hole loops when a new coplanar closed section arrives).
WASM Binary Size
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a class of boolean-operation failures where closed section circles (notably coplanar cylinder caps) were created with arbitrary seam vertices and therefore failed to link against coincident existing boundary circles, leading to duplicate coincident edges, missed same-domain pairing, and invalid/non-manifold topology (often falling back to mesh boolean).
Changes:
- Adopt existing boundary vertices as seam vertices for eligible closed section circles (and re-anchor the parametric range), plus add a geometry-based fallback index to link coincident full-circle blocks even when seam vertices differ.
- Avoid corrupting face splitting by skipping closed sections that coincide with a face’s own closed boundary edge; improve interior-point sampling for periodic faces bounded by closed curves.
- Strengthen/extend topology and volume validation: new coplanar-cap regression test, improved planar face integration (subtract inner wires), and tighter absolute-volume oracles in ignored compound-cut tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/operations/src/boolean/tests.rs | Tightens an ignored compound-cut test oracle to absolute expected volume and documents the remaining known gap. |
| crates/check/src/util.rs | Adds wire_polygon and improves polygon construction by chaining edges via vertex connectivity; improves closed-edge sampling anchored at seam vertices. |
| crates/check/src/properties/face_integrator.rs | Improves periodic UV bounds handling and subtracts inner-wire (hole) contributions for planar faces. |
| crates/algo/src/pave_filler/tests.rs | Adds regression test for coplanar-cap cylinder cut validity (manifold + Euler + volume). |
| crates/algo/src/pave_filler/phase_ff.rs | Implements seam adoption for eligible closed section circles using existing boundary vertices; adds boundary-crossing gating. |
| crates/algo/src/pave_filler/link_existing.rs | Adds a secondary closed-circle geometric index and refactors linking into try_link. |
| crates/algo/src/builder/mod.rs | Adjusts face interior sampling for periodic/non-planar faces bounded by closed curves. |
| crates/algo/src/builder/fill_images_faces.rs | Skips closed section curves that coincide with a face’s own closed boundary edge to prevent wire corruption. |
| crates/algo/Cargo.toml | Adds brepkit-check as an algo dev-dependency for property-based verification in tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if periodic_u && u_max - u_min < 1e-9 { | ||
| u_max = u_min + std::f64::consts::TAU; | ||
| } |
| if periodic_v && v_max - v_min < 1e-9 { | ||
| v_max = v_min + std::f64::consts::TAU; | ||
| } |
| let ellipses_match = |a: &brepkit_math::curves::Ellipse3D, | ||
| b: &brepkit_math::curves::Ellipse3D| { | ||
| (a.center() - b.center()).length() < tol | ||
| && (a.semi_major() - b.semi_major()).abs() < tol | ||
| && (a.semi_minor() - b.semi_minor()).abs() < tol | ||
| && a.normal().dot(b.normal()).abs() > 1.0 - 1e-9 | ||
| }; |
| } | ||
| } | ||
|
|
||
| let expected_vol = 32.0 - std::f64::consts::PI * 0.09 * 2.0; |
Greptile SummaryThree coordinated algo fixes eliminate the duplicate-coincident-circle defect for coplanar-cap boolean operations:
Confidence Score: 5/5Safe to merge; the three-way seam fix is load-tested by selective revert, full suite green, and the new volume test anchors correctness to 0.1%. All algo and check logic paths are carefully gated: the seam-adoption path fires only when No files require special attention; the phase_ff and face_integrator changes each have focused scope, and the new test exercises the critical coplanar-cap path end-to-end. Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
crates/check/src/properties/face_integrator.rs:226-231
**Hardcoded threshold may incorrectly expand a genuinely zero-span periodic UV region**
`u_max - u_min < 1e-9` fires whenever all sampled UV points are within 1 nm of each other in parameter space on a periodic axis — correct for the intended "all vertices on the seam" case. However, a legitimately degenerate face (e.g. a cylinder sliver where the topology is valid but the geometry has zero angular extent) would also trigger this branch and silently expand its integration domain to the full `[0, 2π]` period, producing an incorrect area/volume contribution. `face_uv_bounds` has no `Tolerance` parameter to derive a model-relative threshold; a follow-up to thread `tol` through here would let the threshold scale with the model.
Reviews (2): Last reviewed commit: "test(algo): derive coplanar-cap cut expe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let start_vid = edge.start(); | ||
| let end_vid = edge.end(); | ||
| let forward = match prev_end { | ||
| Some(pe) if start_vid == pe && end_vid != pe => true, | ||
| Some(pe) if end_vid == pe && start_vid != pe => false, | ||
| _ => oe.is_forward(), | ||
| }; |
There was a problem hiding this comment.
The new wire_polygon derives start_vid = edge.start() / end_vid = edge.end() directly inside the wire iteration and uses them instead of oe.oriented_start(edge) / oe.oriented_end(edge). The check-crate rule flags exactly this pattern and points to util.rs::face_polygon as the reference. The PR intentionally replaces orientation-flag traversal with connectivity to fix the wire-chaining bug (orientation flags not chaining head-to-tail), which is a reasonable correctness fix. However, the stated rule is now violated and the rule should be updated to reflect the new canonical pattern if this approach is accepted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/check/src/util.rs
Line: 105-111
Comment:
**Connectivity-based traversal deviates from the crate's oriented-edge rule**
The new `wire_polygon` derives `start_vid = edge.start()` / `end_vid = edge.end()` directly inside the wire iteration and uses them instead of `oe.oriented_start(edge)` / `oe.oriented_end(edge)`. The check-crate rule flags exactly this pattern and points to `util.rs::face_polygon` as the reference. The PR intentionally replaces orientation-flag traversal with connectivity to fix the wire-chaining bug (orientation flags not chaining head-to-tail), which is a reasonable correctness fix. However, the stated rule is now violated and the rule should be updated to reflect the new canonical pattern if this approach is accepted.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Intentional: on boolean-output faces the stored edge orientation flags are exactly what's unreliable here (that mis-trust was one of the three integration bugs this PR fixes), so wire_polygon chains by vertex connectivity instead. Falls back to the original ordering when chaining fails.
| @@ -180,6 +220,60 @@ pub fn perform(topo: &Topology, tol: Tolerance, arena: &mut GfaArena) -> Result< | |||
| Ok(()) | |||
| } | |||
There was a problem hiding this comment.
Circle only, not Ellipse
The closed_index is built only for EdgeCurve::Circle boundary pave blocks and the fallback lookup likewise only matches EdgeCurve::Circle section curves. If an elliptical closed section (e.g. from a conic intersection) shares a seam mismatch with a boundary ellipse, the same duplicate-coincident-edge problem would recur. The PR description calls out circles as the target (cylinder caps), so this is a known scope, but it is worth a follow-up ticket before elliptic cases are exercised.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/algo/src/pave_filler/link_existing.rs
Line: 200-221
Comment:
**Secondary closed index covers `Circle` only, not `Ellipse`**
The `closed_index` is built only for `EdgeCurve::Circle` boundary pave blocks and the fallback lookup likewise only matches `EdgeCurve::Circle` section curves. If an elliptical closed section (e.g. from a conic intersection) shares a seam mismatch with a boundary ellipse, the same duplicate-coincident-edge problem would recur. The PR description calls out circles as the target (cylinder caps), so this is a known scope, but it is worth a follow-up ticket before elliptic cases are exercised.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed, scoped to Circle deliberately — the seam-adoption gate this index backs up only fires for closed circular sections today. Extending the index to Ellipse is queued as a follow-up alongside the ellipse-section work.
When all boundary vertices of a periodic face collapse onto the seam, extend the span by the surface's parameter-domain period rather than a hard-coded TAU, so periodic NURBS surfaces whose period is not 2pi integrate correctly.
ellipses_match accepted concentric same-size ellipses rotated about the normal as coincident. Compare the major-axis direction up to sign and skip the check for circles, where the direction is undefined.
Compute the expected hole volume from the radius and height used to build the cylinder so the oracle cannot desync from the geometry.
Sequential coplanar-cap cuts and cylinder-cylinder fuses regressed when PR #755 (closed-section seam adoption) and PR #756 (periodic band splitter) landed together. Three coordinated fixes in the GFA builder: - same_domain: sample wire edges along their curves instead of taking only the start vertex, so a circular hole left by an earlier cut is visible to the hole-containment test. A single-edge circle wire collapsed to one point and the test treated the hole as absent, cancelling the wall face through the hole. - fill_images_faces: a flush coplanar cap disc (outer wire = one closed circle) no longer absorbs spurious IN-edge arcs projected from the wall outline. Those fragmented the clean disc into a many-sided polygon that escaped edge-set same-domain pairing and survived the cut. - fill_images_faces: the self-boundary coincidence skip only fires when the partner FF face hosts the full circle within its extent. A cylinder rim where a narrower cylinder's cap plane slices the lateral must still split that lateral; skipping it produced an invalid fuse that wrongly passed validation. Re-enables compound_cut_matches_sequential_2x2_grid and updates the box-minus-cylinder golden (fewer duplicate cap vertices).
🤖 I have created a release *beep* *boop* --- ## [2.102.0](v2.101.3...v2.102.0) (2026-06-09) ### Features * **algo:** split u-periodic faces into bands at internal section circles ([#756](#756)) ([39e9425](39e9425)) ### Bug Fixes * **algo:** adopt existing boundary vertices as seams for closed section curves ([#755](#755)) ([3342271](3342271)) * **algo:** trim plane-plane section curves to mutual face overlap ([#754](#754)) ([e692c9c](e692c9c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: brepkit[bot] <265643962+brepkit[bot]@users.noreply.github.com>
Closed section circles (e.g. a cylinder cap coplanar with a box face) were created with a fresh seam vertex at an arbitrary angle.
link_existingmatches section-to-boundary pave blocks by quantized endpoint-pair key, so the new circle never linked with the coincident existing boundary circle: duplicate coincident edges survived, same-domain detection found zero pairs, and the result failed validation (orphan cap disc, 3-face edges, Euler 5) and fell through to the mesh fallback.Changes
phase_ff: when a closed section curve carries an existing boundary vertex on it (within tolerance), adopt that vertex as the seam and re-anchort_rangethere — gated to circles that do not properly cross either face's boundary (the un-gated version regressedfuse_two_cylinders).link_existing: secondary geometric index for full closed circle blocks keyed by quantized center/radius/axis-up-to-sign.fill_images_faces: skip closed sections coincident with a face's own closed boundary edge; builder interior-point sampling handles unsplit periodic faces bounded by closed curves.brepkit-checkface-integration bugs the new absolute-volume assertion exposed: wire chaining by connectivity instead of orientation flags, planar inner-wire hole subtraction, seam-aligned closed-edge sampling.gfa_cut_box_cylinder_coplanar_caps_produces_valid_topology(closed manifold, exact volume); each algo change verified load-bearing by selective revert.compound_cut_matches_sequential_2x2_grid: oracle tightened to absolute volume (the old two-path relative check passed on garbage); stays ignored — cuts 2–4 still drop faces carrying pre-existing hole loops (the band-splitter PR addresses that path).Verification
Full workspace suite green; algo 95, check 44, operations 629 lib; fmt, clippy
-D warnings, boundary check (check added as algo dev-dependency only).