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

Adds doc note on @turf/polygonize #898

Merged
merged 1 commit into from
Aug 14, 2017
Merged

Adds doc note on @turf/polygonize #898

merged 1 commit into from
Aug 14, 2017

Conversation

stebogit
Copy link
Collaborator

@stebogit stebogit added the docs label Aug 13, 2017
@stebogit stebogit self-assigned this Aug 13, 2017
@NickCis
Copy link
Contributor

NickCis commented Aug 13, 2017

I'm currently working on an implementation of unary union. Perhaps it's better to wait until I finish it in order to update the docs with a real solution.

@stebogit
Copy link
Collaborator Author

@NickCis do you think you're going to have it done fairly soon? My goal was basically to close #819 to keep the list as short as possible.
We can easily modify again the docs whenever your module is done.

BTW were you thinking to add unary-union to @turf/polygonize to avoid the issue entirely?

@DenisCarriere
Copy link
Member

@NickCis Nice! Looks like you are doing some good progress on unary-union, looking forward to see everything 100% tested and good to go.

Feel free to create a TurfJS wrapper for your module, @turf/unary-union which we could then use internally for polygonize.

I'm happy to see that you're dist is a babel ES5 output 👍

@DenisCarriere
Copy link
Member

@stebogit My goal was basically to close #819

It's good to update the docs, however this still wouldn't solve the issue.

I really like the idea of the unary-union module, I think it will be used a lot! (well I am personally going to use it! 😄)

@DenisCarriere
Copy link
Member

BTW were you thinking to add unary-union to @turf/polygonize to avoid the issue entirely?

I'm pretty sure that would be the intent... I would suggest we do that 👍 .

@stebogit
Copy link
Collaborator Author

It's good to update the docs, however this still wouldn't solve the issue.

Isn't the current issue just a lack of info on a (currently) missing step to avoid the unexpected (but not wrong apparently) result? That's why the docs label.
Otherwise we might label it as bug and keep it open until unary-union is available.
However I would not consider this a bug.

@NickCis
Copy link
Contributor

NickCis commented Aug 14, 2017

From a developer point of view, polygonize works as expected, the docs clearly say:

Polygonizes a set of lines that represents edges in a planar graph. Edges must be correctly noded, i.e., they must only meet at their endpoints.

But, from an user's perspective polygonize lacks of unary-union functionality.

I would suggest to keep the issue open until i finish the unary-union implementation. I can't give a estimated time since i'm only working on it on my free time.

@DenisCarriere
Copy link
Member

@NickCis Big thanks for those unary-union contributions. 👍

@stebogit We can merge these docs notes and whenever we update polygonize with unary-union we can re-update the docs accordingly.

@DenisCarriere DenisCarriere merged commit 9078c16 into master Aug 14, 2017
@DenisCarriere DenisCarriere deleted the issue-#819 branch August 14, 2017 17:10
@DenisCarriere DenisCarriere added this to the 4.7.0 milestone Aug 14, 2017
@rowanwins
Copy link
Member

rowanwins commented Aug 14, 2017

Gday @NickCis

Nice job on the unary union stuff, that would certainly be super useful for our dissolve module.

As an FYI we've also been trying to provide a bit of support to the martinez repo by @w8r which does the full range of clip, difference, union etc. Not sure if you've seen that but with checking out.

And on a side note it would be great if someone could publish a sweepline module to stop people from having to reinvent that wheel as well, I'm sure it would be a handy module for a range of things.

@w8r
Copy link

w8r commented Aug 14, 2017

And on a side note it would be great if someone could publish a sweepline module to stop people from having to reinvent that wheel as well, I'm sure it would be a handy module for a range of things.

@ggolikov is working on that right now, btw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants