-
Notifications
You must be signed in to change notification settings - Fork 943
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/buffer] Fix projection issues that produce undersized buffers at non-equatorial latitudes #1956
Conversation
The existing buffer code has three problems: - it uses two different projections depending on the latitude of the input geometry. This results in an error discontinuity at 50N. - it switches projections only for the northern hemisphere. - it mistakenly reverses the lonlat center, which has the eventual result of rotating the projection to a skewed center. The third problem is one of the causes of `buffer`'s size issues. At latitudes above 50N, the projection distorts input distances due to not being rotated to the correct input geometry center. At latitudes below 50N, the Mercator projection does not receive a correction for the input distance. If using the Mercator projection, the actual distance used as an input to JSTS BufferOp should be distance / cos((lat * π) / 180). [1] As recommended in the original[2] exploration that prompted the fix introduced in Turfjs#938, this patch switches the buffer prep code to consistently using the Azimuthal Equidistant projection rather than switching between Mercator and Transverse Mercator. When centered on the input geometry, this projection should produce less distortion in all directions for input geometries of reasonable size compared to a cylindrical one. I took the opportunity to optimize slightly; buffering now calls `center` only once, and we don't need to `bbox` the input geom at all. [1] https://gis.stackexchange.com/a/347573 [2] https://github.com/w8r/moscow-rings#solution
tagging @rowanwins @stebogit as people with some context |
Thank you for this fix! Tried a couple of tests with my project and the distance projections are so much better. If I use the index.js file from dist/ then everything works great, but turf.min.js throws this error when trying to use buffer: It's difficult for me to debug it because of the minified file, and the non-minified file doesn't error. I don't even know if this is a bug with your pull request or my environment, but could you please test that for me? |
Thanks for checking it out, @mattzucker. I'm seeing the same issue and can confirm that it's something to do with my change - works fine in |
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.
This looks really great thanks @dyakovlev - sorry it's taken so long to review!
The code is much simpler to follow AND produces more consistent and more accurate results!
For the https://github.com/d3/d3-geo/blob/master/src/projection/azimuthalEquidistant.js#L14 |
Do you think there's a tree shaking issue of some sort? Is there any followup I can help with here? Happy to spend some cycles in turf. |
I'm going to go ahead and just upgrade our whole rollup setup from 0.55.5 to 2.34.2 and hope that fixes the issue. That's nearly 3 years of rollup development so I'm hoping its just magically fixed once I'm done. Its been on my list of improvements to make but I haven't had a compelling reason to do it immediately until now. If that doesn't fix it I'll re-ping you though. I really appreciate the help, when I joined the Turf project my goal was to do a lot of build and infrastructure work, so some of the packages are still a mystery to me. I'm glad you were able to make such a great fix. |
@dyakovlev the upgrade seems to have done the trick, you're off the hook this time :) |
Hi folks, my company recently had an issue of undersized buffers reported that prompted me to spend some quality time in Turf and JSTS source. It seems that this has been an issue reported a number of times (#1246, #1470, #1484, #1547). I think I've found the underlying issue to be a misconfiguration in the projection used in the
buffer
implementation. I hope this can help!npm test
at the sub modules where changes have occurred.npm run lint
to ensure code style at the turf module level.The existing buffer code has three problems:
The third problem is one of the causes of
buffer
's size issues. At latitudes above 50N, the projection distorts input distances due to not being rotated to the correct input geometry center. At latitudes below 50N, the Mercator projection does not receive a correction for the input distance. If using the Mercator projection, the actual distance used as an input to JSTS BufferOp should be distance / cos((lat * π) / 180). [1]As recommended in the original[2] exploration that prompted the fix introduced in #938, this patch switches the buffer prep code to consistently using the Azimuthal Equidistant projection rather than switching between Mercator and Transverse Mercator. When centered on the input geometry, this projection should produce less distortion in all directions for input geometries of reasonable size compared to a cylindrical one.
[1] https://gis.stackexchange.com/a/347573
[2] https://github.com/w8r/moscow-rings#solution
Question - as mentioned in the contributing guidelines, I regenerated READMEs using the provided script. This produced a lot of diffs to links and looked like either a tool misconfiguration or drift in some underlying system. I've chosen to omit this for the time being; was that the correct choice?
Validation - I've visually checked the generated output geojson. Wondering if there's an automated test that would be reasonable to implement - maybe construct a point grid, buffer all of the points, verify their radii to be within some ε of the input distance?