Modify

Opened 10 months ago

Last modified 4 days ago

#19199 new enhancement

[PATCH] There should be some way to determine how many nodes are superflous in a way

Reported by: taylor.smock Owned by: GerdP
Priority: normal Milestone: 21.02
Component: Core Version:
Keywords: template_report Cc:

Description

This can be used to tell the user roughly how many nodes will be removed (although, since it uses the same algorithm as the actual node removal, it should be the same).

The primary reason I want it is so that I can easily tell how many nodes will be removed, if the user simplifies the way, although this patch isn't necessary for my needs (I can just directly call the commands with specified parameters).

I do think it will be a useful UI component, regardless (as seen in the patch).

Attachments (8)

19199.patch (5.5 KB) - added by taylor.smock 10 months ago.
19199.1.patch (7.1 KB) - added by taylor.smock 10 months ago.
This was an attempt to allow the mapview display how the maximum error (threshold) actually affected the way. This does not work, since the EDT is blocked.
19199.gpx_to_osm.patch (1.6 KB) - added by taylor.smock 9 months ago.
Send ways that will be simplified due to converting from GPX to OSM data
19199.threading.patch (1.8 KB) - added by taylor.smock 6 months ago.
Add threading
19199.threading.1.patch (1.9 KB) - added by taylor.smock 6 days ago.
Update patch to current source (line number change only)
19199.threading.2.patch (20.5 KB) - added by taylor.smock 4 days ago.
Add methods to allow for async ExtendedDialogs (so we can update the map), don't update mapview when there are more than 10 ways selected
19199.threading.3.patch (20.8 KB) - added by taylor.smock 4 days ago.
Remove .project file
19199.threading.5.patch (17.7 KB) - added by taylor.smock 4 days ago.
Drop listener code in ExtendedDialog

Download all attachments as: .zip

Change History (33)

Changed 10 months ago by taylor.smock

Attachment: 19199.patch added

Changed 10 months ago by taylor.smock

Attachment: 19199.1.patch added

This was an attempt to allow the mapview display how the maximum error (threshold) actually affected the way. This does not work, since the EDT is blocked.

comment:1 Changed 9 months ago by taylor.smock

Is there any interest for this patch?

comment:2 Changed 9 months ago by simon04

Resolution: fixed
Status: newclosed

In 16566/josm:

fix #19199 - SimplifyWayAction: display how many nodes are going to be removed (patch by taylor.smock, modified)

comment:3 Changed 9 months ago by simon04

Milestone: 20.06

comment:4 Changed 9 months ago by GerdP

Nice! I miss two points:
1) some kind of percentage would be nice, e.g. "about 1,232 nodes (30%) to remove"
2) Shouldn't convert gpx to data use the same code and thus show the same dialog?

comment:5 in reply to:  4 Changed 9 months ago by taylor.smock

Replying to GerdP:

Nice! I miss two points:
1) some kind of percentage would be nice, e.g. "about 1,232 nodes (30%) to remove"

I think I did look at adding a percentage. I don't remember why I didn't (possibly due to line length -- I probably didn't want to try to do a line-wrap).

2) Shouldn't convert gpx to data use the same code and thus show the same dialog?

I've never used that feature -- my local area has recent imagery (~1 year old at this point). I would expect them to use the same dialog.

That being said, the nodes to remove only appear if the calling function sends the ways that are being modified to the dialog. Otherwise, it cannot determine the number of nodes that will be removed.

I'll be uploading a patch for that here in a minute.

Changed 9 months ago by taylor.smock

Attachment: 19199.gpx_to_osm.patch added

Send ways that will be simplified due to converting from GPX to OSM data

comment:6 Changed 9 months ago by GerdP

In 16570/josm:

see #19199: There should be some way to determine how many nodes are superflous in a way

  • show number of nodes also when converting gpx to data layer

19199.gpx_to_osm.patch by taylor.smock

comment:7 Changed 9 months ago by taylor.smock

It looks like the 19199.1.patch was the patch that was merged. If I (or someone else) ever gets around to allowing the map screen to repaint while a dialog window is up, some of the code used will no longer be dead (it is effectively dead -- it is still called, but doesn't do what it is supposed to do). It was supposed to show how the way changed as the threshold was modified.

See https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/SimplifyWayAction.java?rev=16566#L499 for the code in question.

Calling code could probably be modified to do something like this:

MainApplication.worker.submit(() -> {
    int ret = SimplifyWayAction.askSimplifyWays(ways, text, auto);
    realizeSimplifyReturn(ret);
});

There are probably some EDT violations in there somewhere, and there might be some other locations where the EDT is blocked by the simplify way dialog.

comment:8 Changed 7 months ago by Bjoeni

This needs to be moved to another thread. When converting big GPX files to datasets JOSM becomes unresponsive for a long time (more than 20 seconds) just figuring out how many nodes are to be removed (same when changing the max-error).

Changed 6 months ago by taylor.smock

Attachment: 19199.threading.patch added

Add threading

comment:9 Changed 6 months ago by taylor.smock

@Bjoeni: Can you apply that patch and see if it fixes your problem? I don't have a large enough GPX file for it to show any real difference on my machine.

comment:10 Changed 6 months ago by Bjoeni

Yep, that works perfectly fine. Thank you!

comment:11 Changed 6 months ago by stoecker

Resolution: fixed
Status: closedreopened

comment:12 Changed 6 months ago by taylor.smock

(Don't apply that patch to JOSM core yet -- it may have undesirable side effects, for example, what happens if it is still calculating when the dialog is closed? I really need to find something that takes a good amount of time for testing purposes, but worst case scenario, it will run the command and place it in the undo/redo buffer.)

comment:13 Changed 6 months ago by Bjoeni

what happens if it is still calculating when the dialog is closed?

For me that didn't cause any issues. nodesToRemove.setText() goes through without any issues even if the dialog is closed already. Also doesn't cause any issues even if the same dialog is opened again while the closed one is still calculating.

Unfortunately I can't provide you with the original GPX file I'm currently using, but you could just put a Thread.sleep() call in updateNodesToRemove. Or you download a lot of raw GPS data from the server and convert that (doesn't take that long either but long enough to test).

comment:14 Changed 11 days ago by Bjoeni

@taylor.smock
What's the state of this patch? It works for me, and makes things much faster (just tested it without the patch on a 250MB file, JOSM froze for 55 secs before showing the dialog and also each time the value was changed).

comment:15 in reply to:  14 Changed 6 days ago by taylor.smock

Replying to Bjoeni:

@taylor.smock
What's the state of this patch? It works for me, and makes things much faster (just tested it without the patch on a 250MB file, JOSM froze for 55 secs before showing the dialog and also each time the value was changed).

That is a good question. I should have remembered to ping the ticket every two weeks or so (it probably just fell out of the active ticket range).

Anyway, I'll upload a patch that applies cleanly (there was a slight line-number change).

Changed 6 days ago by taylor.smock

Attachment: 19199.threading.1.patch added

Update patch to current source (line number change only)

comment:16 Changed 5 days ago by stoecker

Milestone: 20.0621.02

comment:17 Changed 5 days ago by GerdP

Owner: changed from team to GerdP
Status: reopenednew

comment:18 Changed 5 days ago by GerdP

I have a file containing my collected recent GPX tracks. I created it by merging ~340 tracks, it's about ~23.5 MB. I load that file and choose "Convert to data layer". When I click on convert the unpatched version shows a small delay and than the "Simplify way" dialog while at the same time a new command command is added to the "Command stack window".
With the patch it is even more confusing because the command stack is updated while I'm looking at the popup.
Why is the command added to the UndoRedoHandler before I've even decided what to do?

comment:19 Changed 5 days ago by taylor.smock

To answer your question, I wanted to set it up such that the user could see how the way was changing as they modified the number. (See comment:7). That didn't end up working since the dialog window currently blocks the EDT. However, if we unblock the EDT, the user will be able to see how the way changes as they increase/decrease the number (and it finishes calculating).

Since I didn't want to accidentally leave the way in an inconsistent state if something happened, I added the change command to the undo/redo stack. This meant that if the window crashed, the change could still be undone.

Reading back (comment:12),

(Don't apply that patch to JOSM core yet -- it may have undesirable side effects, for example, what happens if it is still calculating when the dialog is closed? I really need to find something that takes a good amount of time for testing purposes, but worst case scenario, it will run the command and place it in the undo/redo buffer.)

I'll see if I can create a large simplify action to test the corner cases.

comment:20 Changed 5 days ago by GerdP

OK, so two different, probably conflicting goals:
1) Visualize the changes
2) Compute the number (I'd prefer percentage) of removed nodes without changing anything, preferably as fast as possible (multi-threaded?)

Maybe the overall number of way nodes can be used to decide what is more important?

comment:21 Changed 5 days ago by taylor.smock

In order to do either, I first need to calculate the nodes that will be removed. Either one should probably be async, regardless.

For (1), I need to do something so that the EDT isn't blocked by the dialog, so I would need to push the calculation to another thread anyway. For (2), I still have to move it to another thread (multiple), and return the number of nodes that will be removed.

So, I don't think that they are necessarily conflicting goals. But I could be wrong.

Changed 4 days ago by taylor.smock

Attachment: 19199.threading.2.patch added

Add methods to allow for async ExtendedDialogs (so we can update the map), don't update mapview when there are more than 10 ways selected

comment:22 Changed 4 days ago by GerdP

I assume this is WIP? Patch contains mods in .project.

comment:23 Changed 4 days ago by taylor.smock

To answer your question, I *think* it's done. It just needs a little bit of testing.

For example, I *think* I fixed a bug where, when changing the max error quickly, would cause a command to not be undone. I wasn't able to repro again after adding syncs, but I wanted to double check when my computer wasn't under intense memory pressure.

The .project mod was an accidental add to the patch.

As soon as I'm on a real computer, and not my phone, I'll edit the patch.

comment:24 Changed 4 days ago by GerdP

With the patch the command is still added to the command stack before I've decided what to do when using the normal mode (Shift+Y)
Do we really need all the new code? ExtendedDialog has a parameter reg. modal/non-modal. Isn't that enough?

Changed 4 days ago by taylor.smock

Attachment: 19199.threading.3.patch added

Remove .project file

comment:25 in reply to:  24 Changed 4 days ago by taylor.smock

Replying to GerdP:

With the patch the command is still added to the command stack before I've decided what to do when using the normal mode (Shift+Y)
Do we really need all the new code? ExtendedDialog has a parameter reg. modal/non-modal. Isn't that enough?

I added the listener code since I didn't see a method to listen for the close of the modal. I'll double check that.

EDIT: As far as adding the command to the command stack goes, that is largely a fail-safe. Once you close the window, it will undo the command. Unless you have made some other modifications in the mapview prior to closing the window.

Last edited 4 days ago by taylor.smock (previous) (diff)

Changed 4 days ago by taylor.smock

Attachment: 19199.threading.5.patch added

Drop listener code in ExtendedDialog

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain GerdP.
as The resolution will be set.
to The owner will be changed from GerdP to the specified user.
The owner will change to taylor.smock
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from GerdP to anonymous.

Add Comment


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

 
Note: See TracTickets for help on using tickets.