Modify

Opened 6 months ago

Closed 5 months ago

Last modified 5 months ago

#18420 closed defect (fixed)

Move node onto ways not working anymore

Reported by: sharcrash@… Owned by: sharcrash@…
Priority: normal Milestone: 19.12
Component: Core Version:
Keywords: template_report Cc: GerdP

Description

What steps will reproduce the problem?

  1. pressing n shortcut while having selected one node near several close ways

What is the expected result?

connect

What happens instead?

nothing

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

It used to work

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-12-01 23:10:15 +0100 (Sun, 01 Dec 2019)
Build-Date:2019-12-02 02:30:57
Revision:15553
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15553 en) Windows 7 64-Bit
OS Build number: Windows 7 Professional (7601)
Memory Usage: 1228 MB / 1820 MB (354 MB allocated, but free)
Java version: 1.8.0_231-b11, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1280x1024, \Display1 1280x1024
Maximum Screen Size: 1280x1024
Dataset consistency test: No problems found

Plugins:
+ SimplifyArea (34977)
+ areaselector (359)
+ austriaaddresshelper (57)
+ buildings_tools (35171)
+ continuosDownload (82)
+ contourmerge (v0.1.5)
+ ejml (35122)
+ geochat (35163)
+ imagery_offset_db (34908)
+ log4j (34908)
+ reltoolbox (35196)
+ reverter (35226)
+ turnrestrictions (34977)
+ undelete (34977)
+ utilsplugin2 (35238)
+ wikipedia (1.1.3)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Fixme&zip=1
- https://github.com/bastik/mapcss-tools/raw/osm/mapnik2mapcss/osm-results/mapnik.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface&zip=1
+ https://github.com/gmgeo/osmic-josm-style/archive/master.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/Enhanced_Lane_and_Road_Attributes&zip=1

Last errors/warnings:
- W: java.net.SocketTimeoutException: Read timed out
- W: java.net.SocketTimeoutException: Read timed out
- W: java.net.SocketTimeoutException: Read timed out
- W: java.net.SocketTimeoutException: Read timed out
- W: java.net.SocketTimeoutException: Read timed out
- W: java.net.SocketTimeoutException: Read timed out
- W: java.net.SocketTimeoutException: Read timed out
- E: Could not get any sources:
- E: Could not get any sources:
- E: Could not get any sources:

Attachments (5)

18420-sample.osm (2.6 KB) - added by GerdP 5 months ago.
bad.PNG (7.8 KB) - added by GerdP 5 months ago.
case 4 old version
bad2.PNG (43.9 KB) - added by GerdP 5 months ago.
case 4 old version zoomed out several times
''node onto way'' won't attach both ways at once.osm (5.8 KB) - added by sharcrash@… 5 months ago.
Sample situation
18420.patch (5.5 KB) - added by GerdP 5 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 months ago by Don-vip

Owner: changed from team to sharcrash@…
Status: newneedinfo

Can you please attach a sample .osm file showing the problem?

comment:2 Changed 5 months ago by Don-vip

Cc: GerdP added

Could it be a regression of r15428?

Changed 5 months ago by GerdP

Attachment: 18420-sample.osm added

comment:3 in reply to:  2 Changed 5 months ago by GerdP

Replying to Don-vip:

Could it be a regression of r15428?

Hard to say without an example. The results depend on the distance and the zoom and the resolution of the screen and a preference setting.
Load attached example and select the single node. On my PC (1920x1080 screen resolution) I see this
Case 1 Press 3 (zoom to selection) followed by n: Nothing happens
Case 2: Press - (zoom out) followed by n: Node is joined to the closest building
Case 3: undo, 3, zoom out 2 or 3 times, n : Node is joined to the closest building
Case 4: undo, 3, zoom out 4 or more times, n : Popup saying that there are multiple target ways

