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

[rexxars]

Extends booleanContains to support MultiPolygon as the containing geometry for additional geometry types. Previously, booleanContains(MultiPolygon, X) only worked when X was a Polygon.

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 :)

[mfedderly]

A quick look makes me think there's at least one logic bug in this PR, and the fact that we've got within and contains as completely separate implementations seems a little rough. I wonder if we should revisit this from a "we should just mirror de9im's implementation" angle before continuing to maintain both of these packages in their current state. I'm not convinced that either of these implementations are actually spec-complaint at the moment.

Thoughts?

[mfedderly]

Sorry, I think I missed another case that will need some attention, but this time in the MultiLineString implementation.

As an aside: these tests are fantastic (thank you!), and you should totally take credit in the contributors section of package.json if you'd like to.

[mfedderly]

I think that this has a similar false negative problem with a MultiLineString where the first one is on a border and the second one is at least partly inside.

As an aside I looked at the de9im package's implementation of this and they just check the line centroids which makes me a little uncomfortable.

One thing they do seem to do nicely though is upgrade to the Multi equivalents to keep the implementation a little tidier. I think part of why this implementation is tough is because you can't actually reuse the isLineInPoly in this way because of the nuance of a multi check actually allowing some border-only geometries along the way.

[rexxars]

Sorry, I think I missed another case that will need some attention, but this time in the MultiLineString implementation.

Yep, you were right - my apologies. I was requiring every line to have an interior segment, when the "at least one interior point" condition should apply across the MultiLineString as a whole. A line lying entirely on a boundary is fine, as long as one of its siblings "dips into" the interior.

I've pushed a fix. Since isLineInPoly couldn't be reused directly (as you noted, it conflates "not outside" and "has interior" into a single boolean), I split it into a lineInPolyStatus helper that classifies a line as outside/boundary/interior against a polygon. isLineInPoly itself just checks for interior now, so its behavior is unchanged. The MultiLineString check then rejects if any line touches the exterior, and accepts only if at least one line ends up strictly interior somewhere. Let me know if you think of a better way to handle this.

Added fixtures for both the boundary-plus-interior case (true) and the all-lines-on-boundary case (false), and both agree with JSTS.

they just check the line centroids which makes me a little uncomfortable

Same - a centroid of a line that wanders in and out of a polygon can land pretty much anywhere, so I steered away from that approach.

One thing worth noting: a line spanning two polygons that touch at a single point would still return false here, since each line is checked against individual polygons rather than the union. Can't happen for properly disjoint MultiPolygons, so I'd suggest deferring that to the de9im-alignment follow-up, maybe? If not, let me know and I can take a stab at it here.

you should totally take credit in the contributors section of package.json if you'd like to.

Thanks! I'm already in there based on another PR I did earlier, I believe :)

[mfedderly]

🎉 thank you!