-
Notifications
You must be signed in to change notification settings - Fork 992
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. |
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!).