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

Replaced polygon-clipping with polyclip-ts #2729

Merged
merged 7 commits into from
Dec 22, 2024

Conversation

cmurphy23
Copy link
Contributor

@cmurphy23 cmurphy23 commented Oct 9, 2024

This commit replaces the polygon-clipping library with polyclip-ts, a goal mentioned in several issues (#2409, #2277, #2048), with even more issues related to polyon-clipping problems that don't mention polyclip-ts (#2705, #2420, #1983, and others).

This PR is a placeholder, dependent on this PR for polyclip-ts so that the polyclip-ts library doesn't fail the esModuleInterop check. Once those changes have been merged and released, the polyclip-ts version just needs to be incremented in the different modules' package.json files, then turf should should build and the tests should pass.

The affected packages are: intersect, difference, union, dissolve, and mask

It is worth noting that I am not an expert on these algorithms, and in the cases where the test results differed, I simply updated the tests with the new results. The vast majority of the differences were just floating point rounding differences, but there were different results for two turf-difference tests. For the test "issue-721", two polygons are returned for the difference instead of one, where the first polygon is basically identical to the original answer, but a new very small polygon on the border of the polygons has also been found and returned. For "issue-721-inverse", instead of no difference being found, three very small polygons have been found on the borders. I don't actually know what the correct answers are since the test case is truly an edge case and higher precision might produce a different result. However, based on the issues I've mentioned, polyclip-ts seems like the more reliable repo, turfjs is regularly having issues with these operations using polygon-clipping, and it would be nice if it didn't.

Resolves #2409
Resolves #2277
Resolves #2048
Resolves #2705
Resolves #2420
Resolves #1983
... and others (TBC)

Allows truncate workaround to be removed from the following already resolved issues as well:
Resolves #2479
Resolves #2306
Resolves #2601

  • Is this a bug fix, new functionality, or a breaking change?
    I would argue it's closest to a bug fix.
  • Have read and followed the steps for preparing a pull request.
    I followed the steps, just need polyclip-ts to update and release, then this code will work.

@smallsaucepan
Copy link
Member

Really appreciate you putting this together @cmurphy23. Take there is no workaround for the esModuleInterop issue that might let us get this merged (and those issues resolved) sooner?

Also understand your note about the tests. Anything that looks like more than a rounding change, we can go through together, perhaps with one of the other maintainers.

@cmurphy23
Copy link
Contributor Author

No problem, @smallsaucepan, glad to be able to contribute.

The PR for polyclip-ts got merged and a new release was made. This PR now references the new release and should be considered for merging. Thanks!

@smallsaucepan
Copy link
Member

Hi @cmurphy23. Had a go at building your branch and ran into the following error during in turf-intersect. Are you seeing this too?

index.ts(10,27): error TS7016: Could not find a declaration file for module 'polyclip-ts'. '/Users/james/Code/turf/cmurphy23/node_modules/.pnpm/[email protected]/node_modules/polyclip-ts/dist/index.js' implicitly has an 'any' type.
  There are types at '/Users/james/Code/turf/cmurphy23/packages/turf-intersect/node_modules/polyclip-ts/types/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'polyclip-ts' library may need to update its package.json or typings.

Running polyclip-ts through arethetypeswrong does suggest there's a problem with its packaging:

Screenshot 2024-10-26 at 3 34 23 PM

We've fixed problems like these within Turf, so I might submit a PR to polyclip-ts. Wanted to confirm first though that I'm not the only one seeing this error.

@smallsaucepan
Copy link
Member

Created an issue luizbarboza/polyclip-ts#18 to follow up on a possible fix at the polyclip-ts end.

@cmurphy23
Copy link
Contributor Author

Hi @smallsaucepan, I could have sworn the tests were passing, but I must have had some cached file or something, because I tried with a clean repo and the tests failed like you said. Sorry for the mistake. Thanks for making the issue on polyclip-ts, I'll see if I can be helpful over there.

@cmurphy23
Copy link
Contributor Author

Actually, I had updated the exports in the package.json, but didn't put them in my PR because the repo seemed to already have the change. With an exports that looks like this:

    ".": {
      "import": {
        "default": "./dist/index.js",
        "types": "./types/index.d.ts"
      },
      "require": {
        "default": "./dist/index.js",
        "types": "./types/index.d.ts"
      }
    } 

Like this the tests pass and the repo seems to work, but it seems that the import is being resolved after failing some initial resolution.
image

@smallsaucepan
Copy link
Member

All good. Found out the exports PR was rolled back the day after your PR was merged. Just a quirk of timing 🤷

Will give the new issue a week for any testing feedback, then put forward a PR.

@smallsaucepan
Copy link
Member

@cmurphy23 FYI just followed up over on the polyclip issue for some feedback on what a successful PR might look like.

@smallsaucepan
Copy link
Member

Hi @cmurphy23. I think the issues with polyclip-ts packaging are just about resolved. Once 0.16.8 is released we should be good to go. Will you still have some availability to retest and prepare to merge?

@rogup
Copy link

rogup commented Dec 18, 2024

Hi @smallsaucepan @cmurphy23 I just saw that polyclip-ts v0.16.8 has been released. Do you know how long it will roughly take for this to make it into a Turfjs release? If there's anything I can do to progress this, I'm happy to help, but I haven't worked on this repo before.

@smallsaucepan smallsaucepan changed the title replace polygon-clipping with polyclip-ts Replaced polygon-clipping with polyclip-ts Dec 22, 2024
…d a bunch of verification test cases based on problematic example data in related issues.
@smallsaucepan
Copy link
Member

Hi @cmurphy23. Pushed a few changes now that the 0.16.8 polyclip-ts is published. Also added a few verification tests based on the linked issues. Will give it another once over and hopefully approve to merge.

Copy link
Member

@smallsaucepan smallsaucepan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a few empty polygons were added in the case of difference/test/out/issue-#721 and issue-#721-inverse. Proceeding as 721 seems an odd example given we're diffing zero area(?) polygons.

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