Modify

Opened 2 weeks ago

Closed 13 days ago

Last modified 13 days ago

#17158 closed defect (fixed)

"building tool" plugin produces error when create a build over a node with "addr:housenumber" tag and member of relation

Reported by: oleksii.zagorskyi@… Owned by: Upliner
Priority: normal Milestone:
Component: Plugin buildings_tools Version: latest
Keywords: template_report Cc:

Description

What steps will reproduce the problem?

Have a node with "addr:housenumber=16" tag which is a member of a relation (assotiatedStreet), use "building tool" to draw a build over the node.
The existing node being merged with the new build, preserving "addr:housenumber=16" to the new build.

What is the expected result?

It should work without errors in plugin.

What happens instead?

When you do 2nd click to create the new build - an error happens in the "building tool" plugin.
If ignore the error and continue (which is bad, I know), the build is created, but it's not a member of the relation and data is actually broken.

Please provide any additional information below. Attach a screenshot if possible.

NOTE: if draw a build not over the node, but on a side, then move the node over the build and use "Data -> Merge address point" menu - it works great, but it's complicated.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-11-28 01:09:01 +0100 (Wed, 28 Nov 2018)
Build-Date:2018-11-28 00:26:41
Revision:14460
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14460 en) Linux Ubuntu 18.04.1 LTS
Memory Usage: 714 MB / 910 MB (115 MB allocated, but free)
Java version: 1.8.0_121-b13, Oracle Corporation, Java HotSpot(TM) Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080

Dataset consistency test:
[NO DATASET] {Way id=-157355 version=0 VT nodes=[{Node id=-157351 version=0 V lat=50.36605247966937,lon=30.41936695307493}, {Node id=-157354 version=0 V lat=50.36605068553871,lon=30.41953852437232}, {Node id=-157353 version=0 V lat=50.36594631877727,lon=30.41953584216331}, {Node id=-157352 version=0 V lat=50.36594811291188,lon=30.419364270865916}, {Node id=-157351 version=0 V lat=50.36605247966937,lon=30.41936695307493}]} is referenced by {Relation id=6796038 version=3 VT [node 4784243388, way 460398654, node 4784243387, node 4784243383, way 459892042, way 460398678, node 4784243384, node 4784243386, way 460398711, node 4784243385, way 460398714, way 460398653, node 4784243381, way 460398682, node 4784243380, way 460398683, node 4784243379, way 460398656, way 460398720, way 460398685, way -157355, way 460398686, way 170077933]} but not found in dataset


Plugins:
+ BuildingGeneralization (23)
+ DirectDownload (34593)
+ DirectUpload (34502)
+ ElevationProfile (34746)
+ FixAddresses (34511)
+ HouseNumberTaggingTool (34517)
+ ImageWayPoint (34206)
+ InfoMode (34755)
+ PicLayer (34544)
+ ShapeTools (1240)
+ alignways (34489)
+ buildings_tools (34724)
+ editgpx (34751)
+ gpxfilter (34506)
+ imagery_offset_db (34641)
+ log4j (34527)
+ measurement (34529)
+ namemanager (34532)
+ openvisible (34536)
+ poly (34546)
+ reltoolbox (34788)
+ reverter (34552)
+ tageditor (34560)
+ todo (30306)
+ turnrestrictions (34643)
+ undelete (34568)
+ utilsplugin2 (34506)

Tagging presets:
+ https://raw.githubusercontent.com/yopaseopor/traffic_signs_preset_JOSM/master/UA.zip

Last errors/warnings:
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- E: Handled by bug report queue: org.openstreetmap.josm.data.osm.DataIntegrityProblemException: Relation member must be part of the same dataset as relation(relation 6796038, way -155903)
- E: Handled by bug report queue: org.openstreetmap.josm.data.osm.DataIntegrityProblemException: Relation member must be part of the same dataset as relation(relation 6796038, way -157355)

Attachments (0)

Change History (11)

comment:1 Changed 2 weeks ago by anonymous

Component: CorePlugin buildings_tools
Owner: changed from team to Upliner

comment:2 Changed 2 weeks ago by anonymous

