Skip to content

fix(booleans): closed-loop booleans on spheres, cones and cylinders that came out inside-out or wrong - #41

Open
Basnederveen wants to merge 1 commit into
OpenGeometry-io:mainfrom
Bas-Automation-Services:fix/closed-loop-hole-booleans
Open

Basnederveen wants to merge 1 commit into
OpenGeometry-io:mainfrom
Bas-Automation-Services:fix/closed-loop-hole-booleans

Conversation

@Basnederveen

Copy link
Copy Markdown

What

Four bugs in the closed-loop Booleans (one closed intersection loop, a Patch on one side and the face with the loop as a hole on the other). Several of them returned solids that pass validate() but are wrong: inside-out patches, or a face that keeps its material inside the loop. Each was reproduced on main (c3a9eea) before the fix; the table is below.

  1. A loop that winds around a sphere's polar axis was classified in the wrong chart. generic_single_closed_loop_boolean picked Patch or Complement from the loop's UV centroid in the sphere's own chart. When the loop winds around the sphere's polar axis (the other surface passes close to the sphere's centre), that centroid lies on neither cap in particular, and the side was then rebuilt in a reframed chart where the Patch is always the cap clear of the new poles. Which cap got built had nothing to do with the classification.
    Fix: the contractible chart is computed first (assembly/sphere_loop_chart.rs, now also centred on longitude π so the loop lies inside (0, 2π)), a point inside the loop is classified in that chart, and a Complement is built as a full sphere rebuilt in that chart with the loop as a hole (plain full spheres only; anything else falls through to the next handler).
  2. The offset sphere-cylinder handler had the same centroid problem when the sphere's polar axis points through the cylinder wall, and nothing reframed the sphere: cylinder ∩ sphere came out inside-out. It now reports a coverage gap for a loop that winds the sphere, so dispatch falls through to the generic closed-loop Boolean, which handles it with fix 1.
  3. A hole could sit a whole period outside its face's UV window. The hole's pcurve comes from the intersection graph's chart. For a cone with a sphere on +y the cone loop runs at longitude ≈ −π/2 while the lateral face spans [0, 2π], so the hole lay outside its outer loop in UV and the face kept its material there. add_hole_lifted_into_face now sets the hole use's periodic_lift so the loop lies inside the face's window. Every closed-loop hole site uses it: generic single and multiple loops, the two-sided cutter band, and offset sphere-cylinder.
  4. A hole across the face's chart seam is refused. A loop that fits the window under no whole-period shift crosses the seam (u = 0 / 2π). It would cut the seam edges of the outer loop, which a hole can't do, and those results were wrong solids that passed validation (cylinder ∩ crossing sphere had volume 0.986 against a sphere of 0.522). It is now a CoverageGap: ["closed intersection loop as a hole", "loop crossing the chart seam of face '<key>'"].

Behaviour change: the seam cases (4) now return CoverageGap instead of a solid. Those solids were wrong, so no correct result is lost.

Related PRs

This PR is based on main and doesn't include #39 or #40. Both of them hit the bugs fixed here:

  • fix(query): validate_orientation catches faces whose material normal points into the solid #39 (validate_orientation) flags generic_closed_loop_handles_periodic_winding (cone ∩ sphere, face lateral:intersection-patch inside-out). Fix 1 resolves it: the cap is now classified and built in the same chart, and the test here checks points in every region and the volume.
  • fix(query): validate_orientation catches faces whose material normal points into the solid #39 also flags generic_closed_loop_imprints_an_off_axis_sphere_cone_intersection union and subtraction (cone:lateral inverted). Fix 4 resolves those: the loop sits across the cone's seam, so both are now refused with a seam CoverageGap instead of being built inverted. The test here asserts that gap, and checks ∩ and sphere − cone by volume.
  • fix(brep): standard loop winding, enforced by validate() #40 (standard loop winding) renames the cone ∩ sphere test to generic_closed_loop_with_periodic_winding_refuses_the_outer_sphere_cap and makes it assert a refusal, because on main the result keeps the sphere cap outside the cone. Once this PR lands, that test should go back to asserting a valid result (as generic_closed_loop_handles_periodic_winding does here). This PR moves that test from tests/generic.rs to tests/closed_loop_windings.rs, so expect a small merge conflict there.

Cases, before → after

Cone r 2, h 2 at the origin. Wrong = the volume identity fails and/or a point in a known region classifies wrongly, while validate() passes.

Case 3.0 (main) This PR
cone ∩ sphere (0.35 at (1.1, 0, 0.8), loop winds the sphere) wrong (inside-out cone patch) correct; I + (S − C) = S within 1e-4
sphere − cone, same error: pcurve does not lie on edge support correct
cone ∪ sphere, cone − sphere, same error / wrong CoverageGap (loop across cone:lateral seam)
torus ∩ sphere, two winding loops error: pcurve not on edge support / invalid face keys correct, inside/outside points classify right
cylinder ∩ sphere, sphere axis through the wall wrong (inside-out) correct; I + (S − C) = S within 3e-4
sphere − cylinder, same wrong, does not tessellate correct
cylinder ∪ / − sphere, same wrong CoverageGap (cylinder:lateral seam)
cone ∪ / ∩ / − sphere on +y, winding loop error / wrong / wrong correct; U + I = C + S, (C − S) + I = C
sphere − cone on +y, winding loop error correct
cone ∪ / − sphere on +y, no winding wrong (identities off by 0.048) correct (off by ≤ 2.7e-3, tessellation error of the cone)
cylinder r 2 ∪ / ∩ / − crossing sphere on +x wrong (∩ = 0.986, sphere 0.522) CoverageGap
crossing sphere − cylinder correct (0.12219 vs 0.12248) unchanged
cone ∪ / − sphere on +x, no winding wrong (C − S larger than the cone) CoverageGap
cone ∩ sphere, sphere − cone on +x correct unchanged
torus R 3 r 1 with a ring torus poking out at (3, 0, 0) (golden booleans.torus.poking-ring.*) wrong (union: points inside the host tube classify Outside; intersection misses points in both) CoverageGap (the loops sit across the host's and the ring's seams)
the same ring at (0, 3, 0.7), R 0.5, r 0.2, loops clear of both seams correct unchanged (now a test and a golden scene, so generic_multiple_closed_loops_boolean stays covered)

Robustness

  • The chart search is unchanged (26 candidate polar axes, scored by pole proximity and longitude span). It now runs before classification instead of after.
  • The Complement rebuild only applies to a plain full sphere (full_sphere). Any other body returns None and the next handler takes it.
  • Lifting is per periodic chart axis. Non-periodic axes are left alone.

Tests

New, in boolean/tests/closed_loop_windings.rs:

  • generic_closed_loop_handles_periodic_winding (moved from generic.rs): points in both, in each operand only, and the volume range.
  • generic_closed_loop_subtracts_a_cone_from_a_sphere_with_winding_loop: I + (S − C) = S.
  • generic_closed_loop_intersects_a_torus_with_a_sphere_with_winding_loop: two orientations.
  • sphere_cylinder_loop_winding_the_sphere_uses_the_generic_closed_loop: classification and I + (S − C) = S.
  • generic_closed_loop_lifts_the_host_hole_into_its_face_window: on +y, winding and non-winding. ∪, ∩, − classify points in all four regions, U + I = C + S and (C − S) + I = C.
  • winding_loop_across_the_host_seam_is_a_coverage_gap: cone and cylinder hosts.
  • generic_multiple_closed_loops_keep_a_ring_poking_out_of_a_torus_off_its_seams: two loops, both clear of the seams. ∪, ∩ and host − ring go through generic_multiple_closed_loops_boolean and classify a point in every region correctly. Checked separately with 4,000 random points per operation against the operands' own classification: 0 wrong.

Changed:

  • generic_closed_loop_imprints_an_off_axis_sphere_cone_intersection: ∪ and cone − sphere assert the seam gap; ∩ and sphere − cone check (S ∩ C) + (S − C) = S.
  • mixed_sphere_cylinder_containment_preserves_exact_cavity_supports: crossing ∪, ∩ and cylinder cut assert the seam gap; the sphere cut checks its volume against a numerical integral (0.12248).

Gate on Windows x86_64 (rustc 1.99), CI=true:

  • cargo fmt --check, cargo clippy --all-targets -D warnings: clean
  • cargo test --all-targets --no-fail-fast: 791 passed, 0 failed (main: all pass)
  • npm run check:source, check:duplicates, check:cycles: clean
  • check:api not run: it needs the built SDK in dist/, and this PR adds no public items (everything new is pub(crate)).
  • cargo run --release --example budgets: rotated-box union 1.8–2.0 ms on both main and this branch, against a 2000 ms bound.

Golden file (needs re-recording on aarch64-apple-darwin)

I can't record tests/fixtures/golden/aarch64-apple-darwin.json from this machine (Windows, x86_64), so it is unchanged in this PR. I dumped every scene's text with OG_GOLDEN_DUMP on main and on this branch: 9 of the 573 texts differ, all of them cases from the table above.

  • booleans.generic.periodic-winding: same handler, now the correct cap
  • booleans.generic.sphere-cone.union, .subtraction: now the seam CoverageGap
  • booleans.sphere-cylinder.crossing.union, .intersection, .subtraction: now the seam CoverageGap
  • booleans.torus.poking-ring.union, .intersection, .subtraction: now the seam CoverageGap

The new keys are booleans.torus.poking-ring-off-seam.union, .intersection and .subtraction. They keep generic_multiple_closed_loops_boolean covered; its only golden scenes were the poking-ring ones. OG_GOLDEN_UPDATE=1 cargo test --offline --test golden on the recorded target should change only these 12 keys.

Files

  • main/opengeometry/src/operations/modifying/boolean/assembly/sphere_loop_chart.rs (new; the sphere reframing moved here from closed_loop.rs)
  • main/opengeometry/src/operations/modifying/boolean/assembly/closed_loop.rs
  • main/opengeometry/src/operations/modifying/boolean/assembly/loop_pcurves.rs
  • main/opengeometry/src/operations/modifying/boolean/assembly/mod.rs
  • main/opengeometry/src/operations/modifying/boolean/handlers/generic/single_closed_loop.rs
  • main/opengeometry/src/operations/modifying/boolean/handlers/sphere_cylinder/offset.rs
  • main/opengeometry/src/operations/modifying/boolean/tests/{closed_loop_windings.rs (new), generic.rs, sphere_cylinder.rs, support.rs, mod.rs}
  • main/opengeometry/tests/golden/booleans_spheres.rs (the off-seam poking-ring scene)

🤖 Generated with Claude Code

Four bugs in the closed-loop Booleans; several returned solids that pass
validate() but are wrong.

- A loop winding around a sphere's polar axis was classified by its UV
  centroid in the sphere's own chart, where it has no inside, while the
  side was built in a reframed chart. The contractible chart
  (assembly/sphere_loop_chart.rs, now centred on longitude pi) is
  computed first, the Patch is classified in it, and a Complement is a
  full sphere rebuilt in that chart with the loop as a hole.
- The offset sphere-cylinder handler had the same problem; a loop that
  winds the sphere is now a coverage gap there, so dispatch falls
  through to the generic closed-loop Boolean.
- A hole's pcurve could lie a whole period outside its face's UV window
  (cone with a sphere on +y), so the face kept its material. Holes are
  lifted into the window with periodic_lift.
- A hole that fits no lift crosses the face's chart seam; that is now a
  CoverageGap ("closed intersection loop as a hole") instead of a wrong
  solid (cylinder r 2 with a crossing sphere: intersection 0.986 against
  a sphere of 0.522).

Golden: the poking-ring scenes (loops across both tori's seams, results
that misclassify points inside the host tube) are now refused, so a
poking ring clear of the seams is added to keep
generic_multiple_closed_loops_boolean covered. The aarch64-apple-darwin
golden file is not re-recorded here: 9 scene texts change and 3 are new.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VFZvqx1RmXzPtbL9VXP4H7
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.

1 participant