[Patch] Validator doesn't complain about overlapping buildings because rounding happens too late

[GerdP]

What steps will reproduce the problem?

1. Use Buildings Plugin, draw a rectangular building
2. Select the building
3. Press B again to activate buiding plugin
4. Draw a 2nd building starting at one node so that the two buildings touch at one side.
5. Run Validator
6. Save the data to a file
7. Run Validator again

What is the expected result?

Step 4. should produce two nodes which are shared by both ways
Steps 5 and 7 should not produce any warning or informational message

What happens instead?

Only one node is shared.
Step 5 might produce a informational message about overlapping areas
Step 7 probably produces one Warning "Crossing buildings (1)" and now two infos about overlapping areas
Same problem occurs when you upload the data and download again. See also
​https://www.openstreetmap.org/changeset/64137994
I am not sure if we have two different errors here or not, my understanding is that the building plugin should produce two shared nodes, but maybe it doesn't because the algo which detects whether or not a node is on a line or not should use some delta proccessing to handle the rounding problems caused by the spherical calcs.

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

I think the problem here is that JOSM stores the coordinates with high precision and only the safe to file
or upload process rounds the value. A similar problem occured here:
https://josm.openstreetmap.de/ticket/16875#comment:1

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-10-28 22:27:31 +0100 (Sun, 28 Oct 2018)
Build-Date:2018-10-28 21:33:32
Revision:14382
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14382 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 1613 MB / 5461 MB (1353 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34535)
+ apache-commons (34506)
+ buildings_tools (34572)
+ download_along (34503)
+ ejml (34389)
+ geotools (34513)
+ jaxb (34506)
+ jts (34524)
+ measurement (34529)
+ merge-overlap (34664)
+ o5m (34405)
+ opendata (34698)
+ pbf (34576)
+ poly (34546)
+ reverter (34552)
+ undelete (34568)
+ utilsplugin2 (34506)

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

[Klumbumbus]

Regardless the validator, it would be nice if the building tools would actually snap and merge that second node of the second building to the way of the first building.

[GerdP]

Yes, but the problem of the rounding would still persist. I am not sure if this is a problem that should be handled in the validator or in the Coordinate constructor.

[GerdP]

Changing Coordinate is likely to cause side effectss, so maybe all validator routines should use a special getter that would make sure that they see the rounded value for new nodes?

[GerdP]

There is code in the plugin to snap to an existing node (Building.java, findNode()) but it
seems to use a very small distance. I found no code to detect that a node of the new building is close to the edge of another one. I can try to find a solution for this.
Reg. the rounding problems I'd like to hear comments first.

[Klumbumbus]

Replying to GerdP:

but it seems to use a very small distance.

Works fine though.

[GerdP]

I've implemented a first solution for the buildings_tool in patch 16948_buildings-v1.patch.
It seems to work as long as you start the 2nd building on a node of the first.
I've tested these cases: old building is clockwise or ccw, old building has more than one node on the side where the new building is added, new building is longer than old one.

Not yet implemented: A snap to other buildings in case you draw a new building between two or more existing ones.

My normal approach would be this:
Draw the new building, select it, press E to select the nodes, press N to merge the nodes to the nearest way(s).
I first thought I could do the same in the plugin but I found nothing like a cmds.add(new JoinNodeToWaysCommand(...)))
It seems each plugin has to implement these actions on its own?

[GerdP]

16948_buildings-v2.patch now snaps to all buildings. Also, there is no longer the need to start at the node of an existing building.
You just have to draw the building close to one or more others and it will snap to all the buildings which are very close.
I think it would be good to check if the Ctrl key is pressed and avoid any snapping in this case. Where would this be added?

[GerdP]

With 16948_buildings-v3.patch the snapping is disabled when Ctrl is pressed at the time the mouse button is released.
I've also added a check to make sure that we don't snap unusable ways.
I think this is now ready to commit.

[Upliner]

OK, patch is good. I can commit it later this day or you can apply it yourself.

[GerdP]

Fine. I just found some small problems in the java doc. I also have to check if the required JOSM version needs a change.
So, I'll try to fix those and commit later.

[GerdP]

The plugin is now fixed in ​[o34721:34722].

I am now looking for a place to fix the rounding problems. Maybe I'll open a new ticket for that.

[GerdP]

I think the most problematic method is WaySegment.intersects(). This method will return true even when the intersection is extremely close to the end of one of the segments. I've attached a sample file that shows this.
I created two buildings with a shared segment, next I added a node on this shared segment and finally I used G to unglue this node.
When I execute the validator it reports
Error - Building duplicated nodes (1)
Warning - Crossing buildings (1)
I think the 2nd message is wrong, the 1st is misleading. None of the buildings contains the node twice.
The experimental patch crossing.patch changes the test WaySegment.intersects() to tolerate minimal overlaps. It removes the 2nd message
but still find "real" overlaps where they are visible with max. zoom.

[Klumbumbus]

