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

Conversation

@andre-nunes
Copy link
Contributor

commented Aug 14, 2015

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

@andre-nunes

This comment has been minimized.

Copy link
Owner Author

commented on Source/DataSources/GpxDataSource.js in 0cd33cb Aug 2, 2015

@mramato, can you take a quick look at this cycle? I went for maintainability but fear I've sacrificed readability too much.

Initially I had something like this:

var description = defaultValue(queryStringValue(...));
if(defined(description) && description !== ''){
    description = '<p>' + 'Description: ' + description + '</p>'; 
}
var source = ...
...

Repeating this for all the tags, it obviously doesn't scale so after much thought I managed to implement the current solution, let me know which way you find better

Thanks.

This comment has been minimized.

Copy link
Collaborator

replied Aug 4, 2015

This solution is definitely the way to go, we have similar code in GeoJsonDataSource to do a similar thing.

@andre-nunes andre-nunes referenced this pull request Aug 14, 2015
16 of 18 tasks complete
@mramato

This comment has been minimized.

Copy link
Member

commented Aug 19, 2015

Okay, time dynamic tracks should be easy enough with the SampledPositionProperty. For each time/position pair you just call addSample.

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.

@mramato

This comment has been minimized.

Copy link
Member

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.

@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2015

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

@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2015

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

This comment has been minimized.

Copy link
Member

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.

@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2015

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

This comment has been minimized.

Copy link
Member

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);

This comment has been minimized.

Copy link
@andre-nunes

andre-nunes Aug 19, 2015

Author Contributor

@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.

This comment has been minimized.

Copy link
@mramato

mramato Aug 19, 2015

Member

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

This comment has been minimized.

Copy link
@mramato

mramato Aug 19, 2015

Member

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.

This comment has been minimized.

Copy link
@andre-nunes

andre-nunes Aug 19, 2015

Author Contributor

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

andre-nunes added 3 commits Aug 19, 2015
@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2015

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)) {

This comment has been minimized.

Copy link
@mramato

mramato Aug 21, 2015

Member

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.

return Cartesian3.fromDegrees(longitude, latitude, height);
}

function readCoordinates(element) {

This comment has been minimized.

Copy link
@mramato

mramato Aug 21, 2015

Member

readCoordinates and readCoordinate are no longer used and should be removed.

@mramato

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

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

andre-nunes added 2 commits Aug 21, 2015
@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2015

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

@mramato

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

That's exactly why we write the tests 😄

@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2015

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.

@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2015

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

This comment has been minimized.

Copy link
Member

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.

@andre-nunes

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

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

@pjcozzi

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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
Projects
None yet
3 participants
You can’t perform that action at this time.