@turf/boolean-contains: Add MultiPolygon support for non-polygon types

[rexxars]

Changes:

1. Extends booleanContains to support MultiPolygon as the containing geometry for additional geometry types. Previously, booleanContains(MultiPolygon, X) only worked when X was a Polygon.
2. Fixes(?) behavior of booleanContains to return true if every point is inside/on boundary of some polygon in the MultiPolygon, and at least one is strictly interior. This previously returned false, but differs from booleanWithin and doesn't match DE-9IM semantics. In my eyes this is a bugfix, not a breaking change

New supported combinations:

• MultiPolygon contains Point
• MultiPolygon contains MultiPoint
• MultiPolygon contains LineString
• MultiPolygon contains MultiLineString
• MultiPolygon contains MultiPolygon

Follows DE-9IM "contains" semantics:

• Points must be in the interior (not on boundary) of at least one polygon
• LineStrings must be fully contained within a single polygon (cannot span the gap between polygons)
• Each component of multi-geometries must be contained within some single polygon, but different components can be in different polygons

Questions:

1. booleanContains documentation states booleanContains(a, b) is equivalent to booleanWithin(b, a) - if that's true, why does booleanWithin not import the booleanContains module and swap the argument order instead of redefining all the logic?
2. Are there any rules on what should/shouldn't be exported from these modules? The contains module exports both methods without jsdoc, and ones with jsdoc marked as @private (such as compareCoords). I wasn't sure if I should export the new helper methods or not, and if they should be marked as private

[smallsaucepan]

Thanks heaps for putting this together @rexxars. To some of your questions:

why does booleanWithin not import the booleanContains module and swap the argument order instead of redefining all the logic?

We asked ourselves this recently, and discovered some performance differences for what should be equivalent approaches. Before pressing ahead, we wanted to understand why that was happening first. Though yes, ideally we would have a single implementation that served both operations.

Are there any rules on what should/shouldn't be exported from these modules? The contains module exports both methods without jsdoc, and ones with jsdoc marked as @Private

Generally speaking, we try to have each package export a single function (helpers and meta being the exceptions). Any other packages that do that is doing so probably because we've never had a good place to keep common code in the monorepo. We're half way through adding an internal code only package. Suspect between your two PRs there might be some good candidates?

[smallsaucepan]

And by way of confirmation this PR will fill in the blue sections below?

[rexxars]

We asked ourselves this recently, and discovered some performance differences for what should be equivalent approaches. Before pressing ahead, we wanted to understand why that was happening first.

Curious! Not a blocker for this PR, was just curious about the context around it

Suspect between your two PRs there might be some good candidates?

Many of the same methods are implemented in contains and within, yes - and there's a few other methods that I am sure could be moved to a helper/shared util package as well. The main reason why I was asking is that there are many functions exported from the contains package, that are not used by other modules. Some of them are marked as @private, others are not. None of them seems to appear in the API docs. Was just trying to figure out if the methods I added in this PR should also be exported:

• isLineInMultiPolygon
• isMultiLineStringInMultiPolygon
• isMultiPointInMultiPolygon
• isMultiPolygonInMultiPolygon
• isPointInMultiPolygon

For reference, these are all the (currently) exported methods from @turf/boolean-contains:

• booleanContains
• compareCoords
• doBBoxOverlap
• getMidpoint
• isLineInPoly
• isLineOnLine
• isMultiPointInMultiPoint
• isMultiPointInPoly
• isMultiPointOnLine
• isMultiPolyInPoly
• isPointInMultiPoint
• isPolygonInMultiPolygon
• isPolyInPoly

I could see the rationale for exporting the is-prefixed methods, as if you know ahead of time that you're only going to check one geometry against another, you can tree-shake away all the logic for geometries you don't use - but wasn't sure if that was the intention or just a side effect. Other methods, like compareCoords, doBBoxOverlap and getMidpoint seems a bit odd to export from this module however.

And by way of confirmation this PR will fill in the blue sections below?

Yes, that appears correct.

[smallsaucepan]

I would guess it's mostly historical "just in case" exporting. Can you see any drawbacks to the below as a plan:

1. merge this PR
2. get the turf-internals package published
3. hoist everything from compareCoords ... to ... isPolyInPoly into that (maybe keeping a facade in contains and within until a major release to maintain backwards compat in case someone has used those undocumented functions)
4. refactor booleanWithin to use turf-internals too

Not asking you to commit to any of this beyond what you've already done. Just making the most of your recent perusal of those two packages.

[rexxars]

Yep, happy with that plan. If I could be slightly selfish, I'd love to have this merged and released and then do some follow-up PRs to address point 2-4 (happy to help with this and more, but they're not backwards incompatible changes). I'm trying to use this library in groq-js (for our geo functions) and that would unblock that work :)

[smallsaucepan]

Would you mind expanding on the reasoning here? I would have expected for a MultiPoint to be contained in a poly every point of that MP is contained.

[rexxars]

The topological definition of "contains" per the DE-9IM says
that A contains B requires two conditions:

1. No point of B lies in the exterior of A (tested on line 298)
2. At least one point of the interior of B lies in the interior of A (tested below)

If every point of the MultiPoint (B) sits exactly on the polygons boundary (eg. on an edge or vertex), then the interiors of A and B never intersect.

This is point two of what I mentioned in the PR description:

Fixes(?) behavior of booleanContains to return true if every point is inside/on boundary of some polygon in the MultiPolygon, and at least one is strictly interior. This previously returned false, but differs from booleanWithin and doesn't match DE-9IM semantics. In my eyes this is a bugfix, not a breaking change