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

@turf/boolean-contains - support for multipolygon inside polygon #2357

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MartinP-C
Copy link

@MartinP-C MartinP-C commented Oct 31, 2022

Add support for feature2 being a MultiPolygon inside of a Polygon

  • 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.

@MartinP-C
Copy link
Author

Adds support for the inverse of #2337 (A multipolygon completely contained by a polygon)

@nicolasperez19
Copy link

@rowanwins hi! When is this feature going to be released? Merging this feature would help solve #1915 and help answer this comment.

@smallsaucepan
Copy link
Member

Hi @MartinP-C. Know it's been a while since you submitted this. We're working to get some old PRs merged. Are you still on board to see this one through with some assistance?

@MartinP-C
Copy link
Author

Hi @smallsaucepan , absolutely. What is needed?

@smallsaucepan
Copy link
Member

Thanks @MartinP-C. Wondering a couple of things:

Given booleanContains and booleanWithin are two sides of the same coin, should we expand the scope of the PR to make sure they're aligned? A quick look at the multipolygon tests in within shows some are being skipped so there might be some gaps there too.

Secondly, if the above is true, I've noticed contains and within share a lot of very similar (though not identical) internal functions e.g. isPointInMultiPoint or isPolyInPoly. Would you mind seeing if this is an opportunity to pull the logic into a common module rather than having duplicated code?

Possibly more than you signed on for 😅 though keen to introduce consistency and reuse where we can.

@MartinP-C
Copy link
Author

MartinP-C commented Nov 10, 2023

It is a bit more but I'm up for the challenge :)
So:

  • unskip tests for booleanWithin and update
  • ensure same feature types for feature1/2 and feature2/1 are able to be checked via booleanContains and booleanWithins (and ensure there are tests covering these)
  • share common code where possible

Where should common code go?
I notice in some packages they share from the helpers package and in others it is from a package itself.
so, for example, should isPolyInPoly become it's own package?
or just exist within booleanContains (or Within) and import it from there?

@smallsaucepan
Copy link
Member

Thanks @MartinP-C. That all sounds great.

Regarding your question about where to put common code, have started discussion #2537. Perhaps jump in there first and we can work through an approach.

@masch
Copy link

masch commented Mar 4, 2024

@MartinP-C @smallsaucepan Hi! Can I help with anything to upload this feature? It'll be great to have it

@g94giulio
Copy link

Hi! Any update about this feature? It'll be great to have it!

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

Successfully merging this pull request may close these issues.

5 participants