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/ellipse uses overly simplistic sampling method #1767

Closed
cmbasnett opened this issue Oct 9, 2019 · 3 comments · Fixed by #2739
Closed

@turf/ellipse uses overly simplistic sampling method #1767

cmbasnett opened this issue Oct 9, 2019 · 3 comments · Fixed by #2739
Assignees

Comments

@cmbasnett
Copy link

cmbasnett commented Oct 9, 2019

The current sampling method for generating an ellipse in turf is to simply take samples at regular angle intervals (essentially identical to the @turf/circle implementation). This results in ugly and stretched ellipses where the difference between the length of the semi-major axes is appreciable. I propose that it should use arc-length intervals instead, as this results in a more natural representation of the ellipse.

Here is a gist of a drop-in replacement of @turf/ellipse that does arc-length interval sampling. You are also able to specify an accuracy value to tune performance vs. accuracy.

https://gist.github.com/cmbasnett/b62ea614e7b05f713b03835b3e93d4d3

Here's a jsfiddle with a side-by-side comparison of the two methods, highlighting the weaknesses of the current implementation (left: current, right: new hotness). http://jsfiddle.net/hkejm20t/88/

Screen Shot 2019-10-09 at 12 58 58 PM

@rowanwins rowanwins self-assigned this Jan 2, 2020
@rowanwins
Copy link
Member

Thanks for the contribution @cmbasnett , I'll incorporate this into the v7 branch

@guy-baron1
Copy link

Any news on this? Love the library but the current ellipse function is just not good enough for our needs

@icemagno
Copy link

How about rotation?

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

Successfully merging a pull request may close this issue.

6 participants