Modify

Opened 9 years ago

Closed 7 years ago

#7295 closed defect (fixed)

Possible information loss and NPE with Replace Geometry command

Reported by: joshdoe Owned by: team
Priority: major Milestone:
Component: Plugin utilsplugin2 Version:
Keywords: replace geometry Cc: joshdoe, rickmastfan67

Description (last modified by joshdoe)

I've noticed a few potential issues with the Replace Geometry command, where information can possibly be lost and/or NPEs can be triggered.

  1. Currently if you use the Replace Geometry command and one or more of the source or target way's nodes are tagged, then it's likely that the tags will either be lost (as the node is replaced by another), or the tagged node will be moved to a location where the tags may no longer be valid (barrier=gate, entrance=building, etc.). I think this command should only be allowed to operate on non-new (uploaded) ways which don't have any tagged nodes, or at least don't have nodes with interesting tags (non source,fixme, etc.). The replacement way could have tagged nodes, but if the nodes are replaced with id>0 nodes, the tags should be transferred.
  1. I've also noticed that relations can become corrupted when using this command. First we need to decide if relation memberships should transfer; I think it makes sense to do this at least for outer/inner, maybe ask every time? If not, then we should not allow touching objects that are members of relations. I've gotten NPE's when replacing the geometry where the new way is part of a multipolygon.
  1. If replacement nodes are greater than 3e-4 degrees (hardcoded) from the other nodes, the nodes aren't moved & transferred. I think by default there should be no limit to the distance, but have it settable in the preferences (using meters instead of degrees.

Attachments (6)

ticket7295.osm (334.1 KB) - added by rickmastfan67 9 years ago.
assign_nodes_hungarian.patch (22.1 KB) - added by joshdoe 9 years ago.
assign nodes using Hungarian algorithm
ticket7295_x2.7z (133.8 KB) - added by rickmastfan67 9 years ago.
ticket7295_x4.7z (194.7 KB) - added by rickmastfan67 9 years ago.
use_djikstra_assignment.patch (38.1 KB) - added by joshdoe 9 years ago.
utils2plugin using Hungarian algorithm for assigning nodes
ticket7295_x5.7z (328.1 KB) - added by rickmastfan67 9 years ago.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 9 years ago by joshdoe

Description: modified (diff)
Summary: Replace geometry command shouldn't touch tagged nodesPossible information loss and NPE with Replace Geometry command

comment:2 Changed 9 years ago by joshdoe

Resolution: fixed
Status: newclosed

Fix 1 and 3 (distance configurable with utilsplugin2.replace-geometry.max-distance) and a few other potential bugs in [o27623]. 2 should be handled by #7278. I'm being a bit aggressive with disallowing the action under certain conditions, but I'd rather have complaints about it not letting them do something rather than producing bad data; we can more easily rectify the former.

JAR updated in [o27625].

Last edited 9 years ago by joshdoe (previous) (diff)

comment:3 Changed 9 years ago by joshdoe

Resolution: fixed
Status: closedreopened

(Moved from #7278)

Replying to rickmastfan67:

Ok, this new way is driving me NUTS. Especially if the new way and the way you're replacing have both the same starting/ending nodes and those nodes have tags on them. IMO, it's all but nerfted the usefulness of the "Replace Geometry" command.

Example:
A way between two nodes that are traffic lights (as an example) is really bad and doesn't look great, yet the two traffic light nodes are in the correct places. So, I trace a new way that starts and ends at those two traffic light nodes. And then I go to the replace geometry command to replace the old way with the new one, and I'm prevented from doing so because of those traffic light nodes. I then have to remove the lights (temporarily) from the nodes, then replace geometry and then put the lights back on the nodes.

I consider that a waste of time. I mean, at least if the first and last nodes of both the original and the way to replace it are the same, it shouldn't matter if those two nodes at have tags on them or not. They aren't being replaced or even moved.

Thanks for reporting this. As I mentioned above I expected there would be some issues, but I had the goal of preventing accidental loss of information.

I think what would solve this and similar problems is to not consider any tagged nodes that are shared between the old and new ways as "important" when determining whether to allow the replacement or not. I'll look at this soon, probably tonight or tomorrow. In the meantime you can use an older version like this one.

comment:4 Changed 9 years ago by joshdoe

Cc: rickmastfan67 added

It was easier than I thought, fixed in [o27698] (JAR published in [o27699]). Let me know if this works. I'll leave this open until confirmed.

comment:5 in reply to:  4 Changed 9 years ago by rickmastfan67

Resolution: fixed
Status: reopenedclosed

Replying to joshdoe:

It was easier than I thought, fixed in [o27698] (JAR published in [o27699]). Let me know if this works. I'll leave this open until confirmed.

Thanks for fixing that. I just tried it with a section between two traffic lights and one section with 3 lights (one in the middle), and both times the "Replace Geometry" command worked just as expected without me having to temporarily delete the traffic light (or anything else that was tagged on the node). :) So, I'll mark this one as fixed. ;)

comment:6 Changed 9 years ago by rickmastfan67

Hey joshdoe, I'm curious about the utilsplugin2.replace-geometry.max-distance setting. I noticed mine is set at "1". Is that "1" in km? If so, to set the max distance at 500m, I would put in that line "0.5", correct?
(BTW, take a quick look @ #7405).

comment:7 in reply to:  6 ; Changed 9 years ago by joshdoe

Replying to rickmastfan67:

Hey joshdoe, I'm curious about the utilsplugin2.replace-geometry.max-distance setting. I noticed mine is set at "1". Is that "1" in km? If so, to set the max distance at 500m, I would put in that line "0.5", correct?
(BTW, take a quick look @ #7405).

Actually that's currently in degrees, so more like 140km (for my location, it will vary quite a bit). The default is effectively infinity for most editing. I didn't see any value in having a max distance, but the original author seems to use it so I made it an option. I'd be curious to know why you'd want to have a limit.

As a side note the property is in degrees because I was lazy (i.e. it was the easiest, fastest way). This should likely be changed to use meters.

comment:8 in reply to:  7 ; Changed 9 years ago by rickmastfan67

Replying to joshdoe:

Replying to rickmastfan67:

Hey joshdoe, I'm curious about the utilsplugin2.replace-geometry.max-distance setting. I noticed mine is set at "1". Is that "1" in km? If so, to set the max distance at 500m, I would put in that line "0.5", correct?
(BTW, take a quick look @ #7405).

Actually that's currently in degrees, so more like 140km (for my location, it will vary quite a bit). The default is effectively infinity for most editing. I didn't see any value in having a max distance, but the original author seems to use it so I made it an option. I'd be curious to know why you'd want to have a limit.

As a side note the property is in degrees because I was lazy (i.e. it was the easiest, fastest way). This should likely be changed to use meters.

Well, since the changes you made recently, I've been noticing nodes sometimes move over 2km from their original location when using the "Replace Geometry" feature. I thought that the nodes would move to the closest new node to them originally instead of most of the time recently, the beginning of the way. I've been noticing this happening a lot lately, especially when I'm cleaning up a longer way (3km+). I mean, if there is a new node in the new way that's less than 25m from the original node, shouldn't the ID be moved to that node instead of the beginning of the way?

comment:9 in reply to:  8 Changed 9 years ago by joshdoe

Resolution: fixed
Status: closedreopened

Replying to rickmastfan67:

Well, since the changes you made recently, I've been noticing nodes sometimes move over 2km from their original location when using the "Replace Geometry" feature. I thought that the nodes would move to the closest new node to them originally instead of most of the time recently, the beginning of the way. I've been noticing this happening a lot lately, especially when I'm cleaning up a longer way (3km+). I mean, if there is a new node in the new way that's less than 25m from the original node, shouldn't the ID be moved to that node instead of the beginning of the way?

I haven't been able to reproduce this problem. Can you provide a sample file and instructions on how to reproduce this? Code is in there to replace available nodes in one way with available nodes in the other. Available here means that it is not a member of a relation, is tagged, etc. I think there are some edge cases we don't handle, but I wanted to err on the side of caution, preferring to discard a historical connection over risking the deletion of information. I'm happy with it, but if you or others can provide specific ways to improve this and handle some of the edge cases, I'll try and fix it (should probably be a separate ticket however, closing this one).

comment:10 Changed 9 years ago by rickmastfan67

Josh, next time I do a major re-trace of a long way and I have this happen, I'll provide a file here so you can easily duplicate this yourself. Just give me a few days to find a way I want to overhaul. lol. ;)

comment:11 Changed 9 years ago by rickmastfan67

Ok Josh, I got a way in a file (which will be attached after this post) that will allow you to duplicate this and see the 1km+ movement of some of the nodes when there was a replacement node almost right beside it. This way was originally a 16 node way being turned into a 42 node way with the help of the "Replace Geometry" action.

Here are the steps to reproduce this:

  1. load the attached file into JOSM.
  2. select the two ways to do the "Replace Geometry" on (way 74283226 + the blank one that intertwines with it)
  3. Now do the "Ctrl+Shift+G" to "Replace Geometry" for the two ways. (Or do it from the "More Tools menu, doesn't matter which way it's done.)
  4. Now, "Undo" this (there is a reason for this, just trust me).
  5. Now, find the following node on the original way from before the replacement (Node 148607496) and select it.
  6. Zoom out some so that you can see the entire way.
  7. Now go up to your tool bar and hit the "redo" command.
  8. Watch Node 148607496 (which is now highlighted) move over 1.5km from it's original location.

The above directions will allow you to see the the node movement without having to fully rely on the History window's distance info for nodes since after I upload the file here, I'll be uploading the new data to the OSM database to correct the roads in that area.

Also, if you select the node in the general area that Node 148607496 was in, you'll also see that none of them got IDs, even though they were the ones that should have gotten the ID (and history) instead of a new node over 1.5km away.

One more note on this. Sometimes I've seen different results on the nodes in this way. Sometimes the nodes didn't move as far when doing the replacement as I'd hope to show you the problem. However, with this file that I'm attaching, I've been able to reproduce the same exact results 5 times in a row after closing and restarting JOSM each time, so, hopefully the results will be the same on your system so you can see what I'm talking about here.

Changed 9 years ago by rickmastfan67

Attachment: ticket7295.osm added

comment:12 Changed 9 years ago by rickmastfan67

Whoops, forgot the build info of JOSM that I'm using to duplicate this:

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2012-03-06 02:36:00
Last Changed Author: Don-vip
Revision: 5046
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2012-03-05 22:41:23 +0100 (Mon, 05 Mar 2012)
Last Changed Rev: 5046

Identification: JOSM/1.5 (5046 en)
Memory Usage: 123 MB / 2730 MB (78 MB allocated, but free)
Java version: 1.6.0_31, Sun Microsystems Inc., Java HotSpot(TM) 64-Bit Server VM
Operating system: Windows 7
Dataset consistency test: No problems found

Plugin: OpeningHoursEditor (27852)
Plugin: buildings_tools (27984)
Plugin: licensechange (27964)
Plugin: mapdust (27884)
Plugin: measurement (27957)
Plugin: openstreetbugs (27852)
Plugin: reverter (27865)
Plugin: turnrestrictions (27891)
Plugin: undelete (27852)
Plugin: utilsplugin2 (27937)

comment:13 in reply to:  11 ; Changed 9 years ago by joshdoe

Replying to rickmastfan67:

Ok Josh, I got a way in a file (which will be attached after this post) that will allow you to duplicate this and see the 1km+ movement of some of the nodes when there was a replacement node almost right beside it. This way was originally a 16 node way being turned into a 42 node way with the help of the "Replace Geometry" action.
[snip]

Thanks, I think I understand the problem now. The code stores candidate source and target nodes in a Set, which guarantees no duplicates, but doesn't guarantee an order. When findNearestNode() is called, I'm sure it does find the nearest available node at that moment, but it doesn't do any sort of overall "distance moved" minimization. This is called the assignment problem. In the conflation plugin I use the Hungarian algorithm to solve this.

It would help to keep the source and target nodes in an ordered set, so assuming the two ways start in the same general area and follow the same general direction, the assignment results wouldn't be so wonky. However if you have a replacement way with fewer nodes than the to-be-replaced way (reference way and subject way in conflation terms), covering the same general distance, you'll still have some nodes that are displaced a great deal. The best solution would then be using the Hungarian algorithm. You can see what the assignment would look like by installing the (highly experimental!) conflation plugin, then selecting all child nodes of one way as the Source and all child nodes of the other as Target. (I haven't updated the plugin to use the better terms of reference and subject yet).

Changed 9 years ago by joshdoe

assign nodes using Hungarian algorithm

comment:14 in reply to:  13 Changed 9 years ago by joshdoe

Replying to joshdoe:

Replying to rickmastfan67:

Ok Josh, I got a way in a file (which will be attached after this post) that will allow you to duplicate this and see the 1km+ movement of some of the nodes when there was a replacement node almost right beside it. This way was originally a 16 node way being turned into a 42 node way with the help of the "Replace Geometry" action.
[snip]

Thanks, I think I understand the problem now. The code stores candidate source and target nodes in a Set, which guarantees no duplicates, but doesn't guarantee an order. When findNearestNode() is called, I'm sure it does find the nearest available node at that moment, but it doesn't do any sort of overall "distance moved" minimization. This is called the assignment problem. In the conflation plugin I use the Hungarian algorithm to solve this.

Try out the attached plugin which uses the Hungarian algorithm for assigning nodes based on overall lowest distance moved, and see the corresponding patch if interested. I moved the algorithm from the conflation plugin for this (since conflation depends on utilsplugin2 anyways). Let me know @rickmastfan67.

comment:15 Changed 9 years ago by rickmastfan67

I just tried this new version you attached with the test file I provided above and it seems to do a MUCH better job than previously. The node I gave as the example above moved correctly to the new node right beside it instead of moving over 1.5km.

So, I'd say commit the tweaks to the official version and release it. ;)

comment:16 Changed 9 years ago by rickmastfan67

Hmm, on second thought, I've been testing this "modified" version some more, and some more fine tuning could be used. Look at the new attached file.

  1. once you have the file loaded, Ctrl+F and paste in the following word (without the quotes): "deerfield".
  2. Once it's found the one way with that name, zoom in on it from the Selection box.
  3. Do the replace geometry function on both Deerfield Drive and it's replacement way.
  4. Now, undo and then find the following node in the original way (109533698).
  5. Hit the redo button and watch node 109533698 go the beginning of the way (moving 148.8m) instead of staying right in the same area.

This is happening with the modified utilsplugin2.jar (205,071 bytes) you attached above. That node ID bypassed 8 brand new nodes before it "found a home". So, a little more tweaking might be needed.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2012-03-09 02:32:19
Last Changed Author: simon04
Revision: 5061
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2012-03-08 22:03:42 +0100 (Thu, 08 Mar 2012)
Last Changed Rev: 5061

Identification: JOSM/1.5 (5061 en)
Memory Usage: 145 MB / 2730 MB (96 MB allocated, but free)
Java version: 1.6.0_31, Sun Microsystems Inc., Java HotSpot(TM) 64-Bit Server VM
Operating system: Windows 7
Dataset consistency test: No problems found

Plugin: OpeningHoursEditor (27852)
Plugin: buildings_tools (27984)
Plugin: licensechange (27964)
Plugin: mapdust (27884)
Plugin: measurement (27957)
Plugin: openstreetbugs (27852)
Plugin: reverter (27865)
Plugin: turnrestrictions (27891)
Plugin: undelete (27852)
Plugin: utilsplugin2 (27998) - modified version attached in this thread (205,071 bytes)

Changed 9 years ago by rickmastfan67

Attachment: ticket7295_x2.7z added

comment:17 Changed 9 years ago by rickmastfan67

And now I just had the modified version of utilsplugin2 crash on me when using the "Replace Geometry". :(

I've saved the file right at the point where this crash is reproducible 100% of the time with the modified version. The current "official" version doesn't crash. If you want me to attach the file, let me know and I'll do it and I'll post the instructions to cause the crash.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2012-03-09 02:32:19
Last Changed Author: simon04
Revision: 5061
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2012-03-08 22:03:42 +0100 (Thu, 08 Mar 2012)
Last Changed Rev: 5061

Identification: JOSM/1.5 (5061 en)
Memory Usage: 193 MB / 2730 MB (83 MB allocated, but free)
Java version: 1.6.0_31, Sun Microsystems Inc., Java HotSpot(TM) 64-Bit Server VM
Operating system: Windows 7
Dataset consistency test: No problems found

Plugin: OpeningHoursEditor (27852)
Plugin: buildings_tools (27984)
Plugin: licensechange (27964)
Plugin: mapdust (27884)
Plugin: measurement (27957)
Plugin: openstreetbugs (27852)
Plugin: reverter (27865)
Plugin: turnrestrictions (27891)
Plugin: undelete (27852)
Plugin: utilsplugin2 (27998)

java.lang.ArrayIndexOutOfBoundsException: 0
	at utilsplugin2.dumbutils.HungarianAlgorithm.copyOf(HungarianAlgorithm.java:132)
	at utilsplugin2.dumbutils.HungarianAlgorithm.hgAlgorithm(HungarianAlgorithm.java:148)
	at utilsplugin2.dumbutils.ReplaceGeometryUtils.replaceWay(ReplaceGeometryUtils.java:299)
	at utilsplugin2.dumbutils.ReplaceGeometryUtils.replaceWayWithNew(ReplaceGeometryUtils.java:227)
	at utilsplugin2.dumbutils.ReplaceGeometryUtils.replaceWithNew(ReplaceGeometryUtils.java:38)
	at utilsplugin2.dumbutils.ReplaceGeometryAction.actionPerformed(ReplaceGeometryAction.java:48)
	at javax.swing.AbstractButton.fireActionPerformed(Unknown Source)
	at javax.swing.AbstractButton$Handler.actionPerformed(Unknown Source)
	at javax.swing.DefaultButtonModel.fireActionPerformed(Unknown Source)
	at javax.swing.DefaultButtonModel.setPressed(Unknown Source)
	at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(Unknown Source)
	at java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
	at java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
	at java.awt.Component.processMouseEvent(Unknown Source)
	at javax.swing.JComponent.processMouseEvent(Unknown Source)
	at java.awt.Component.processEvent(Unknown Source)
	at java.awt.Container.processEvent(Unknown Source)
	at java.awt.Component.dispatchEventImpl(Unknown Source)
	at java.awt.Container.dispatchEventImpl(Unknown Source)
	at java.awt.Component.dispatchEvent(Unknown Source)
	at java.awt.LightweightDispatcher.retargetMouseEvent(Unknown Source)
	at java.awt.LightweightDispatcher.processMouseEvent(Unknown Source)
	at java.awt.LightweightDispatcher.dispatchEvent(Unknown Source)
	at java.awt.Container.dispatchEventImpl(Unknown Source)
	at java.awt.Window.dispatchEventImpl(Unknown Source)
	at java.awt.Component.dispatchEvent(Unknown Source)
	at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
	at java.awt.EventQueue.access$000(Unknown Source)
	at java.awt.EventQueue$1.run(Unknown Source)
	at java.awt.EventQueue$1.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(Unknown Source)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(Unknown Source)
	at java.awt.EventQueue$2.run(Unknown Source)
	at java.awt.EventQueue$2.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(Unknown Source)
	at java.awt.EventQueue.dispatchEvent(Unknown Source)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.run(Unknown Source)

comment:18 in reply to:  16 Changed 9 years ago by joshdoe

Replying to rickmastfan67:

Hmm, on second thought, I've been testing this "modified" version some more, and some more fine tuning could be used. Look at the new attached file.
[snip]

Okay, I think I've fixed this in the attached updated plugin, or at least it works with this test case. I believe I may have fixed the other problem you reported; if this is not the case, please attach an OSM.XML file so I can reproduce and debug. Thanks for testing this for me, I'll commit if it works for you as well.

comment:19 Changed 9 years ago by rickmastfan67

Alright, I just tested the new attached version. The x2 file works fine now. Also, the crash that I experienced above doesn't happen anymore with the new version.

So, as far as I can tell, this version looks good to go imo. But I'll test it some more overnight and let you know by morning if it's fully behaving and I don't experience any more crashes with it and no more crazy node movement. ;)

comment:20 Changed 9 years ago by rickmastfan67

Ok, I just had something really crazy happen. So, I'm importing some ways from a Tiger 2010 file that I have for a county in PA to replace some old ways. So, I do a replace geometry on one of them to move the old way (all data removed first) to the new way. Then, I do a retrace of the way to clean it up some and then do another replace geometry on it. The problem with that is JOSM all of a sudden hangs up and starts making my CPU go nuts to the point of having to crash JOSM because the replace geometry never completes and JOSM is completely frozen.

So, I can't provide a crash report on this, however, I can attach the file and the steps to see this happening. Here's how to repeat this complete crash of JOSM that doesn't provide an error report:

  1. Once the file is download/opened in JOSM, Ctrl+F and paste the following road name into the search box (minus quotes of course): "Missouri Lane"
  2. Right click in the Selection box and zoom in on the selected object.
  3. Now, zoom in closer and also select the completely blank way.
  4. Now select the "Replace Geometry" command.

The result is JOSM completely freezing up, your CPU starts to go crazy, and you're forced to crash Java to close JOSM and stop the CPU madness.

Here's my JOSM build info (file will be attached afterwards):

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2012-03-11 02:32:23
Last Changed Author: Don-vip
Revision: 5068
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2012-03-11 02:01:33 +0100 (Sun, 11 Mar 2012)
Last Changed Rev: 5068

Identification: JOSM/1.5 (5068 en)
Memory Usage: 122 MB / 2730 MB (84 MB allocated, but free)
Java version: 1.6.0_31, Sun Microsystems Inc., Java HotSpot(TM) 64-Bit Server VM
Operating system: Windows 7

Plugin: OpeningHoursEditor (27852)
Plugin: buildings_tools (27984)
Plugin: licensechange (27964)
Plugin: mapdust (27884)
Plugin: measurement (27957)
Plugin: openstreetbugs (27852)
Plugin: reverter (27865)
Plugin: turnrestrictions (27891)
Plugin: undelete (27852)
Plugin: utilsplugin2 (28028)

Changed 9 years ago by rickmastfan67

Attachment: ticket7295_x4.7z added

comment:21 in reply to:  20 Changed 9 years ago by skyper

Replying to rickmastfan67:

The result is JOSM completely freezing up, your CPU starts to go crazy, and you're forced to crash Java to close JOSM and stop the CPU madness.

Happened to my last night/this morning, too, but I was real tired had just wanted to upload my edits to go to bed. I also was not sure wether JOSM or my system caused the problem.

I used r5068 with openjdk 1.7 on a debian testing with only utilsplugin2 installed. It happened with three layers: data layer invisable but activated, gpx-layer and bing-imagery. I did scroll out real far.

It look to me like there is some problem when java runs out of memory.

Last edited 9 years ago by skyper (previous) (diff)

comment:22 in reply to:  20 Changed 9 years ago by joshdoe

Replying to rickmastfan67:

Ok, I just had something really crazy happen.
[snip]

Okay, it seems to hang up in the Hungarian algorithm, an implementation which I borrowed from here. I've now made this method optional and disabled by default in [o28046] until I get the wrinkles out. You'll need to set utilsplugin2.replace-geometry.robustAssignment to true to enable this method, otherwise it will use the old less robust but more reliable method (for now).

Changed 9 years ago by joshdoe

utils2plugin using Hungarian algorithm for assigning nodes

comment:23 Changed 9 years ago by joshdoe

Please try the attached JAR (renaming necessary), which seem to work okay. The default for the attached JAR is to use this new method. I borrowed another assignment problem algorithm from here. It brought in a number of files, so I'm not sure if I'm including them the best way. No license is stated in the files, but their FAQ indicates the code is GPLv3. I won't commit and make this the default until I get more feedback. Big thanks to James (rickmastfan67) for your thorough bug reporting; keep it coming!

Surprisingly I can't find any standard-ish algorithm libraries for Java, like how C++ has Boost; let me know if I'm missing something obvious.

Last edited 9 years ago by joshdoe (previous) (diff)

comment:24 Changed 9 years ago by rickmastfan67

Josh, I just tested the version you attached above in [5068] and I had no problems. The Replace Geometry function worked correctly on the same way that crashed JOSM for me last night.

Hopefully this will be the last bug with the new method to eliminate.

Anyways Josh, no problemo on the bug reporting. ;) I may not be able to code stuff, but when I do come across a bug and I can reproduce it 100% time, I'll report it and give detailed instructions on how to reproduce it. ;)

comment:25 in reply to:  24 Changed 9 years ago by joshdoe

Replying to rickmastfan67:

Josh, I just tested the version you attached above in [5068] and I had no problems. The Replace Geometry function worked correctly on the same way that crashed JOSM for me last night.

Glad to hear it. I'll wait a bit to commit it. I'm also not sure about pulling these algorithm classes in, so maybe another developer can comment.

comment:26 Changed 9 years ago by akks

On Googled page http://robotics.stanford.edu/~gerkey/tools/hungarian.html there is something about "suffer from the endless loop" problem...

comment:27 in reply to:  23 ; Changed 9 years ago by bastiK

Replying to joshdoe:

No license is stated in the files, but their FAQ indicates the code is GPLv3.

"we plan to release our code under the GNU General Public License, version 3 (GPLv3)." - This isn't enough, maybe just ask the authors for permission?

comment:28 in reply to:  27 Changed 9 years ago by joshdoe

Replying to bastiK:

Replying to joshdoe:

No license is stated in the files, but their FAQ indicates the code is GPLv3.

"we plan to release our code under the GNU General Public License, version 3 (GPLv3)." - This isn't enough, maybe just ask the authors for permission?

Kevin Wayne, the author, gave permission via email to use AssignmentProblem.java and all file dependencies under GPLv3. The license for the rest of the code (as a whole) is still under negotiation with the publisher. I've committed this in [o28116].

comment:29 Changed 9 years ago by rickmastfan67

Hey Josh. Everything has been going fine with the "utilsplugin2.3.jar" version you posted above until today. Just had JOSM freeze once again completely using the "Replace Geometry" function. I'll attach the file after this post.

  1. Ctrl+F and paste "Martin Road" into the search box.
  2. Right click in the Selection box and zoom in on the selected object.
  3. Now, zoom in closer and also select the completely blank way.
  4. Now select the "Replace Geometry" command.

JOSM hangs on attempting to replace this way.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2012-03-25 01:32:08
Last Changed Author: simon04
Revision: 5116
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2012-03-24 13:08:07 +0100 (Sat, 24 Mar 2012)
Last Changed Rev: 5116

Identification: JOSM/1.5 (5116 en)
Memory Usage: 345 MB / 2730 MB (193 MB allocated, but free)
Java version: 1.6.0_31, Sun Microsystems Inc., Java HotSpot(TM) 64-Bit Server VM
Operating system: Windows 7
Dataset consistency test: No problems found

Plugin: OpeningHoursEditor (27852)
Plugin: buildings_tools (27984)
Plugin: licensechange (27964)
Plugin: mapdust (27884)
Plugin: measurement (27957)
Plugin: openstreetbugs (27852)
Plugin: reverter (28089)
Plugin: turnrestrictions (27891)
Plugin: undelete (27852)
Plugin: utilsplugin2 (28045)

Changed 9 years ago by rickmastfan67

Attachment: ticket7295_x5.7z added

comment:30 Changed 9 years ago by rickmastfan67

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

comment:31 Changed 9 years ago by rickmastfan67

Josh, you might want to take a look @ the file attached in #7609 asap.

comment:32 Changed 9 years ago by Cobra

I can reproduce #7609 even with a non-existing .josm directory. Install utilsplugin2, restart josm, open that file, select both ways and replace geometry -> josm hangs.

When splitting the old way "replace me" into two parts (this is what I removed: http://www.openstreetmap.org/browse/way/159497412), replace geometry works fine.

comment:33 Changed 9 years ago by joshdoe

It's a problem with the Dijkstra algorithm. Try the attached JAR. I will probably get rid of the warning (about falling back to the simpler method). Of course it would be nice to fix the underlying issue.

comment:34 Changed 9 years ago by Cobra

This version doesn't block josm and is working correctly. Warns immediately "Exceeded iteration limit for roubust method, using simpler method", then merges the new and old way fine.

comment:35 Changed 7 years ago by rickmastfan67

Resolution: fixed
Status: reopenedclosed

This has been fixed/finished for ages, so, I think we can marked this one as closed. :)

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.