-
Notifications
You must be signed in to change notification settings - Fork 935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mask - option for islands in holes to be pulled out as separate MultiPolygon exterior rings #1305
Comments
Hey @andrewharvey thanks for the interesting topic. Here my two cents.
They should be part of a |
If that's the case then the current output of turf.mask is wrong and it looks okay in Leaflet by luck, in which case what I propose shouldn't be an option it should be the default behaviour. |
I checked on the GeoJSON spec mailing list and this is correct, the islands should be part of a MultiPolygon. Checking the source code I realised the mask function is defined at a very low level in terms of operations in the exterior and interior rings, so technically what it currently does is correct, just not what I as a user would expect. So I'm thinking we should change the definition to be higher level and change the behaviour so input Polygon's which have inners are spit out as new Polygons part of a MultiPolygon. I'm sure there's a lot of edge cases though where this won't work, am I opening a can of worms? turf.difference works as expected for this use case, but it doesn't handle all the edge cases like the overlapping polygon test, it errors with "side location conflict". /cc @rowanwins |
also related turf.mask seems to get stuck in an infinite loop on this https://gist.github.com/andrewharvey/7cf52badbe77e9843096a882a5e93c3e. I've since migrated to using turf.difference instead of mask. |
Agree -- @andrewharvey did you figure this out? I am getting the same invalid Polygon using difference: a "Polygon" with two concentric interior rings, the second of which is an island in the hole. UPDATE: I get an invalid Polygon using |
This is a simple test case that fails to produce a valid MultiPolygon for both
|
The mask function takes an
input
polygon and inverts it such all areas outside theinput
polygon now form the inside of theoutput
.When you pass
mask
a Polygon with a hole (an exterior ring and an interior ring) like this:https://gist.github.com/andrewharvey/971b9db1900a62efa7635f6a6d337ae7
It will simply add a new global exterior ring and make the input rings interior rings.
Although I'm not 100% sure if supported by the GeoJSON spec many applications which support GeoJSON allow for a GeoJSON
Polygon
with an interior ring inside another interior ring to be interpreted as an island inside a hole inside a polygon.I think the winding order of the interior rings should be used to spell out if the ring should be a hole or island, although this seems to make no difference in Leaflet.
So in Leaflet this works and visually creates the mask you expect.
https://gist.github.com/andrewharvey/c092a6c4932a4d94ecb6cd59dc935b90
However not all applications support this for example earcut does not: mapbox/earcut#94.
I propose
mask
should have an option for these islands within holes to be pulled out as separate exterior rings as part of aMultiPolygon
geometry type rather than inside thePolygon
type which is what earcut asks for.I'm working on a PR at the moment to do this, appreciate any thoughts on the issue.
It should also fix the issue where you can't round trip that second example back to the first by again applying the mask.
The text was updated successfully, but these errors were encountered: