Add gpx support #9152
Add gpx support #9152
Conversation
|
Thank you so much for the pull request @jtorresfabra! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
|
There was a problem hiding this comment.
This is a great start @jtorresfabra ! I've left some notes on fixes, and I think I may have found something that may have been missed in the original PR review (whether GPX needs to resolve external URLs). A few more things:
- Should we support clamp to terrain/height reference?
On one hand, GPX data provides elevation relative to the ellipsoid, so the position in 3D should be rendered as-is to be "correct". On the other hand, a lot of GPX data is things moving on the ground, and even the sample files in the Sandcastle aren't quite correct with Cesium World Terrain (they're slightly underground). I've noticed this with my own GPS data, that some points are just going to be underground due to inaccuracies.
I think it shouldn't be hard to do something similar to GeoJSONDataSource where you can pass an option to clamp to the ground, or pass a height reference, so it'd allow you to clamp the waypoints/path to the ground or make them relative to the ground. The default can be HeightReference.NONE to leave the data as-is.
The Sandcastle example should use Cesium World Terrain.
- We should double check the license on the sample GPX data used here and add the license to https://github.com/CesiumGS/cesium/blob/master/LICENSE.md#example-applications.
See this comment on where the data comes from #2939 (comment). We can consider simplifying it and using fewer GPX files from one source if you can find any ones you think work better as examples.
- Test coverage seems low, see: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/feature/rebased-gpx/Build/Coverage/Firefox%2080.0%20(Linux%20x86_64)/DataSources/GpxDataSource.js.html.
I also haven't tested what happens with an invalid GPX file/if errors are reported correctly etc.
|
I've added you to Cesium GS's corporate CLA. Don't forget to add yourself to CONTRIBUTORS.md under Cesium GS! |
|
@OmarShehata thanks for the review! I tried to address all the comments. But there are still some concerns:
2 & 3) I'm going to look for better data and implement more tests. |
jtorresfabra
force-pushed
the
feature/rebased-gpx
branch
from
a4c8cec
to
77d446b
Compare
|
Do you have any timeline / plan to review / merge it? Thanks a lot in advance! |
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
3 similar comments
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
2 similar comments
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @jtorresfabra! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
|
@mramato Do you have the bandwidth to review this PR? If not, could you recommend someone else who may be well suited for final review? |
|
@eleu This PR fell off our radar but we're close to getting this feature in. We'd appreciate any feedback or sample data you may have. |
There was a problem hiding this comment.
Overall this is looking great to me! It would be great if we could have a member of the community who's familiar with GPX take a look at this for the sake of completeness.
Not a blocker to this PR, but I'm just noting that this doesn't support the style specification, as I've seen some other plugins and libraries which do support.
@sanjeetsuhag Can you please update CHANGES.md?
|
Also please merge in |
|
Thanks @sanjeetsuhag and all those involved! I think this is in a good place and we're ready to merge, exciting! |
This PR adds support for GPX loading. It's based off this old PR #2939.
Test files for waypoints, tracks and routes have been added, there is also a SandCastle with a dropdown menu for different gpx files here: http://localhost:8080/Apps/Sandcastle/index.html?src=GPX.html. ( test it online )
On top of the rebasedPR we added some symbology options: clampToGround, trackColor and waypoint and track images. Also path symbols have been removed in favor of polyline symbols. The main reason is that path symbols does not support clamp to ground so it was done for the sake of consistency within the symbology options.