I think Case 1 is the problem here. Reason for the behaviour is that the action selects way segments within a fixed range of screen pixels using NavigatableComponent.getNearestWaySegmentsImpl(). The range value is stored in preference mappaint.segment.snap-distance, default is 10, for screens with a high resoltion this might be too small.

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

Changed 5 months ago by GerdP

Attachment: bad.PNG added

case 4 old version

Changed 5 months ago by GerdP

Attachment: bad2.PNG added

case 4 old version zoomed out several times

comment:4 Changed 5 months ago by GerdP

Before r15428 the behaviour was different for Case 4: The action connected two or more building in a rather unpredictable way:
case 4 old version
If n is pressed when zoomed out far the result is complete weird:
case 4 old version zoomed out several times

Version 0, edited 5 months ago by GerdP (next)

comment:5 Changed 5 months ago by sharcrash@…

Hello! Sorry for the late reply, i totally had forgotten.

Here is a sample with a road centred between a forest area and a grass area. Some nodes are not connected and would be useful to smoothen the road direction according real situation. If i press n to attached the node to both areas at once, it doesn't do it and we get the message: multiple target ways, no common point found, nothing happened. It used to work though in a previous JOSM version. Not sure when/which sorry, because sometimes i don't update directly.

Changed 5 months ago by sharcrash@…

Sample situation

comment:6 Changed 5 months ago by GerdP

Ah, yes, I think this might be caused by the changes in r15428, it is a variant of case 4 described above.
Not sure if the old code worked well in this case, but I'll try to change the code so that it selects the closest of multiple segments in "snap distance".
As a work around you can select the way to which the node should be moved.

comment:7 Changed 5 months ago by GerdP

I am not sure where to fix this problem. I've learned that the results returned from NavigatableComponent.getNearestWaySegmentsImpl() are not reliable when zoomed out far. It should return the nearest way segments ordered by perpendiculer distance but it doesn't. It returns the wanted segments, but the order is not always correct. The problem is that the position of the selected node is rounded to int values BEFORE doing the distance calculations. The more you zoom out the larger the rounding error.
The current algortihm in JoinNodeWayAction relies on the correct order.
So, I could either implement the sorting in JoinNodeWayAction again using the precise positions or we could change the interface to one of the oldest routines in JOSM to use Point2D instead of Point.

comment:8 Changed 5 months ago by GerdP

Clarification: The code in NavigatableComponent.getNearestWaySegmentsImpl() might be correct, it just doesn't make much sense to do the sorting based on a Point rounded to int.
Anyway, there is also a problem in the JoinNodeWayAction code, so I'll try to fix that first using a locally sorted list.

Changed 5 months ago by GerdP

Attachment: 18420.patch added

comment:9 Changed 5 months ago by GerdP

The algorithm is quite complex because it allows multiple nodes and target ways. Goal is to find the closest way segment and to move the node onto that segment, but also onto other segments which are overlapping or almost overlapping at the place where the node is moved. I hope I got it right now.
I found a possible reason for the random unit test failures. I changed field data from HashMap to LinkedHashMap because we iterate over the elements and that should not be done in a random order. I'll try again to code stable tests including the problems in this ticket...

comment:10 Changed 5 months ago by GerdP

Resolution: fixed
Status: needinfoclosed

In 15610/josm:

fix #18420: Move node onto ways not working anymore

  • sort result from NavigatableComponent again using higher precision
  • improve handling of multiple target ways with similar distance so that the node is added to the closest segment and also to segments with almost the same position but NOT to segments further away or with a similar distance but different position.
  • fix possible error when multiple nodes are selected
  • use LinkedHashMap for field data to avoid random results from iterator (hope this fixes the unit test problems)

comment:11 Changed 5 months ago by GerdP

Milestone: 19.12

comment:12 Changed 5 months ago by sharcrash@…

That should help us working faster and if needed to connect a node to multiple ways, pressing again n should do the trick now. Thanks!

Modify Ticket

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