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

Fix crash on turf.intersect for touching polygons #2238

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pietervdvn
Copy link

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

Submitting a new TurfJS Module: N/A

Hi,

While testing turfjs in MapComplete, I discovered that turf.intersect(p0, p1) would crash if these polygons shared a border over multiple points.

I've added a testcase for this in the turf-intersect-package - which indeed crashed. Then I fixed it by checking that the result of the polygon-clipping would return a correct linear ring (at least for the outer ring).

At last, I've updated the documentation to clarify that polygons that touch will return null as well.

@pietervdvn
Copy link
Author

These are the test polygons:
image

@JamesLMilner
Copy link
Collaborator

Seems sensible to me and is always good to cover edge (literally) cases. One for @rowanwins?

@pietervdvn
Copy link
Author

Cool! Thx for merging, looking forward to the next release!

@pietervdvn pietervdvn closed this Dec 28, 2021
@twelch
Copy link
Collaborator

twelch commented Dec 29, 2021

Actually @pietervdvn this PR was not merged back to turf master, I did the opposite and merged latest turf master to this PR to update it and run the CI tests. Can you reopen it?

@pietervdvn pietervdvn reopened this Dec 29, 2021
@pietervdvn
Copy link
Author

oops!

@rowanwins rowanwins self-requested a review January 11, 2022 10:04
@rowanwins
Copy link
Member

Thanks for this PR @pietervdvn

So the main question mark here is what is the right output. Prior to v6 when we use jsts for performing the operation the returned feature could be a point, line or poly. That functionality kinda got lost as we experimented with and transitioned to polygon-clipping, but I think it's probably the better behaviour that we could now reinstate.

So perhaps we should rejig this PR so it can once again provide that response? That would be a breaking change so we'd need to include it in the next major release.

Extract from v5.1.5 jsdoc

@returns {Feature|null} returns a feature representing the point(s) they share (in case of a {@link Point} or {@link MultiPoint}), 
the borders they share (in case of a {@link LineString} or a {@link MultiLineString}), 
the area they share (in case of {@link Polygon} or {@link MultiPolygon}). 
If they do not share any point, returns `null`.

Does that make sense?

@rowanwins rowanwins added this to the 7.0.0 milestone Jan 11, 2022
@pietervdvn
Copy link
Author

The old behaviour might be better, but I do not have the time nor experience to fix it.

@rowanwins
Copy link
Member

I've done some looking at this. polygon-clipping doesn't actually return the full line that intersects along the edge, it's returning duplicate points. And it also doesn't return a point where corners intersect.
So it's a bit more problematic than a simple fix and will need some more thought :(

@rowanwins rowanwins marked this pull request as draft January 13, 2022 09:50
@smallsaucepan smallsaucepan modified the milestones: v7, v8 Dec 9, 2023
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

Successfully merging this pull request may close these issues.

None yet

5 participants