[Patch] GPX layer -> data layer convert does not simplify track

[anonymous]

Many JOSM users load their GPX track log, convert it to a data layer, and then add it to OSM as a way. The resulting ways have many data points and are noisy. Potlatch has an automated solution to this problem in its conversion function: (From ​http://potlatchosm.wordpress.com/category/tips/ )

"Converting GPX tracks directly to ways can have a pitfall: if you’ve recorded a trackpoint every second, that’s a lot of unnecessary points on long, straight journeys. So Potlatch does a couple of things to avoid this. When you click ‘Track’, it “simplifies” the track to remove all the unnecessary points on straight lines. (For the curious, this is done using the Douglas-Peucker algorithm.)"

JOSM has a similar filter in the "utilsplugin" plugin, but it requires a user to a) know that it exists and b) install it and c) remember to call it on imported data.

I suggest that a "simplify" function is incorporated in the JOSM gpx->data conversion function, and that it is enabled by default.

[Don-vip]

@Bjoeni: what do you think about adding simplification to the GPX->OSM conversion options dialog?

[Bjoeni]

I probably wouldn't use it, because I usually simplify the lines once I merged them or they are ready to be merged, with the max-error value depending on the type of the way and the noise of the GPS track in that region.

But I agree that this could make it easier for new users and since it is in the core by now (for just 8 years :D) it shouldn't be a big deal to implement that.

I am however travelling at the moment and can't patch anything (or I could... but its not much fun if this netbook takes 30sec for AutoComplete, 1min to find a declaration, 3min to find references and 10min to build the project :P)

But if you / someone else wants to implement that, I have no objections.

[Don-vip]

Replying to Bjoeni:

its not much fun if this netbook takes 30sec for AutoComplete, 1min to find a declaration, 3min to find references and 10min to build the project :P)

Haha I have the same problem when traveling :) I'll wait your comeback :)

[Bjoeni]

See the other ticket, I'll probably be back around February. Considering this ticket is 9 years old, that doesn't make much of a difference anymore - but if you don't want to wait that long you can do it ;)

[Bjoeni]

I implemented that, however the problem is that simplify obviously doesn't do anything if the nodes have tags from the conversion.
Two options:

• Only show the dialog when no tags have been converted
• Consider the GPX tags "uninteresting". They would have to be put in a namespace like gpx:time and gpx:ele during conversion, because obviously we can't deem something like plain "ele" uninteresting. I just gave that a quick try and it seems to work quite well, that would also mean that the user wouldn't be bothered by remaining nodes when deleting a line anymore and that we could create a validator warning in case a converted tag has been forgotten (for the gpx: namespace in general).

What would you guys prefer?

[GerdP]

An option would be to create a way with untagged nodes, pass it to simplifyWayAction and add the ele tags to the remaining nodes.

[Bjoeni]

That could technically work, yeah. But the tags would have to be cached while converting and be applied later, which could have a huge impact on the performance (especially when converting files > 100MB). I guess it would be easier and more performant to rewrite the simplifyWayAction.

But the problem about that is that it would behave differently if you simplify the way during conversion or manually later on. That's why I suggested making it uninteresting, the behavior would be identical both times (and being able to validate it is also an advantage I think).

[Bjoeni]

Just sanitized it, here are the two patches:

• one with the dialog for the SimplifyWayAction and the conversion GPX->OSM [2760-simplify.diff] and
• the other one additionally with the gpx tags prefixed and deemed "uninteresting" and an according validator warning [2760-simplify-uninteresting.diff]

The first one just doesn't do anything when simplifying a layer with nodes, the second one is able to do this.

n.b. the second patch partially conflicts with the patch on #16796, but that shouldn't be a big issue.

[GerdP]

I see some with these patches:
1) simple: public method SimplifyWayAction.simplifyWay() is required by other plugins, you renamed it to createSimplifyCommand()
I agree that the new name is clearer but we have to keep the old name.
2) When I press Shift+Y to simplify a way I see the new popup with the value that I used before to simplify GPX ways. Was that intended?
I think it would be better to store two different values. I prefer to have a small value like 0.5 m for Shift+Y while I would use maybe 2.5 m for GPX data.

Besides that I wonder if we should split ways to match the 2000 nodes limit when converting to data layer. This also happens when you import eg. kml files, see class DataSetUpdater in opendata plugin.

[Bjoeni]

1) simple: public method SimplifyWayAction.simplifyWay() is required by other plugins, you renamed it to createSimplifyCommand()
I agree that the new name is clearer but we have to keep the old name.

Didn't think of that, I can change it. Btw. is there an easy way to figure out if any of the "official" plugins use a function? Some are on github, some on this server, so I guess not really?

2) When I press Shift+Y to simplify a way I see the new popup with the value that I used before to simplify GPX ways. Was that intended?
I think it would be better to store two different values. I prefer to have a small value like 0.5 m for Shift+Y while I would use maybe 2.5 m for GPX data.

Yes intended, but I agree different values make more sense. The remember checkbox already uses different values.

Besides that I wonder if we should split ways to match the 2000 nodes limit when converting to data layer. This also happens when you import eg. kml files, see class DataSetUpdater in opendata plugin.

I don't think that's a good idea, at least not automatically.
1.) I quite often use JOSM to edit GPX tracks by converting them back and forth. Not its main purpose, but at least the user must be able to opt out
2.) I don't really see the situation where this does something good. If someone tries to upload an unedited GPX track with more than 2000 nodes, he probably doesn't know what he's doing. And anyways, in a situation where someone wants to upload a track this long for good, it's better to split ways manually where it makes sense.
3.) If you have a track with 3000 nodes and extract 500 to upload - it would probably always split the part you wanted to upload ;)

[GerdP]

Replying to Bjoeni:

1) simple: public method SimplifyWayAction.simplifyWay() is required by other plugins, you renamed it to createSimplifyCommand()
I agree that the new name is clearer but we have to keep the old name.

Didn't think of that, I can change it. Btw. is there an easy way to figure out if any of the "official" plugins use a function? Some are on github, some on this server, so I guess not really?

I check the sources from ​https://svn.openstreetmap.org/applications/editors/josm
Not sure which plugins don't appear there.

Besides that I wonder if we should split ways to match the 2000 nodes limit when converting to data layer. This also happens when you import eg. kml files, see class DataSetUpdater in opendata plugin.

I don't think that's a good idea, at least not automatically.
1.) I quite often use JOSM to edit GPX tracks by converting them back and forth. Not its main purpose, but at least the user must be able to opt out

OK, I did not think about this use case.

2.) I don't really see the situation where this does something good. If someone tries to upload an unedited GPX track with more than 2000 nodes, he probably doesn't know what he's doing. And anyways, in a situation where someone wants to upload a track this long for good, it's better to split ways manually where it makes sense.
3.) If you have a track with 3000 nodes and extract 500 to upload - it would probably always split the part you wanted to upload ;)

I agree in both points. I think I just wanted to point out that we handle different import formats in different ways.

[Bjoeni]

Updated version:

• changed name of createSimplifyCommand() back to simplifyWay(), and instead renamed newly created simplifyWays() to doSimplifyWays() to avoid confusion
• save different max-error values for manually and automatically called simplifyWay. Settings are nowsimplify-way.max-error, simplify-way.remember, simplify-way.auto.max-error, simplify-way.auto.remember
• convert both prefixed and non-prefixed tags back to GPX for backward compatibility
• extracted "gpx:" prefix to a constant

[GerdP]

Thanks!

• The patch also fixes an unrelated typo in unnecessary.mapcss introduced with r11146. Is that intended? I'd prefer to have a distinct ticket for that.
• I don't understand the change in unit test OsmDataLayerTest. I think you should add another test to check your changes unless you think that the old behaviour was wrong.

• Not related to your patch: It seems "Convert to GPX layer" doesn't fully work. When I convert an untagged way only the GPX points are displayed This changes when I add a name tag to the way before converting. Seems to be a problem in the renderer as the TrkSeg information is not lost when I safe the converted layer to file.

[Bjoeni]

