-
Notifications
You must be signed in to change notification settings - Fork 991
WIP migrate @turf/buffer to TypeScript #2991
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
base: master
Are you sure you want to change the base?
Conversation
|
FWIW this would be amazing if it works out for both the buffer and the clipping operations! On the off chance if there is any support you would need for this, let me know. |
|
Spent some time taking a look at just the intersect operations today. Couple of insights if you haven't got there already:
For reference the aforementioned benchmarks: Current 7.3 polyclip-ts: My clipper replacement (but as noted above, anything final will be slower than this: |
|
re: winding order, I think it still needs the reversal step because latitude is inverse of the cartesian Y axis. re: integer vs float64, I intended to use the float64 variants before but got it wrong in the first commit. I added the D suffix today to all of the calls. My output fixtures are now visually the same (except I think clipper2 adds more points along the rounded sides with clipper's default settings). I also had to add handling for invalid winding orders as seen in polygon-with-holes's inner ring. turf.min.js's linting does complain about the bigint syntax, which I think I'm not transitively using, but gets retained by the build instead of being treeshaken. We may have to do a major rev to be able to change that to some newer syntax (probably es2022). Perhaps when we do this, we just go ahead and document that we may roll forward and use any syntax that caniuse declares as "baseline" or another metric that we can use with babel's preset-env. |
|
Winding order HOWEVER - given that you are also projecting in your code the projection (which, apropos of the quote, is indeed from a graphics library :-)) it is the projection that does as you say and inverts the y and therefore requires the winding reversing that you are doing. I was playing with clipping, so was not projecting, hence experiencing the opposite. So in summary - clipper and geojson wind the same, but clipper and a d3 geoAzimuthalEquidistant projection wind opposite. Therefore - no reversal required when using geojson, reversal is required when running through a projection. Integer vs Float I think this means that the bigint syntax will be a requirement for clipper, so definitely major release, but as you point out it has been baseline for ages. Won't comment on the broader policy question. |
…s on closed linestrings
For the es5 compatible cdn version we have (I think) been able to transform and polyfill the bigint code in #2997. And if someone is importing the individual package esm / cjs directly into a web page, any browsers not supporting bigint will be under the > 0.25% / not dead criteria. For the node side of things, I think we're on safe ground to say we can start depending on a library that utilises ecmascript bigint. Node18 supports that at runtime, and that's the oldest node version we support. |
| - hi64: Number(res >> 64n) | ||
| - }; | ||
| + lo64: JSBI.toNumber(JSBI.bitwiseAnd(res, JSBI.BigInt("0xffffffffffffffffn"))), | ||
| + hi64: JSBI.toNumber(JSBI.signedRightShift(res, JSBI.BigInt(64))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at this, I'm kind of skeptical that we can safely do lo64, hi64 without loss of precision, because Number can only contain 2^53 – 1. 🤔
| "@turf/unkink-polygon": "workspace:*", | ||
| "@turf/voronoi": "workspace:*", | ||
| "@types/geojson": "^7946.0.10", | ||
| "jsbi": "^4.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be a devDependency? And its a touch sketchy to only do this here because we're actually using the patched version of clipper2-ts in the @turf/buffer tests. That means that consumers would be the first folks to test this with the real BigInt syntax.
|
@smallsaucepan curious for your take on how you like the pnpm patch for clipper2-ts (most recent commit on this PR). I think this is probably simpler than trying to use the babel transform since it didn't work out of the box. Edit: Its probably worth noting that I generated the |
Instead of messing around with @turf/jsts needing types, there's actually a new entrant into the underlying library space that solves a lot of problems at once.
clipper2-ts seems pretty exciting for us. I happened to find it because of our desire to drop our jsts dependency, but it also has intersect, union, difference, xor. So perhaps we can rework the other packages to depend on this library (polyclip-ts is slow with its bignumber implementation, and we'd have to fork it if we wanted to drop that). Because it's a port of a mature C library, it already comes with a ready-made test suite.
In the current state of this PR, the test suite fails to run, and 3 of the output fixtures look weird/broken (issue#783, negative-buffer, polygon-with-holes), but I'm pretty sure this is just me not understanding how to use the library.
Here's some numbers I grabbed with the work in progress version. Dropping @turf/jsts saves us ~26 of the total turf.min.js size, and it benchmarks faster (sometimes by a lot!).
// Note: the above benchmarks were done with native BigInt, and not the patched JSBI BigInt implementation. I need to redo the benches before merging this.
Fixes #2929, #2920, #2895, #88, #2522, #2701
Fixes #2469 (Although I'm not really convinced that we create a usable polygon at the pole)