booleanPointInPolygon - Perf improvement + fix edge case #2821
Conversation
This file contains 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
Optimize booleanPointInPolygon to return "true" as soon as one polygon contains the point instead of looping over all the polygons
…polygon and inside another one
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.
Perf improvement
No breaking changes; it simply return early once it confirmed that the point is inside at least one polygon, instead of checking all the other polygons.
Fix edge case
If the point was on the edge of a polygon, the function was returning immediately "true" if the option ignoreBoundary was false, and false otherwise.
But the RFC does not say that polygons inside a multipolygon cannot overlap, it only says a multipolygon is an array of polygons.
https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.7
So in case the point is on the border of one of the polygon and ignoreBoundary is true, the loop should continue to see if the point is inside an another polygon.
Note that some libraries seems to consider that multipolygon must not overlap
Leaflet/Leaflet#6173 (comment)
But this PR is not even a breaking change if we consider this definition, since for a non overlapping multipolygons the result of the function will stay the same ; the only difference is that it will indeed loop over all the polygons instead of exiting early.
Tests
The tests have been ran for this package, and a new case has been created to specifically test the situation with a multipolygon with a point on the borders of a small polygon inside a bigger one.
I let Github CI run the full tests.