Skip to content

fix: classifyAndGroup pairs a hole with the wrong hull - #41

Open
terbin wants to merge 2 commits into
TeamJM:1.21.1_2.0.0from
terbin:fix/classify-and-group-hole-containment
Open

fix: classifyAndGroup pairs a hole with the wrong hull#41
terbin wants to merge 2 commits into
TeamJM:1.21.1_2.0.0from
terbin:fix/classify-and-group-hole-containment

Conversation

@terbin

@terbin terbin commented Jul 18, 2026

Copy link
Copy Markdown

Symptom

PolygonHelper.classifyAndGroup sometimes attaches a hole to the wrong hull. The
hull 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:

final Area intersection = new Area(hullArea);   // hullArea = toArea(hull), filled solid
intersection.intersect(holeArea);
if (!intersection.isEmpty()) { hullHoles.add(hole); iterator.remove(); }

toArea(hull) fills the hull ignoring nesting, so "intersects" is far too weak a
test. 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)

Area a = new Area(new Rectangle(0, 0, 300, 300));       // outer square (hull)
a.subtract(new Area(new Rectangle(30, 30, 240, 240)));  // cut a big rectangular hole
a.add(new Area(new Rectangle(125, 125, 50, 50)));       // island sitting inside the 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:
createPolygonFromArea only extracts SEG_MOVETO/SEG_LINETO segments, so an
ellipse'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:

  • Containment, not intersection: hull H owns hole O only if O lies entirely inside H
    (O minus H is empty). An island's fill intersects a hole it does not own, so
    intersection is not enough.
  • Innermost when several contain it: containing hulls are nested, so rank candidates by
    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.
  • A hole belongs to exactly one hull; a hole left with no containing hull (malformed
    input) is dropped rather than forced onto an unrelated hull.

isHole, toArea, and createPolygonFromArea's contour extraction are unchanged;
only the grouping logic changed.

Tests

Adds common/src/test/java/journeymap/api/v2/client/util/PolygonHelperTest.java:

  • nestedIslandDoesNotStealTheEnclosingHullsHole reproduces the bug (fails on the old
    code, passes on the fix).
  • holeIsPairedWithTheInnermostContainingHull covers a hole enclosed by two nested
    hulls.
  • holeWithNoContainingHullIsDropped pins the drop path for an unenclosed hole.
  • plainDonutKeepsItsHole and severalNonNestedHolesAllGroupWithTheirHull guard the
    common 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:test passes, 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.

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.
@mysticdrew

Copy link
Copy Markdown
Member

Can you show screenshot/video of the issue in action and then similar with the fix please?

@terbin

terbin commented Jul 20, 2026

Copy link
Copy Markdown
Author

here I have a teal and amber color before the fix:
Screenshot 2026-07-18 204800
here after the fix:
Screenshot 2026-07-18 224757

this is of different areas. if you want I can redo-screenshots of the same area, this is an outline for a yet unreleased world download mod that shows the chunks where you downloaded entities for sure in teal, and does an estimate for chunks, where you didn't get the entities in amber. Happy to redo screenshots of the same area if required or in another form if you need.

@mysticdrew

Copy link
Copy Markdown
Member

These screenshots are fine I think.
Which AI tool did you use for the PR?

@terbin

terbin commented Jul 20, 2026

Copy link
Copy Markdown
Author

Claude Code Opus 4.8, multiple review rounds.

@mysticdrew

mysticdrew commented Jul 20, 2026

Copy link
Copy Markdown
Member

Claude Code Opus 4.8, multiple review rounds.

Thanks for the honesty.
I'll have a few comments on the code shortly.

@terbin

terbin commented Jul 20, 2026

Copy link
Copy Markdown
Author

No hurries. I have implemented a workaround in my mod so it also works with if this PR does not get merged.
And I completely understand if you choose not to want to pull in AI generated code.

Comment thread common/src/main/java/journeymap/api/v2/client/util/PolygonHelper.java Outdated
Comment thread common/src/main/java/journeymap/api/v2/client/util/PolygonHelper.java Outdated
Comment thread common/src/main/java/journeymap/api/v2/client/util/PolygonHelper.java Outdated
Comment thread common/src/main/java/journeymap/api/v2/client/util/PolygonHelper.java Outdated
@mysticdrew

Copy link
Copy Markdown
Member

No hurries. I have implemented a workaround in my mod so it also works with if this PR does not get merged. And I completely understand if you choose not to want to pull in AI generated code.

I am fine with AI generated code, these tools are a huge help. But they need to be well formed PRs and not slop. This PR is clean and not sloppy. Thanks for that. I've had some that are really bad in the journeymap-legacy repo.

I used fable and codex 5.6 to assist with the review as well.

- fall back to the first intersecting hull for a hole that no hull
  contains, preserving classifyAndGroup's documented arbitrary-polygon
  behavior instead of silently dropping a partially overlapping hole
- share one signed shoelace helper between isHole and ringArea
- scope the order-independence note to normalized Area contours and
  document the hole-association contract on classifyAndGroup
- initialize the per-hull hole lists with forEach
- add a partial-overlap regression test
@terbin
terbin requested a review from mysticdrew July 21, 2026 01:44

@mysticdrew mysticdrew left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix and speedy responses and code review updates.

I'll get this merged when I have more time to cherry pick to all versions api.2.0.0, This will be in the next release for supported versions.

@terbin

terbin commented Jul 21, 2026

Copy link
Copy Markdown
Author

Thank you for the kind interaction and helpful comments. Really helps learning.

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