Skip to content

feat(algo): split u-periodic faces into bands at internal section circles#756

Merged
andymai merged 10 commits into
mainfrom
feat/gfa-periodic-band-split
Jun 9, 2026
Merged

feat(algo): split u-periodic faces into bands at internal section circles#756
andymai merged 10 commits into
mainfrom
feat/gfa-periodic-band-split

Conversation

@andymai

@andymai andymai commented Jun 9, 2026

Copy link
Copy Markdown
Owner

When a tool pierces through a solid (cylinder bore through a slab), the periodic lateral face's internal section circles were interpreted as planar disc+hole loops. None of the emitted sub-faces was the real lateral band: classification kept a phantom membrane disc, the top circle became a free edge (Euler 3), and the operations wrapper fell back to the mesh path — non-manifold, volume-lossy, and run-to-run jittery. This implements the band design from the in-code post-mortem in face_splitter/special_cases.rs, including the seam-anchoring that the reverted prior attempt (#535) lacked.

Changes

  • Seam-anchor pre-pass in fill_images_faces: closed circle FF curves on cylinder/cone laterals are re-parameterized to start at surface.evaluate(seam_u, v_i); anchor vertices pre-registered so lateral band wires and plane-face hole wires resolve to the same VertexId.
  • split_periodic_face_into_bands: emits N+1 band sub-faces sorted by v with on-surface interior points at (seam_u + π, v_mid); boundary-coincident (flush-cap) circles skipped; fully-flush tools bail to legacy paths.
  • Disc path now preserves pre-existing inner wires, so sequential cuts no longer drop earlier holes.
  • classifier/ray_cast: geometrically chained wire polygons, plane-face hole exclusion, analytic ray-cylinder crossing counts for full-period cylindrical faces (the flat-polygon approximation flipped parity for rays through bores).
  • New validate_shell_closed (flags free edges) used by the algo pinning tests; operations wrapper rejects Intersect results with free edges.
  • Un-ignores 11 tests, including both algo pinning tests (single bore: Euler 2 manifold; 4×4 sequential grid: F=22, strict closed manifold), the golden box-minus-cylinder regression (golden regenerated — volume/bbox unchanged, tessellation differs because the lateral is now real cylinder bands), and gridfinity sequential_booleans_volume_accuracy (Solid validation and volume regression on lip/fillet geometry #260).

Six tests stay ignored: their failures are box-box flush-fuse non-manifold cases with no periodic faces involved (separate gap).

Verification

Full workspace green (45 suites, 0 failures); the regression gates that reverted the prior attempt all pass — compound-cut 2×2/3×3/4×4 grids, honeycomb (timed, no hang), shelled-target; fmt, clippy -D warnings, boundary check.

andymai added 5 commits June 9, 2026 12:46
…cles

Closed circle intersection curves on a cylinder/cone lateral do not bound
discs - they separate the periodic surface into stacked bands. The disc
interpretation produced a phantom membrane face and a free top circle
(Euler=3), forcing the operations layer into the mesh-boolean fallback for
every cylinder bore.

- seam-anchor pre-pass: re-parameterize each closed section circle to
  start at the face's seam u, and pre-register the anchor vertex so the
  periodic face's band wires and the plane faces' hole wires share it
- split_periodic_face_into_bands: emit N+1 band sub-faces (lower circle +
  seam segment + upper circle reversed + seam segment reversed), end bands
  reusing the original boundary circles; interior points at seam_u + pi
  are on-surface and strictly off the opposing solid's boundary
- preserve pre-existing inner wires through the internal-loop split path
  and accept single-closed-circle hole wires, so sequential cuts keep the
  holes from earlier operations
The ray-cast classifier flattened every face into a polygon built from
raw edge start vertices, which broke in three ways once boolean results
kept real cylindrical bore faces:

- wires that store edge sets (not traversal order) produced self-crossing
  zigzag polygons, corrupting crossing parity for box walls
- a full-period cylindrical face flattened to a single polygon counts one
  crossing where the real surface has two, flipping parity for any ray
  through the bore
- plane-face holes were ignored, so rays through a bore still counted the
  cap crossings

Polygons are now chained geometrically with dense sampling of closed
curved edges, plane faces exclude hole hits, and full-period cylindrical
faces use an analytic ray-cylinder crossing test bounded by the face's
v-range (tangent grazes count as zero to preserve parity).
…sect results

validate_shell_closed flags free edges (count==1) in addition to
over-shared edges, distinguishing open shells from non-manifold ones.
The existing validate_shell_manifold keeps its lenient contract since
open-shell callers (chamfer/fillet tests, fuse onto open containers)
depend on it.

The boolean wrapper now rejects Intersect results whose outer shell has
free edges: a tolerance-thin sliver can keep only some of its bounding
faces while Euler accidentally balances, slipping garbage past the
euler==2 gate. Cut and Fuse keep the legacy lenient gate - some coplanar
results carry boundary edges yet still beat the mesh fallback on volume.
A tool whose cap is flush with the target face produces a section circle
at the lateral's own boundary circle v. That circle duplicates the
existing boundary edge and must not create a zero-height band; skip it
and split only at strictly interior circles. When every section is
boundary-coincident (fully flush tool), bail to the generic coplanar
paths.
Un-ignored (each verified 3-10x):
- algo: gfa_cut_box_cylinder_through_produces_valid_topology,
  gfa_cut_box_cylinder_grid_through_sequential_produces_valid_topology
  (now asserted with the strict closed-shell validation)
- operations: cut_cylinder_from_box, cut_cylinder_from_box_volume,
  box_cut_cylinder_edge_count, compound_cut_single_tool_matches_boolean,
  compound_cut_two_disjoint_cylinders, sequential_cut_preserves_surface_types,
  cut_box_cylinder_volume_decreases, golden_boolean_box_minus_cylinder
- wasm: sequential_booleans_volume_accuracy

The box-minus-cylinder golden was regenerated per the documented
UPDATE_GOLDEN procedure: volume/bbox/center-of-mass are unchanged
(volume 717.762071); vertex/triangle counts changed because the result
is now a true B-rep bore instead of mesh-fallback output.
Copilot AI review requested due to automatic review settings June 9, 2026 20:21
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

WASM Binary Size

File main PR Delta
brepkit_wasm.wasm 4.72 MB 4.75 MB +35.1 KB (+.7%)

⚠️ Size increased slightly.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the “cylinder/cone lateral band” design for u-periodic faces so closed section circles no longer get misinterpreted as planar disc loops, which previously produced phantom membrane faces, free edges, and unstable mesh-fallback results in boolean operations involving bores through solids.

Changes:

  • Adds a seam-anchoring pre-pass for full-circle intersection curves on periodic laterals and a new split_periodic_face_into_bands path that emits stacked lateral band sub-faces between consecutive section circles.
  • Improves point-in-solid classification via ray casting by (a) chaining wire edges geometrically, (b) excluding planar holes from crossings, and (c) using analytic ray–cylinder crossing counts for full-period cylindrical faces.
  • Tightens validation/gating for boolean results by introducing validate_shell_closed (rejects free edges) and using it in pinning tests plus an Intersect-specific “no free edges” gate; un-ignores multiple regression tests and updates the relevant golden output.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/golden/data/boolean_box_minus_cylinder.golden Updates golden output reflecting new lateral band topology/tessellation while keeping volume/bbox stable.
crates/wasm/src/bindings/gridfinity_tests.rs Un-ignores volume accuracy regression test tied to boolean robustness.
crates/topology/src/validation.rs Adds validate_shell_closed and tests for rejecting free edges / accepting closed two-sided shells.
crates/operations/tests/golden_regression.rs Un-ignores golden regression for box-minus-cylinder boolean case.
crates/operations/tests/boolean_stress.rs Un-ignores stress tests for cylinder cut scenarios.
crates/operations/src/measure/mod.rs Un-ignores volume regression test covering cylinder band classification.
crates/operations/src/boolean/tests.rs Un-ignores multiple boolean regression tests previously blocked by periodic-face issues.
crates/operations/src/boolean/mod.rs Adds Intersect-specific rejection of free-edge results via has_free_edges.
crates/algo/src/pave_filler/tests.rs Switches pinning tests to strict closed-shell validation and un-ignores them.
crates/algo/src/classifier/ray_cast.rs Refactors ray-cast geometry collection and adds analytic full-period cylinder crossing handling + hole exclusion.
crates/algo/src/builder/fill_images_faces.rs Adds seam-anchor pre-pass and re-parameterizes closed section circles to start on the periodic seam.
crates/algo/src/builder/face_splitter/special_cases.rs Introduces split_periodic_face_into_bands and preserves pre-existing inner wires in the disc-loop fallback.
crates/algo/src/builder/face_splitter/mod.rs Routes periodic faces through the band splitter and improves inner-wire handling for closed curved holes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR implements the band-split fix for u-periodic faces (cylinder/cone laterals) cut by closed section circles — the root cause of the phantom-membrane disc topology, Euler-3 non-manifold results, and GFA mesh-boolean fallback tracked since PR #534.

  • Seam-anchor pre-pass (fill_images_faces): full-circle FF curves on cylinder/cone faces are re-parameterized to start at the surface seam, with seam-point vertices pre-registered so cross-face wire sharing works without position-fallback duplication.
  • split_periodic_face_into_bands (special_cases.rs): emits N+1 stacked band sub-faces with (seam_u + π) rem 2π interior points; boundary-flush circles skipped, fully-flush tools fall through to legacy paths.
  • Analytic ray-cylinder classifier (ray_cast.rs): replaces the flat-polygon crossing count (parity-flip for full-period bores) with a quadratic solve; cylinder_hole_bands is intended to handle flush-cap inner-wire holes but is currently unreachable due to a sampling-threshold mismatch.
  • Free-edge guard (boolean/mod.rs): Intersect results with free edges are rejected; 11 previously-ignored tests now pass.

Confidence Score: 5/5

Safe to merge — all 45 test suites pass and the regression gates from the reverted PR #535 are green.

The core algorithm is well-tested end-to-end by the pinning tests and golden regression. The three findings are non-blocking style or latent correctness concerns that do not affect correctness on the enabled test matrix.

crates/algo/src/classifier/ray_cast.rs — the cylinder_hole_bands threshold deserves a follow-up before any flush-cap inner-wire scenario is enabled.

Important Files Changed

Filename Overview
crates/algo/src/builder/face_splitter/special_cases.rs Replaces the long-standing known-gap comment with the real split_periodic_face_into_bands implementation; one new _ => wildcard on an EdgeCurve tuple match violates the wildcard-match-arms rule.
crates/algo/src/builder/fill_images_faces.rs Adds seam-anchor pre-pass and cap-disc IN-edge filtering; hardcoded tolerances already flagged in prior thread.
crates/algo/src/classifier/ray_cast.rs Adds analytic ray-cylinder crossing and cylinder_hole_bands helper; the wraps_full u-span threshold is unreachable with 16-sample circle polygons, making hole-band detection dead code.
crates/algo/src/builder/face_splitter/mod.rs Routes u-periodic non-plane faces with no pre-existing inner wires into the band path; correctly preserves pre-existing inner wires for the disc/loop path.
crates/algo/src/builder/same_domain.rs Replaces vertex-only polygon sampling with 8-samples-per-edge curve sampling, fixing silent hole-cancellation for single-circle inner wires.
crates/topology/src/validation.rs Adds validate_shell_closed with two well-targeted unit tests.
crates/operations/src/boolean/mod.rs Adds has_free_edges check gated on BooleanOp::Intersect; Cut/Fuse intentionally keep the lenient gate.
crates/algo/src/pave_filler/tests.rs Un-ignores two pinning tests; new test comment mis-states genus-2 for an Euler=2 BRep result.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/algo/src/builder/face_splitter/special_cases.rs:200-204
New `_ =>` wildcard on an `EdgeCurve`-containing tuple match — per the `wildcard-match-arms` rule, catch-all arms on `EdgeCurve` patterns should be avoided so the compiler surfaces any new variants. If a new `EdgeCurve` variant is ever added, this arm silently routes it to `return None` instead of generating a compile error. The intended semantics can be expressed exhaustively without a blanket wildcard.

```suggestion
        match (&e.curve_3d, is_closed) {
            (EdgeCurve::Circle(_), true) => boundary_circles.push(e),
            (EdgeCurve::Line, false) => seam_edges.push(e),
            // Any other combination (open circle, closed line, NURBS, etc.)
            // means this face's boundary isn't a standard seam+circles layout.
            (EdgeCurve::Circle(_), false)
            | (EdgeCurve::Line, true)
            | (EdgeCurve::Nurbs(_), _)
            | (EdgeCurve::Ellipse(_), _) => return None,
        }
```

### Issue 2 of 3
crates/algo/src/classifier/ray_cast.rs:301-306
**`wraps_full` threshold unreachable with 16-sample circles**

`wire_polygon` generates `n_samples = 16` interior samples at fractions `k/(n+1)` for `k = 1..=16`. The highest-u sample lands at `u ≈ TAU × 16/17 ≈ 5.91`, so `u_max − u_min ≈ 5.91 < TAU − 1e-3 ≈ 6.282` — the gap is ~0.37 rad, about 370× larger than the tolerance. `wraps_full` is always `false` for any seam-anchored full-circle inner wire, making `cylinder_hole_bands` always return an empty `Vec` and the `FaceGeom::Cylinder { hole_bands }` non-empty path dead code. A viable fix is to threshold at `TAU * (n_samples as f64 / (n_samples + 1) as f64) - epsilon`, or to check directly whether the inner wire contains a single closed circle edge rather than measuring the u-span of a polygon approximation.

### Issue 3 of 3
crates/algo/src/pave_filler/tests.rs:1532-1535
The error message says "closed genus-2 manifold" but the codebase convention is that `V − E + F = 2` is the closed-manifold BRep invariant for all through-bore results regardless of topological genus — the grid-of-16 test explicitly calls this "genus 0". A box with 2 through bores has topological genus 2 (χ = −2), but the BRep Euler count hits 2 because inner-wire loops cancel the genus terms.

```suggestion
    assert_eq!(
        euler, 2,
        "Euler V-E+F should be 2 for a closed manifold (BRep convention: inner loops \
         cancel genus terms), got V={v} E={e} F={f}"
    );
```

Reviews (3): Last reviewed commit: "fix(algo): coordinate seam adoption with..." | Re-trigger Greptile

Comment thread crates/algo/src/builder/face_splitter/special_cases.rs Outdated
Comment thread crates/algo/src/builder/fill_images_faces.rs Outdated
Comment thread crates/algo/src/classifier/ray_cast.rs Outdated
andymai added 3 commits June 9, 2026 13:43
The periodic band splitter evaluated the band interior at seam_u + pi
without wrapping. For a seam u in (pi, 2pi) the parameter exceeds 2pi,
which can return None for surfaces that reject out-of-domain u and bail
the whole band split via the ? operator. Wrap with rem_euclid(TAU) to
match the antipode handling already used elsewhere in the band path.
Hoist the hardcoded full-circle, degenerate-seam, and on-circle
tolerances in the seam-anchor pre-pass into named constants. The
on-circle radial tolerance stays at 1e-6 (looser than the linear
default) because the anchor is built from two project_point + evaluate
round-trips whose float error accumulates.
The analytic ray-cylinder crossing path was gated on the face having no
inner wires, so a full-period cylindrical face carrying a hole fell back
to the flat-polygon parity approximation that this path exists to avoid.
Extend the analytic path to faces whose inner wires are
full-circumference v-band holes: count analytic lateral crossings, then
exclude any crossing whose axial parameter lands inside a hole band.
Non-banded (partial-arc) holes still force the polygon fallback.
@andymai andymai enabled auto-merge (squash) June 9, 2026 20:45
andymai added 2 commits June 9, 2026 13:47
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).
@andymai andymai merged commit 39e9425 into main Jun 9, 2026
18 checks passed
@andymai andymai deleted the feat/gfa-periodic-band-split branch June 9, 2026 21:44
@brepkit brepkit Bot mentioned this pull request Jun 9, 2026
brepkit Bot added a commit that referenced this pull request Jun 9, 2026
🤖 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants