fix: classifyAndGroup pairs a hole with the wrong hull#41
Open
terbin wants to merge 1 commit into
Open
Conversation
It paired each hole with the first hull whose filled area intersected it, which is order-dependent and wrong when an island hull sits inside another hull's hole. Pair each hole with the innermost hull that actually contains it instead, and add PolygonHelperTest.
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.
Symptom
PolygonHelper.classifyAndGroupsometimes attaches a hole to the wrong hull. Thehull that truly owns the hole then renders with no hole carved, so its fill covers a
region that was supposed to be empty. When two mods draw adjacent filled tones (one
showing through the other's hole), the mis-paired hull's fill floods solid over the
neighboring tone.
Root cause
The grouping paired a hole with the first hull whose filled outline merely
intersected it, then removed the hole from the pool:
toArea(hull)fills the hull ignoring nesting, so "intersects" is far too weak atest. It breaks whenever a hull is nested inside another hull's hole (an island
sitting inside a hole): both the outer hull and the inner island have filled areas
that overlap the hole between them, so whichever hull the loop visits first claims it.
The pairing is therefore order-dependent, and when the inner island wins it is handed
a hole larger than itself (island minus hole renders as nothing) while the outer hull
is left with no hole and fills solid over the region that should have been carved out.
Reproduction (nested island-in-hole)
The contours are the square (hull), the big rectangle (a hole), and the small
rectangle (an island hull). Correct output: the big hole belongs to the square,
and the island has no holes. On the old code the hole is order-dependently grabbed by
the island, the square keeps no hole, and the square's fill covers the region that
should have stayed empty.
(The test builds this topology with rectangles rather than ellipses on purpose:
createPolygonFromAreaonly extractsSEG_MOVETO/SEG_LINETOsegments, so anellipse's curved contour would be dropped. Rectilinear contours also match JourneyMap's
real usage, which unions chunk squares.)
Fix
Pair each hole with the innermost hull that actually contains it:
(
OminusHis empty). An island's fill intersects a hole it does not own, sointersection is not enough.
the unsigned shoelace area of the hull ring and pick the smallest that contains the
hole. This also makes the result independent of contour order.
input) is dropped rather than forced onto an unrelated hull.
isHole,toArea, andcreatePolygonFromArea's contour extraction are unchanged;only the grouping logic changed.
Tests
Adds
common/src/test/java/journeymap/api/v2/client/util/PolygonHelperTest.java:nestedIslandDoesNotStealTheEnclosingHullsHolereproduces the bug (fails on the oldcode, passes on the fix).
holeIsPairedWithTheInnermostContainingHullcovers a hole enclosed by two nestedhulls.
holeWithNoContainingHullIsDroppedpins the drop path for an unenclosed hole.plainDonutKeepsItsHoleandseveralNonNestedHolesAllGroupWithTheirHullguard thecommon cases.
Each test also asserts the invariant that re-unioning every (hull minus its holes)
reproduces the input
Area, which catches both a stolen and a dropped hole../gradlew :common:testpasses, including the pre-existing tests.Branch
Based on
1.21.1_2.0.0, the repository's default branch. The same change applies to the sibling per-Minecraft-version branches; happy to retarget or port it if you would prefer a different base.