Modify

Opened 10 years ago

Closed 2 months ago

Last modified 2 months ago

#2760 closed enhancement (fixed)

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

Reported by: anonymous Owned by: team
Priority: normal Milestone: 19.10
Component: Core Version:
Keywords: gpx Cc: chris.bainbridge@…, Bjoeni, ris

Description (last modified by Don-vip)

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.

Attachments (9)

2760-simplify-auto.png (21.1 KB) - added by Bjoeni 8 months ago.
2760-simplify-manual.png (20.7 KB) - added by Bjoeni 8 months ago.
2760-validator.png (27.1 KB) - added by Bjoeni 8 months ago.
2760-simplify.diff (10.4 KB) - added by Bjoeni 8 months ago.
2760-simplify-uninteresting.diff (17.9 KB) - added by Bjoeni 8 months ago.
2760-simplify-uninteresting-V2.diff (19.7 KB) - added by Bjoeni 8 months ago.
2760-simplify-V3.diff (21.8 KB) - added by Bjoeni 2 months ago.
2760-tests-V1.diff (283.1 KB) - added by Bjoeni 2 months ago.
tracks.gpx (49.0 KB) - added by Bjoeni 2 months ago.

Download all attachments as: .zip

Change History (46)

comment:1 Changed 10 years ago by anonymous

Cc: chris.bainbridge@… added

comment:2 Changed 10 years ago by stoecker

Type: defectenhancement

comment:3 Changed 14 months ago by Don-vip

Cc: Bjoeni added
Description: modified (diff)

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

comment:4 Changed 14 months ago by 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.

comment:5 in reply to:  4 Changed 14 months ago by 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 :)

comment:6 Changed 13 months ago by 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 ;)

comment:7 Changed 8 months ago by 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?

comment:8 Changed 8 months ago by 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.

comment:9 Changed 8 months ago by 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).

Last edited 8 months ago by Bjoeni (previous) (diff)

comment:10 Changed 8 months ago by Bjoeni

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

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.




Changed 8 months ago by Bjoeni

Attachment: 2760-simplify-auto.png added

Changed 8 months ago by Bjoeni

Attachment: 2760-simplify-manual.png added

Changed 8 months ago by Bjoeni

Attachment: 2760-validator.png added

Changed 8 months ago by Bjoeni

Attachment: 2760-simplify.diff added

Changed 8 months ago by Bjoeni

comment:11 Changed 8 months ago by 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.

Last edited 8 months ago by GerdP (previous) (diff)

comment:12 Changed 8 months ago by 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 ;)

comment:13 in reply to:  12 ; Changed 8 months ago by 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.

comment:14 Changed 8 months ago by 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

Changed 8 months ago by Bjoeni

comment:15 Changed 8 months ago by 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.

comment:16 in reply to:  15 Changed 8 months ago by 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.

comment:17 Changed 8 months ago by Bjoeni

see #17596

comment:18 in reply to:  13 Changed 8 months ago by 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.

comment:19 Changed 8 months ago by 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.

comment:20 Changed 8 months ago by 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

comment:21 Changed 8 months ago by Klumbumbus

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

comment:22 Changed 2 months ago by 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.

comment:23 Changed 2 months ago by Don-vip

Keywords: gpx added
Milestone: 19.10
Priority: minornormal

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.

comment:24 Changed 2 months ago by 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.

Changed 2 months ago by Bjoeni

Attachment: 2760-simplify-V3.diff added

comment:25 Changed 2 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 15419/josm:

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

comment:26 Changed 2 months ago by Don-vip

In 15422/josm:

see #2760 - javadoc

comment:27 Changed 2 months ago by Don-vip

Cc: ris added
Resolution: fixed
Status: closedreopened

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.

Last edited 2 months ago by Don-vip (previous) (diff)

comment:28 Changed 2 months ago by 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?

comment:29 Changed 2 months ago by 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.

comment:30 Changed 2 months ago by Bjoeni

I'll have a look, thanks.

comment:31 Changed 2 months ago by 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?

comment:32 in reply to:  31 Changed 2 months ago by 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.

comment:33 Changed 2 months ago by 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).

Changed 2 months ago by Bjoeni

Attachment: 2760-tests-V1.diff added

comment:34 Changed 2 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 15431/josm:

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

comment:35 Changed 2 months ago by 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?

Last edited 2 months ago by Bjoeni (previous) (diff)

Changed 2 months ago by Bjoeni

Attachment: tracks.gpx added

comment:36 Changed 2 months ago by Don-vip

In 15432/josm:

see #2760 - fix unit tests

comment:37 Changed 2 months ago by Bjoeni

Thanks!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.