-
Notifications
You must be signed in to change notification settings - Fork 961
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-destination strips altitude/elevation member from origin coordinate. #2850
Comments
Thanks for raising this @prozvora. Turf hasn't traditionally used altitude in its calculations, so I reckon you'll find several more places where that third coordinate is dropped. Even though starting to use altitude is probably out of scope for the foreseeable future, we should at least pass it through unchanged where that it makes sense to. Bearing that in mind, is this something you'd be able to look at a PR for? Adding tests, making sure it doesn't unintentionally impact other packages? Fwiw, I suspect it would actually help in some cases e.g. #2672 That might be more than the four lines you mention above though so wanted to ask first if you had the bandwidth to take a deeper look. |
I know it affects destination and ellipse. I can add make a PR from my fork and add tests on those packages. I'll also take a look at #2672. @smallsaucepan If there are any other packages you think may be affected, please just list them and I can add test cases for them too. I haven't used the entire turf repo extensively. Thanks! |
Thanks for putting that PR together. Have found a few questions. Currently point() does a runtime check to make sure the elements it returns are numbers. This hasn't been an issue as Turf more or less ignored altitude and made no promises about what was coming back in that third position. We're now saying Turf does at least recognise altitude, so we should make sure we aren't returning values that violate the spec. That would mean expanding the existing runtime check to look at the third element as well. However, that could break existing code if someone has been (unwisely) passing non-numeric data in that field. Any ideas on how we could handle that? The other question is about tesselate. Looking at the spec it seems possible to have some positions with altitude defined and some without. What's the expected behaviour if there are a mixture? Is there a strategy that would make the result predictable and backwards compatible? For example, do we call earcut with 3 dimensions all the time, and trim each resulting point individually if altitude is undefined? |
Sounds reasonable about the checking, so long as we do mention the limitation in the docs for now. Will re-look at tightening up the runtime check for the next major release (i.e. v8). On the "only some points might have elevation values" take this square: [
[1, 1],
[1, 2, 50],
[2, 2, 75],
[2, 1],
[1, 1],
] According to the spec this is valid. Each of the individual positions has two or three numeric values. The current elevation test in the PR only checks the first position though. As is it would strip the elevations from all points. I'm guessing what we should return is these two triangles: [
[1, 1],
[1, 2, 50],
[2, 2, 75],
[1, 1],
] and [
[1, 1],
[2, 2, 75],
[2, 1],
[1, 1],
] Neither losing elevation where it was defined, nor adding it where it wasn't. So, to make sure we respect elevation, we either need to:
|
I think 1 is the most intuitive from a user perspective, and coincides with a "pass through" philosophy for that coordinate element. I can take a stab at implementing that for tessellate, if that wouldn't increase the scope of my PR too much. Or I can split out tessellate changes into a separate PR, since the destination/ellipse changes don't involve any further manipulation of the elevation coordinate member. |
Note that options 1 and 2 were identical from the user's perspective. Was pointing out it might not be worth checking first - just do it the same way all the time. Good idea. Let's keep this PR focused on ellipse. Then we can have a look at the issue more broadly and do some experiments to see what might be the best approach. |
The
destination
function does not return all members of theorigin
parameter.https://github.com/Turfjs/turf/blob/master/packages/turf-destination/index.ts#L40
Note the return value creates a Position array with
[lng, lat]
, but the origin parameter may have a third element, which should be carried over.https://github.com/Turfjs/turf/blob/master/packages/turf-destination/index.ts#L70
GeoJSON specifies that a Position may have a third element, representing altitude or elevation.
https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.1
This also affects
turf-ellipse
, which usesdestination
in its conversion of an ellipse to a polygon.https://github.com/Turfjs/turf/blob/master/packages/turf-ellipse/index.ts#L151
The text was updated successfully, but these errors were encountered: