Repository navigation
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
Conversation
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
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
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.
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 onmain(c3a9eea) before the fix; the table is below.generic_single_closed_loop_booleanpicked 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).add_hole_lifted_into_facenow sets the hole use'speriodic_liftso 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.CoverageGap:["closed intersection loop as a hole", "loop crossing the chart seam of face '<key>'"].Behaviour change: the seam cases (4) now return
CoverageGapinstead of a solid. Those solids were wrong, so no correct result is lost.Related PRs
This PR is based on
mainand doesn't include #39 or #40. Both of them hit the bugs fixed here:validate_orientation) flagsgeneric_closed_loop_handles_periodic_winding(cone ∩ sphere, facelateral:intersection-patchinside-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.generic_closed_loop_imprints_an_off_axis_sphere_cone_intersectionunion and subtraction (cone:lateralinverted). 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.generic_closed_loop_with_periodic_winding_refuses_the_outer_sphere_capand makes it assert a refusal, because onmainthe result keeps the sphere cap outside the cone. Once this PR lands, that test should go back to asserting a valid result (asgeneric_closed_loop_handles_periodic_windingdoes here). This PR moves that test fromtests/generic.rstotests/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.main)cone:lateralseam)cylinder:lateralseam)booleans.torus.poking-ring.*)generic_multiple_closed_loops_booleanstays covered)Robustness
full_sphere). Any other body returnsNoneand the next handler takes it.Tests
New, in
boolean/tests/closed_loop_windings.rs:generic_closed_loop_handles_periodic_winding(moved fromgeneric.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 throughgeneric_multiple_closed_loops_booleanand 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: cleancargo test --all-targets --no-fail-fast: 791 passed, 0 failed (main: all pass)npm run check:source,check:duplicates,check:cycles: cleancheck:apinot run: it needs the built SDK indist/, and this PR adds no public items (everything new ispub(crate)).cargo run --release --example budgets: rotated-box union 1.8–2.0 ms on bothmainand 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.jsonfrom this machine (Windows, x86_64), so it is unchanged in this PR. I dumped every scene's text withOG_GOLDEN_DUMPonmainand 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 capbooleans.generic.sphere-cone.union,.subtraction: now the seam CoverageGapbooleans.sphere-cylinder.crossing.union,.intersection,.subtraction: now the seam CoverageGapbooleans.torus.poking-ring.union,.intersection,.subtraction: now the seam CoverageGapThe new keys are
booleans.torus.poking-ring-off-seam.union,.intersectionand.subtraction. They keepgeneric_multiple_closed_loops_booleancovered; its only golden scenes were the poking-ring ones.OG_GOLDEN_UPDATE=1 cargo test --offline --test goldenon 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 fromclosed_loop.rs)main/opengeometry/src/operations/modifying/boolean/assembly/closed_loop.rsmain/opengeometry/src/operations/modifying/boolean/assembly/loop_pcurves.rsmain/opengeometry/src/operations/modifying/boolean/assembly/mod.rsmain/opengeometry/src/operations/modifying/boolean/handlers/generic/single_closed_loop.rsmain/opengeometry/src/operations/modifying/boolean/handlers/sphere_cylinder/offset.rsmain/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