Summary: "building tool" plugin crashes when create a build over a node with "addr:housenumber" tag and member of relation"building tool" plugin produces error when create a build over a node with "addr:housenumber" tag and member of relation

comment:3 Changed 2 weeks ago by anonymous

I've found what is the bug in the plugin code and I've fixed it.

Here is a simple patch:

Index: src/org/openstreetmap/josm/plugins/buildings_tools/Building.java
===================================================================
--- src/org/openstreetmap/josm/plugins/buildings_tools/Building.java    (revision 34797)
+++ src/org/openstreetmap/josm/plugins/buildings_tools/Building.java    (working copy)
@@ -385,13 +385,14 @@
         }
         cmds.add(new AddCommand(ds, w));

-        addAddress(w);
-
         if (snap) {
             snapBuildings(w, nodes, cmds);
         }
         Command c = new SequenceCommand(tr("Create building"), cmds);
         UndoRedoHandler.getInstance().add(c);
+
+        addAddress(w);
+
         return w;
     }

comment:4 Changed 2 weeks ago by stoecker

Seems correct. It would be even better, when in this case addAddress would add its commands to "cmds" instead of creating a separate undo action. Untested, does this work:

  • src/org/openstreetmap/josm/plugins/buildings_tools/Building.java

     
    336336        Collections.sort(ways);
    337337        w = ways.get(0);
    338338
    339         addAddress(w);
     339        addAddress(w, null);
    340340
    341341        return w;
    342342    }
     
    385385        }
    386386        cmds.add(new AddCommand(ds, w));
    387387
    388         addAddress(w);
     388        addAddress(w, cmds);
    389389
    390390        if (snap) {
    391391            snapBuildings(w, nodes, cmds);
     
    451451        }
    452452    }
    453453
    454     private void addAddress(Way w) {
     454    private void addAddress(Way w, Collection<Command> cmdList) {
    455455        if (ToolSettings.PROP_USE_ADDR_NODE.get()) {
    456456            Node addrNode = getAddressNode();
    457457            if (addrNode != null) {
    458                 Collection<Command> addressCmds = new LinkedList<>();
     458                Collection<Command> addressCmds = cmdList != null ? cmdList : new LinkedList<>();
    459459                for (Entry<String, String> entry : addrNode.getKeys().entrySet()) {
    460460                    w.put(entry.getKey(), entry.getValue());
    461461                }
     
    472472                    addressCmds.add(new ChangeCommand(r, rnew));
    473473                }
    474474                addressCmds.add(new DeleteCommand(addrNode));
    475                 Command c = new SequenceCommand(tr("Add address for building"), addressCmds);
    476                 UndoRedoHandler.getInstance().add(c);
     475                if (cmdList == null) {
     476                    Command c = new SequenceCommand(tr("Add address for building"), addressCmds);
     477                    UndoRedoHandler.getInstance().add(c);
     478                }
    477479            }
    478480        }
    479481    }

comment:5 Changed 2 weeks ago by stoecker

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

comment:6 Changed 2 weeks ago by stoecker

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

comment:7 Changed 2 weeks ago by oleksii

Still, don't forget to move "addAddress(w, cmds);" 5 lines down.

comment:8 Changed 2 weeks ago by stoecker

No, that would be wrong. My patch includes the changes into the SequenceCommand and thus the order should be correct.

comment:9 Changed 13 days ago by stoecker

Resolution: fixed
Status: newclosed

comment:10 Changed 13 days ago by oleksii.zagorskyi@…

As for consistency ...
In a function createCircle() the addAddress() is called as very last step before return;

While in the similar (affected) function createRectangle(), the addAddress() was/is called NOT as very last step before return;

That was a way I fixed the issue - call addAddress() as very last before return;

I'm not a programmer, so don't want to argue :), but it looks to me like a not consistent thing.
I've tested your commit and it works good, I'm satisfied.

comment:11 Changed 13 days ago by stoecker

;-) You don't need to look at the code positions itself, but on the relevant logic.

Before addAddress() was called before the CommandStack action "Create building" was done, but used the elements of this action. Now addAddress() is part of the same CommandStack and thus in correct order again.

createCircle() is still suboptimal, as it creates more than one Command item, but at least it is in correct order.

Last edited 13 days ago by stoecker (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Upliner.
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.