Check all clip vertices in the spherical Sutherland-Hodgman containment fallback - #471
Merged
Conversation
…nt fallback When `_intersection_sutherland_hodgman` clips the subject down to nothing, it falls back to asking whether the clip polygon was inside the subject all along. That check only tested `clip_points[1]`, and `spherical_orient >= 0` counts the boundary as inside — so any subject merely touching that one vertex was credited with the clip polygon's whole area. Test every clip vertex instead. Under the documented convex-convex precondition this changes no answer: a sweep of 159600 convex pairs hits the empty-output branch 158732 times and never once finds `clip_points[1]` inside the subject, and genuine containment does not empty the output in the first place. It only costs O(m) point tests on a branch that is already the slow path. Fixes #467 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #467.
The defect
When the spherical
_intersection_sutherland_hodgmanclips the subject down tonothing, it falls back to asking whether the clip polygon was inside the subject
all along. That check only tested
clip_points[1]:_point_in_convex_spherical_polygonusesspherical_orient >= 0, so the boundarycounts as inside. Any subject that merely touches the clip ring's first vertex
passed the test and was handed the clip polygon's entire area.
Instrumenting the three grazing tiles from the issue's MWE shows it directly —
for each one,
output = 0 ptsand the per-clip-vertex containment vector is[1, 0, 0, 0, 0]. One vertex in, four out, whole clip returned. Summed over thethree disjoint tiles that is 3x the clip's own area; in the reported polar case
it is 145x, where every degenerate quad of a 216-column mesh shares the pole.
The fix
Require every clip vertex to be inside:
Two things I verified before landing this, both of which say it is free:
pairs hits the empty-output branch 158732 times and finds
clip_points[1]inside the subject zero times. Re-running the sweep after the change gives an
identical result, so this is a no-op in contract.
already returns the right answer when the clip is inside the subject — for a
small quad inside a big one the output is 4 points, not empty. The branch's
stated purpose is handled upstream.
It stays sound out of contract too:
_point_in_convex_spherical_polygonon anon-convex subject under-approximates (it answers about the half-space
intersection, a subset), so it cannot produce a false positive.
The cost is O(m) point tests instead of 1, on a branch that is already the slow
path. Only the spherical path has this fallback; the planar path has none.
Test
Adds "Grazing tiles are not handed the whole clip". Written first: it failed 4/4,
at exactly 1.0x the clip area per tile and 3.0x summed, then passed.
One caveat worth flagging for review: as noted in #467, reaching the fallback at
all takes a non-convex clip, so the fixture is out-of-contract input — with a
convex clip a grazing subject survives every half-space test and leaves a 1-2
point output that the degenerate branch already zeroes. The test is therefore
framed as graceful degradation: the answer may be wrong, but it must be wrong
towards zero, never towards a whole fabricated cell. That is a realistic concern
rather than a contrived one, since a near-convex cell can go non-convex
numerically (see #466). If an out-of-contract fixture is unwanted in the suite,
the fix stands on the in-contract sweep alone and the test can be dropped.
Verification
Full suite passes (
Testing GeometryOps tests passed); Sutherland-Hodgman 69/69.The issue's MWE now returns exactly
0.0for all three tiles.🤖 Generated with Claude Code