Replying to GerdP:

Thanks!

• The patch also fixes an unrelated typo in unnecessary.mapcss introduced with r11146. Is that intended? I'd prefer to have a distinct ticket for that.

Yes. I just didn't bother creating a new ticket for one }...

• I don't understand the change in unit test OsmDataLayerTest. I think you should add another test to check your changes unless you think that the old behaviour was wrong.

It checks the conversion and now checks both prefixed and non-prefixed GPX-tags. So before it checked three times if it could convert "ele", now it checks once "gpx:ele" and twice "ele". Same for "time"/"gpx:time". I can create a second one, but honestly I don't really see the point of doing that.

• Not related to your patch: It seems "Convert to GPX layer" doesn't fully work. When I convert an untagged way only the GPX points are displayed This changes when I add a name tag to the way before converting. Seems to be a problem in the renderer as the TrkSeg information is not lost when I safe the converted layer to file.

That's a regression of #16963, see comment:23. That happens if a track doesn't have timestamps. I'll open a new ticket for that.

[Bjoeni]

see #17596

[Bjoeni]

Replying to GerdP:

Replying to Bjoeni:

1) simple: public method SimplifyWayAction.simplifyWay() is required by other plugins, you renamed it to createSimplifyCommand()
I agree that the new name is clearer but we have to keep the old name.

Didn't think of that, I can change it. Btw. is there an easy way to figure out if any of the "official" plugins use a function? Some are on github, some on this server, so I guess not really?

I check the sources from ​https://svn.openstreetmap.org/applications/editors/josm
Not sure which plugins don't appear there.

I checked that, it's used by OpenData and cadastre-fr and both essentially iterate through a collection of ways, and execute the returned commands straight away, so they could also use the new method that's now called doSimplifyWays. Would it be an idea to modify the plugins and rename the method or is that not an option for backward compatibility reasons?
I just don't really like the idea of having a method called simplifyWay() that doesn't actually do anything.

[GerdP]

I just don't really like the idea of having a method called simplifyWay() that doesn't actually do anything.

I am also not sure how to handle these cases. Normally you would first deprecate the method. See also ticket:13538#comment:5 where I ask a related question.

[Klumbumbus]

In 14987/josm:

• see #2760 see #13804 - fix validator rule syntax
• see #17592 - fix validator rule syntax
• fix #17594 - warn about tracktype=grade2 with surface=sand|mud

[Klumbumbus]

I fixed the typo as this ticket will maybe not be fixed in 19.04.

[Bjoeni]

@Vincent, any plans regarding this patch?
It's not so much about the dialog but about prefixing the converted tags and therefore having a validator warning for this, because I lately saw a lot of blindly uploaded (likely automatically converted) tags.

A small sample from my region: only nodes of a highway that either have a time or an ele tag with at least 3 decimals: ​https://overpass-turbo.eu/s/MSq
Most or all of them appear to be from some 10-15 y/o version of Potlach, but I'm afraid that we're basically causing the same behavior since the tags are converted in JOSM as well.

[Don-vip]

Sorry, I forgot about it.
I'm ok with it. Concerning the simplifyWay method, please deprecate the existing one and add a new method with a better name. This way it gives time to plugin developers (including me) to update.

[Bjoeni]

Done.

