Skip to content

fix(boolean-within): require an interior point in MultiPoint-in-Polygon check#3085

Open
greymoth-jp wants to merge 2 commits into
Turfjs:masterfrom
greymoth-jp:fix/boolean-within-multipoint-interior
Open

fix(boolean-within): require an interior point in MultiPoint-in-Polygon check#3085
greymoth-jp wants to merge 2 commits into
Turfjs:masterfrom
greymoth-jp:fix/boolean-within-multipoint-interior

Conversation

@greymoth-jp

Copy link
Copy Markdown

booleanWithin(multiPoint, polygon) gives the wrong answer when a MultiPoint has an interior point followed by a boundary point.

isMultiPointInPoly should return true when every point is inside the polygon (boundary allowed) and at least one point is strictly interior. But oneInside was declared and never assigned, so the if (!oneInside) branch ran on every iteration and overwrote isInside with the strict-interior result of the current point. The final return output && isInside then depended only on whether the last point was interior, not whether any point was.

booleanWithin(
  multiPoint([
    [0.5, 0.5], // interior
    [1, 1], // corner / boundary
  ]),
  polygon([
    [
      [0, 0],
      [1, 0],
      [1, 1],
      [0, 1],
      [0, 0],
    ],
  ])
);
// master:   false
// expected: true   (0.5,0.5 is interior, 1,1 is on the boundary, both are inside)

Reversing the points to [[1, 1], [0.5, 0.5]] returns true on master, so the result is also order-dependent.

This is the mirror of booleanContains, whose equivalent helper was fixed for this case in #3024 (booleanContains(a, b) is documented as equivalent to booleanWithin(b, a)). The fix here latches oneInside once a strictly-interior point is found, matching that change.

Added a true fixture covering the interior-then-boundary case (test/true/MultiPoint/Polygon/MultiPointOneOnBoundaryOneInterior.geojson). It fails before the change and passes after; the existing fixtures are unchanged. Not a breaking change.

The open draft #3011 de-duplicates the contains/within helpers and would cover this too. Happy to close in favour of that if you'd rather fold it in, but since the bug is in a released version this seemed worth fixing on its own.

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