I just tested the plugin update. I don't know if it would be too complex but the snapping in JOSM usually happens already before the mouse click. (The same how the second node of the second building snaps to nodes of the first building.)
As the new snapping on ways distance is pretty short it is unpredictable if I'm close enough for a snap or if I need to draw that second node more precisely on the existing way. Only after the click you know if it was close enough or if you must cancel and start again drawing that second building.

[GerdP]

I'd also like to have that. It should be rather easy as long as we only want to snap the corner where the cursor is and not the other two. Well, I have a look tomorrow.

[Klumbumbus]

Replying to GerdP:

to snap the corner

I meant the edges. Snapping on the corners works fine already.

[GerdP]

Yes, sure. When you draw the rectangle you have three moving corners. I refered to the one under the cursor.

[Klumbumbus]

Other problems I noticed:

• After snapping the first building is no longer rectangular. (Seems to be the reason for "das Extruder Werkzeug (X) in den Ecken ausgefranste Punkte stehen lässt. Man muss also zunächst mit Q alles rechteckig machen." in ​https://forum.openstreetmap.org/viewtopic.php?pid=725014#p725014)
• The snap distance should depend on the distance on screen (pixel distance) so the snapping distance is independent from the current zoom level. (There are already some preference keys which work like that, search for "snap" in the advanced preferences)
• Zoom far in to 0.01 m and draw a small second building on a corner and edge of the first building. --> JOSM crashes (hangs). Also note the missing node in the top right corner of the first building at the moment of the crash.

[GerdP]

OK, yes, the fixed distance is probably not a good idea.

[GerdP]

Hm, calculating nodes with nearly zero distance is quite error prone. I tend to think that the tool should stop to work when the zoom is far too small or too high.

[GerdP]

I think I found a solution for most of the problems. With the patch improve_snap.patch

• the tool uses the configured snap distance (mappaint.node.snap-distance) instead of a hard coded value
• the cursor changes to the "joinway" when appropriate
• the start node is placed on the line, not next to it, this minimizes the distortian of existing buildings

I am not 100% sure whether to use filter OsmPrimitive::isSelectable (used now) or OsmPrimitive::isUsable (used before).
My understanding is that we don't want to snap to filtered objects.

Open problem: When you draw a rectangle the cursor might indicate a snap to line while it is not close to the new building.
Also vice versa. When this happens it is likely that the shape of one of the buildings is changed.

[GerdP]

While writing the above I notived that I missed the case when you draw a new building in three steps. This is now also handled with
improve_snap-v2.patch

[GerdP]

I need help here.
1) The first screenshot shows a scenario where the cursor in not close to any other building but the western corner of the new building is likely to snap to the corner of the existing building. Sorry, screenshot tool doesn't catch the cursor.
When I release the mouse button in this case it is unclear what will happen. If a snap to the corner occurs the created building will not be rectangular. We cannot show that information in the cursor because the cursor itself may also be close to another building, as you see in the second screenshot. My understanding is that we should prefer to create a rectangular shape. Even with improve_snap-v2.patch this isn't the case. I'd like to display this conflict before the mouse button is released, maybe in the cursor or with a change in the colour of the new building or something else. How can I do that?
2) If one presses Ctrl in the second situation he probably wants to avoid the snap to the corner, but the result would be no snap at all and thus a duplicated node and similar problems. So I think we should store the information about Ctrl status for each node that is created with a mouse click. Should I change the code to do that?

[GerdP]

I've tried to fix the problems in ​​[o34724:34725]. I did not find a solution for the case where one might expect a snap at one of the moving corners to existing nodes or lines.
It now works this way:
When you draw the building the cursor snaps to existing ways or lines, when you click in that situation the stored position is extremely close to an existing object. When the drawing process is finished the building is only connected with those very close objects.
So, if you want to snap to more than one corner you have to start at the right place, else the new building might only snap to the line close to the corner.

[Klumbumbus]

I tested it and it works fine for me. Great thanks.

The "problems" shown in your screenshots in comment:24 can easily be avoided by not selecting the big building before. You usually select another building only if you draw seperate buildings, not when you draw attached buildings. So the shown situations should not really occour at real mapping.

...the result would be no snap at all and thus a duplicated node and similar problems. So I think we should store the information about Ctrl status for each node that is created with a mouse click. Should I change the code to do that?

That could be an nice enhancement. But I think the situations when you need CTRL at a building the has one merged node and one not merged but very close node are very rare in real mapping too, so no big issue.

[GerdP]

Thanks for testing. I also think that the remaining problems are small and there is always a way to avoid them.
So I think the problems caused by the plugin are solved, but the general problem with the rounding remains. I'll open a new ticket for that and think about a better description for this one.

[Klumbumbus]

Added to MOTD:
https://josm.openstreetmap.de/wiki/StartupPageSource?action=diff&version=2432

[GerdP]

The problems in building_tools are solved.

[Klumbumbus]

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

[Klumbumbus]

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