In case of multiple gpx:* tags, the validator only complains about one (because it stops looking for further matches once it found one, e.g. it doesn't look for gpx:time once gpx:ele has been found on a node). This means that when clicking "fix problems", it removes all gpx:ele and complains again the next time you try to upload. Probably not worth changing the validator because of this small issue, just wanted to note it.

[Don-vip]

In 15419/josm:

fix #2760 - simplify track during GPX layer -> data layer conversion (patch by Bjoeni)

[Don-vip]

In 15422/josm:

see #2760 - javadoc

[Don-vip]

Two tests are failing in headless mode:
https://josm.openstreetmap.de/jenkins/job/JOSM/lastCompletedBuild/testReport/

org.openstreetmap.josm.actions.SimplifyWayActionTest.testSingleWay
org.openstreetmap.josm.actions.SimplifyWayActionTest.testMoreThanTenWays

java.awt.HeadlessException
	at java.awt.GraphicsEnvironment.checkHeadless(GraphicsEnvironment.java:204)
	at java.awt.Window.init(Window.java:484)
	at java.awt.Window.<init>(Window.java:436)
	at java.awt.Window.<init>(Window.java:591)
	at java.awt.Dialog.<init>(Dialog.java:665)
	at javax.swing.JDialog.<init>(JDialog.java:592)
	at org.openstreetmap.josm.gui.ExtendedDialog.<init>(ExtendedDialog.java:159)
	at org.openstreetmap.josm.gui.ExtendedDialog.<init>(ExtendedDialog.java:136)
	at org.openstreetmap.josm.actions.SimplifyWayAction.askSimplifyWays(SimplifyWayAction.java:145)
	at org.openstreetmap.josm.actions.SimplifyWayAction.actionPerformed(SimplifyWayAction.java:191)

It probably requires some mocking to fix.

[Bjoeni]

I could rewrite the tests so that only the simplification without dialogs but not the actual action is checked, but I never worked with JMockit and don't really have the time to learn that atm. What do you want me to do?

Also, why does the other dialog (warning about more than 10 ways) work? Didn't see any mocking there, or is that because it's an OptionPane?

[ris]

You shouldn't need to know anything about JMockit - I've designed classes for mocking responses to dialogs - in this case ExtendedDialogMocker. The class has documentation for simple cases, for more complex cases grep the tests for various uses of it.

[Bjoeni]

I'll have a look, thanks.

[Bjoeni]

Just had a quick look... I'm not sure if I really get it (as mentioned above, I'm new to mocking and relatively new to unit testing).

The failing tests don't seem to test anything anyways (or at least not assert anything, they just make sure that the action can run without runtime errors), and since the whole point of mocking should be to get rid of the isHeadless()-calls (and definitely not create new ones), it doesn't make sense to test the action itself anymore in my understanding.
So what I would do is only check the simplifyWay-method (and the result, so the actual simplifying, imo more important than what the tests are doing right now), but not the dialog, because that seems quite pointless in this case.

Am I missing something here?

[Don-vip]

Replying to Bjoeni:

The failing tests don't seem to test anything anyways (or at least not assert anything, they just make sure that the action can run without runtime errors), and since the whole point of mocking should be to get rid of the isHeadless()-calls (and definitely not create new ones), it doesn't make sense to test the action itself anymore in my understanding.
So what I would do is only check the simplifyWay-method (and the result, so the actual simplifying, imo more important than what the tests are doing right now), but not the dialog, because that seems quite pointless in this case.

You're right, you can drop these useless tests and add a more meaningful one that tests the simplification logic.

[Bjoeni]

Done.
I also added some tests for the conversion GPX->OSM, because I anyways added the test files and modified the conversion with this patch as well (the gpx: prefix).

[Don-vip]

In 15431/josm:

fix #2760 - update unit tests (patch by Bjoeni)

[Bjoeni]

The tracks.gpx went missing for some reason (svn says it's under version control but didn't include it in the diff for some reason). Can you put it in the test/data/tracks folder please?

[Don-vip]

In 15432/josm:

see #2760 - fix unit tests

[Bjoeni]

Thanks!

[Don-vip]

In 18075/josm:

see #2760 - only consider GPX-prefixed tags uninteresting during the "convert to data" simplification process

[Bjoeni]

(I'm on mobile so didn't test anything)

What was the reason for that change? If I remember correctly this causes ways to not be deleted properly either (e.g. if you try to delete a way with gpx:time tags on each node, the nodes will remain after you deleted the way).

[Don-vip]

I work on a new feature to edit GPX data using OSM tools and I need to differentiate nodes tagged with gpx: from those who won't, but you're right, I didn't think of the deletion use case. I'll find another way.

[Don-vip]

In 18076/josm:

see #2760 - revert r18075 - causes nodes to remain after deletion of converted ways

[taylor.smock]

Ticket #14885 has been marked as a duplicate of this ticket.