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

do SORT_BY_TIMESTAMP only for tracks with timestamps #52

Merged
merged 1 commit into from
Feb 28, 2021

Conversation

lukey78
Copy link
Contributor

@lukey78 lukey78 commented Feb 28, 2021

There are GPX tracks with all data except timestamps, e.g. when users create GPX files in a planning tool.

Currently, analyzing these tracks crashes if the SORT_BY_TIMESTAMP option is set (actually, there seem to be tools outside that do not pre-sort the points by timestamp).

This merge request checks if the first point has a timestamp. If not, the sorting is not done.

@Sibyx
Copy link
Owner

Sibyx commented Feb 28, 2021

Hi @lukey78,
first of all thank you for your contribution. I was also skeptical about the pre-sorting track points, but track is suppose to be an ordered set of points (according to the spec, but it's not specified how). This behavour could be disabled by default in next major release (but still not sure about it).

I am not sure if checking value of the first point is enough. I came up with two proposals:

  1. We can introduce some kind of private property of segment or track, which will tell us, if there is a point without time property inside of container
  2. We can check if there is a point without time before the sort (but it could be a performance issue in larger files)

Looking forward to your opinion, I am open to the discussion. Your implementation is still improving stability so PR will be merged.

@Sibyx Sibyx added the bug label Feb 28, 2021
@lukey78
Copy link
Contributor Author

lukey78 commented Feb 28, 2021

Yes, I'm aware it does not prevent all cases.
I use it with an app where several thousand GPS files have been uploaded from many different sources. I stumbled upon two or three cases where tracks were not sorted by timestamps, so I activated the Sort option. Then, of course, importing the tracks with no timestamps brought ab the error. Tracks with no timestamps at all are quite common.

But I didn't encounter tracks where only some timestamps were missing, although the file format allows it.

Thanks for merging, then I can switch back to the official repo in my project.

@Sibyx Sibyx merged commit a0185f4 into Sibyx:master Feb 28, 2021
@Sibyx
Copy link
Owner

Sibyx commented Feb 28, 2021

@lukey78 1.1.2 was released. Enjoy 🚀

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

Successfully merging this pull request may close these issues.

2 participants