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

GSoC 2015: Initial GPX support #2939

Closed
wants to merge 45 commits into from
Closed

Conversation

andr3nun3s
Copy link
Contributor

I'm opening this PR right now to facilitate code reviewing. Most of the visual stuff is done.

There's still some things I need to work on, see below.

What's missing:

  • Better test coverage

Please review so we can get this to a decent state ASAP

@andr3nun3s andr3nun3s mentioned this pull request Aug 14, 2015
18 tasks
@mramato
Copy link
Contributor

mramato commented Aug 19, 2015

It looks like in some cases the waypoints get the correct elevation (but the track stays on the ground) and in other cases the opposite happens (the track in the air and the waypoints are on the ground) It might just magically get fixed when you add the dynamic track code, but I wanted to let you know it was happening. The sample file I just emailed you shows it pretty well.

@andr3nun3s
Copy link
Contributor Author

Does course mean yaw? As in roll-pitch-yaw.

These are not on the official GPX schema, I guess those are extensions but I could look into implementing these as well.

That elevation bug is strange, I'll see if I forgot something related to coordinate reading

@andr3nun3s
Copy link
Contributor Author

About the elevation, on the sample file you've emailed me the waypoints don't have an elevation tag, so it is set to 0 as default. The track points (trkpt) have elevation, that's why the track floats in the air while the waypoints stay on the ground... I believe that's working as intended

@mramato
Copy link
Contributor

mramato commented Aug 19, 2015

Does course mean yaw? As in roll-pitch-yaw.

They are basically the same thing, but everyone has subtle differences, Cesium uses heading/pitch/roll. It looks like this data is specifically coming from ardupilot, which is an open source drone platform. I have a feeling it's pretty popular. If it's easy to add support for these fields, let's do it (we can always change it later). If you run into problems, then we'll skip it for now.

I believe that's working as intended

Okay, Google Earth has similar results, so I guess it's just a data problem. I'll probably play around with it at some point and see if we can improve anything.

Let me know when the dynamic tracks are ready to look at.

Do I have commit access to your fork? If not, would you mine giving it to me? I might make some final tweaks when we're done before things get merged so I want to have it just in case.

@andr3nun3s
Copy link
Contributor Author

I gave you commit access.

My main issue with dynamic tracks is that in GPX the time tag is not mandatory so we can get data that has an uneven number of points and timestamps. I was thinking about simplifying this:

If all track points have a timestamp (# positions = # timestamps), the track is time dynamic.
else
if you can't match all track points with a timestamp (# positions != # timestamps) the track shouldn't be time dynamic. This is what is implemented right now and I'd need to implement the time dynamic case.

What do you think?

@mramato
Copy link
Contributor

mramato commented Aug 19, 2015

That sounds like a good strategy to me.


//trk represents a track - an ordered list of points describing a path.
function processTrk(dataSource, geometryNode, entityCollection, sourceUri, uriResolver) {
var entity = getOrCreateEntity(geometryNode, entityCollection);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mramato, could you take a look at this function (processTrk)? Not sure why time dynamic tracks aren't working yet. What am I doing wrong?

You can use exampleTrk.gpx as an example of a time dynamic track.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have entity.positions instead of entity.position on line 508.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also want to make sure that there's a default billboard for time-dynamic entities, so you see more than just the track.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the time-dynamic track seems to be working now!

@andr3nun3s
Copy link
Contributor Author

Hey @mramato

I just added more tests, I think that test coverage is pretty good right now, can you look into it and tell me if I missed something?

Other than that what is missing for us to wrap this up?

Thanks

return undefined;
}
var hrefResolved = false;
if (defined(uriResolver)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uriResolver is a left over from the KML code you copied, it's not actually used anywhere in this file and should be removed from all of the function signatures (along with this defined block). This also means that the hrefResolved bool can be removed as well.

@mramato
Copy link
Contributor

mramato commented Aug 21, 2015

getPerson, getEmail and getLink all appear to be missing unit tests.

@andr3nun3s
Copy link
Contributor Author

Done, managed to find and fix a couple of bugs while adding the missing tests. Thanks a lot!

@mramato
Copy link
Contributor

mramato commented Aug 21, 2015

That's exactly why we write the tests 😄

@andr3nun3s
Copy link
Contributor Author

Also, I noticed that there is course/pitch/roll information. You should be able to use SampledProperty(Quaternion) to handle these and assign it to entity.orientation course is just the heading and you can use Transforms.headingPitchRollQuaternion to create the quaternions. I have a sample file I'll email you offline to code with.

Looking at heading/pitch/roll right now. A few questions: ´
I have to compute a quaternion for each track point, where do I assign these quaternions? One SampledProperty(Quaternion) per quartenion but where do I add these samples? Do I use the same SampledPositionProperty I used for the times and coordinates of the points?
If you could point me to any example I would be grateful.

I'm also assuming this orientation info is only relevant for time-dynamic tracks.

@andr3nun3s
Copy link
Contributor Author

I have this code (simplified to omit checks for defined,etc) to process the orientation on a single track point:

function processOrientation(node){
        var heading = defaultValue(CesiumMath.toRadians(queryNumericValue(node, 'course', namespaces.gpx)), 0.0);
        var pitch = defaultValue(CesiumMath.toRadians(queryNumericValue(node, 'pitch', namespaces.gpx)), 0.0);
        var roll = defaultValue(CesiumMath.toRadians(queryNumericValue(node, 'roll', namespaces.gpx)), 0.0);
        var center = Cartesian3.fromDegrees(0.0, 0.0);

        var quartenion = Transforms.headingPitchRollQuarternion(center, heading, pitch, roll);
        var orientationProperty = new SampledProperty(quartenion);
       ...
}

What should the default value be for each one of heading/pitch/roll and what do I do with the sampled property afterwards

@mramato
Copy link
Contributor

mramato commented Aug 21, 2015

You assign it to entity.orientation and just use 0 for the default. However, if none of the properties exist, I wouldn't create a SampledProperty at all, since most data won't have them and it wastes memory.

@andr3nun3s
Copy link
Contributor Author

Hey @mramato, what's needed to merge this? It will be hard for me to get orientation in before next weekend

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 14, 2017

Does anyone in the community have the bandwidth to take this over the finish line?

Twitter fame and Cesium swag await! 🥇

@mramato
Copy link
Contributor

mramato commented Oct 20, 2017

I merged this into a new gpx and updated it to match master.

I'm going to close this PR but anyone interested in taking this over the home stretch should use gpx as a starting point. Depending on my schedule, I may ever try and do it myself since this is probably 98% complete as is.

Thanks again for the original work @Andre-Nunes!

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

Successfully merging this pull request may close these issues.

3 participants