Skip to content
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

coords array: polygon with holes vs multipolygon #44

Closed
codeofsumit opened this issue Sep 22, 2017 · 17 comments
Closed

coords array: polygon with holes vs multipolygon #44

codeofsumit opened this issue Sep 22, 2017 · 17 comments

Comments

@codeofsumit
Copy link

codeofsumit commented Sep 22, 2017

Hey there, I'm trying to replace turfs difference and intersect with your library in leaflet.pm (drawing plugin for leaflet) due to size.
To illustrate the problem I've included gifs of using your library.

When using difference, I'm having a bit trouble figuring out if the result is a MultiPolygon or a Polygon with a hole.
In your examples, you always expect it to be a Multipolygon but if you treat a hole that way - two layers are being drawn.

Using diff on a polygon when the result is a regular polygon
lpm

Using diff on a polygon, cutting it in half, and the result is a multipolygon:
lpm2

Using diff on a polygon, cutting a hole in it. The resulting coords array looks exactly like a multipolygon, so both are getting drawn:
lpm3

How would you recommend to find out if it's a hole or a multipolygon?

@codeofsumit codeofsumit changed the title polygon with holes vs multipolygon coords array: polygon with holes vs multipolygon Sep 22, 2017
@DenisCarriere
Copy link
Contributor

@codeofsumit Side topic, we would be using the martinez library for TurfJS's intersect & difference if the hole in the MultiPolygon issue get sorted out.

👍

@rowanwins Could we use martinez right now?

@rowanwins
Copy link
Collaborator

Mmm at the moment I think we're just trying to sort out a performance bug @DenisCarriere . We could try and incorporate it into turf and see how the tests and benchmark faired, I'll try and take a look at that this week and will report back

@rowanwins
Copy link
Collaborator

Actually just dropping another note in here, I've attempted to replace jsts within turf/difference but I didn't have much joy. There still seems to be some issues with multipolygons as well as polygons with holes. The test suite for turf was very handy for identifying the issues so that was positive :)

@w8r
Copy link
Owner

w8r commented Sep 26, 2017

difference is broken, as I stated before, but it wasn't before we made changes, so I think we can fix it.

@rowanwins
Copy link
Collaborator

ahh sorry @w8r I misunderstood that other comment you left, I'll try getting one of the other operations into turf and see if I uncover anything else

@rowanwins
Copy link
Collaborator

No joy with union either @w8r when using this input I get this output, the hole is nested too deeply

@w8r
Copy link
Owner

w8r commented Sep 26, 2017

Very cool that you have examples!

@codeofsumit
Copy link
Author

hey guys, just wanted to ask how it's going on this? @w8r you mentioned difference is broken - can you let me/us know if this will change short-term or stay this way for the foreseeable future (considering your own time working on it if no surprise PR comes in)?

@rowanwins
Copy link
Collaborator

slow is the answer @codeofsumit :(

I'm currently tied up with some turf work for the next week but once that is sorted this is going to be my main priority.

@w8r
Copy link
Owner

w8r commented Oct 9, 2017

I am sorry it goes so slow, but I really work on it sporadically. Maybe we can really plan a session when we can sit and resolve that stuff once and for all. I really believe we have lots of test cases and now that the code is simpler, we can fix the problem. Actually holes marking was done by indexing and in the paper it even supports 'islands' as 'holes' of 'holes', which might be the flaw. I really need to sit down and look at it closely

@codeofsumit
Copy link
Author

any updates here?

@rowanwins
Copy link
Collaborator

Hi @codeofsumit

I've made some progress which is available in #42 , although there are still a couple of outstanding issues that occur in very isolated situations. I would say that in 95% of situations you'll likely get a good and predictable result, so whether that is enough for you I'm not sure...

@rowanwins
Copy link
Collaborator

Oh and ps in the branch mentioned above everything is output as a multipolygon. It may be there that the output only have 1 polygon, but this was we can provide a consistent output in terms of nesting etc.

@codeofsumit
Copy link
Author

@DenisCarriere is it enough to get it into turf?

@rowanwins
Copy link
Collaborator

It's really close, but not quite sufficient for my liking.
FYI I'm involved in Turf as well, as soon as all this work is tidied up I'll make sure it's incorporated in Turf asap.

@DenisCarriere
Copy link
Contributor

👍 Agreed! We will definitely add Martinez in Turf! This will open up many new doors for new spatial operations.

Looking forward to that day! 🎉

@w8r
Copy link
Owner

w8r commented Dec 20, 2017

will be fixed in #42

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

No branches or pull requests

4 participants