[patch] Route relations with split start do not show links as expected in the relation editor

[matthijs]

When defining routes, there can be splits in the route when the route going forward uses different ways than the route going back (e.g. with one-way streets or separately mapped cycleways on both sides of the street). This uses the "forward" and "backward" roles. When such a split happens in the middle of a route, the relation editor nicely reflects this in the third "link" column.

In the Dutch cycle node network, routes are frequently split at the start or end as well. This means that a given cycle node number is present in two or more places (close together, e.g. at opposite sides of a bridge or road), since there are multiple places where you can arrive at a given cycle node and find directions to move on to the next node (depending on where you are coming from). For example, see ​here for how something would typically look.

There has been some ​recent discussion (in Dutch, sorry) about how to best map this. Ideally, one would like to map each route until the first of these multiple nodes that a route encounters, and use one ore more additional routes to connect the nodes with the same number (for routing purposes).

However, in some cases, such as the one shown above, this would result in a route where the start and/or end of the route is split (e.g. the starting point is one node, but when you traverse the route in reverse, the ending point is another node). Currently, JOSM does not have a meaningful connection graph in the relation editor for the case with a split start of the route (a split end of the route is handled correctly).

This is illustrated by the attached file, which contains a few ways and two routes: One where the start and end are split, and one route where additional ways are added to the route to join them (so that both directions use the same start and ending points). Below, the "split" route is selected:

In the relation editor, the split route looks messy at the start, but as expected at the end:

The joined route, does look ok in the relation editor at the start and end:

Because of this messy display, it is hard to check that a route is correct. This has led to a practice where routes are closed up at the end with additional one-way route members to make JOSM render the connection graph properly (this is also done with the real-world example I linked above). However, it would be better if JOSM could render this situation better, leaving more freedom to the mappers.

Ideally, I think this situation could be drawn like this:

I've made a patch that actually implements this, I'll push it to a github PR next. The code that handles this is quite complicated, so it took me a while to find the right place to add this. I haven't done very thorough testing yet, this probably also warrants a unit test (but I couldn't get ant test to work quickly, probably because my Junit version is 3.8 and/or because the issues shown at ​https://github.com/openstreetmap/josm/pull/60), so I'm leaving that for later.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-07-30 12:44:04 +0200 (Thu, 30 Jul 2020)
Revision:16812
Build-Date:2020-07-31 01:30:49
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (16812 en) Linux Ubuntu 19.10
Memory Usage: 1413 MB / 2048 MB (33 MB allocated, but free)
Java version: 11.0.7+10-post-Ubuntu-2ubuntu219.10, Ubuntu, OpenJDK 64-Bit Server VM
Look and Feel: javax.swing.plaf.metal.MetalLookAndFeel
Screen: :0.0 1280x1024 (scaling 1.0x1.0)
Maximum Screen Size: 1280x1024
Best cursor sizes: 16x16 -> 16x16, 32x32 -> 32x32
Java package: openjdk-11-jre:amd64-11.0.7+10-2ubuntu2~19.10
WebStart package: icedtea-netx:amd64-1.8-0ubuntu8
Java ATK Wrapper package: libatk-wrapper-java:all-0.35.0-3
libcommons-logging-java: libcommons-logging-java:all-1.2-2
fonts-noto: fonts-noto:-
VM arguments: [--add-reads=java.base=ALL-UNNAMED,java.desktop, --add-reads=java.desktop=ALL-UNNAMED,java.naming, --add-reads=java.naming=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.awt=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/javax.jnlp=ALL-UNNAMED,java.desktop, --add-exports=java.base/com.sun.net.ssl.internal.ssl=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.net.www.protocol.jar=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.action=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.provider=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.util=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.validator=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.x509=ALL-UNNAMED,java.desktop, --add-exports=java.base/jdk.internal.util.jar=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.net.www.protocol.http=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.awt.X11=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.applet=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.applet=ALL-UNNAMED,jdk.jsobject, --add-exports=java.naming/com.sun.jndi.toolkit.url=ALL-UNNAMED,java.desktop, -Dicedtea-web.bin.name=javaws, -Dicedtea-web.bin.location=/usr/lib/icedtea-web/bin/javaws]
Dataset consistency test: No problems found

Plugins:
+ measurement (35405)
+ utilsplugin2 (35487)

[matthijs]

Example showing problem

[matthijs]

Pullrequest is here: ​https://github.com/openstreetmap/josm/pull/61

[skyper]

See #6166 for the problem sorting these relations.

Thought there is a ticket about this problem with displaying the connectivity, as well, but I do not find it.

[simon04]

Hi, thank you for your patch! It breaks one unit test in WayConnectionTypeCalculatorTest, can you check/fix that?

org.junit.ComparisonFailure: 
Expected :[FPH FORWARD, FP FORWARD, NONE, FPH FORWARD, NONE, FPH FORWARD, NONE]
Actual   :[FP FORWARD, FP FORWARD, NONE, BPH BACKWARD, NONE, FPH FORWARD, NONE]

	at org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculatorTest.testLoop(WayConnectionTypeCalculatorTest.java:129)
	at org.openstreetmap.josm.testutils.JOSMTestRules$TimeoutThread.run(JOSMTestRules.java:777)

If possible, please include your Split-end routes.osm​ in source:trunk/nodist/data/relation_sort.osm​ and write a unit test in WayConnectionTypeCalculatorTest.

[matthijs]

Thought there is a ticket about this problem with displaying the connectivity, as well, but I do not find it.

Seems #6179 is about this, but it was closed as a duplicate of the sorting ticket.

I hadn't checked sorting yet, it seems sorting for these kinds of relations does not work yet. I'll have a look, but properly sorting them might not be so easy, I'm afraid.

As for testing, I got the test suite running locally now (apparently the Debian ant-optional package ​misses the ant-junitlaucher.jar file, but downloading a copy into /usr/share/ant works around that). I also see that the headless tests run properly on github and indeed flag this test failure as well. Running single tests looked a bit cumbersome, so I added an ant target for that, see ​https://github.com/openstreetmap/josm/pull/62

The one failing test was partly because of a missing check in my code (I had considered it, decided it wasn't needed, but this testcase incidentally shows a case where it is needed), and partly because the relation in the testcase is a bit of a mess (even after sorting, which I think is the point of this test, to show that sorting is imperfect, there is also a TODO in the code). The messy part of that relation is rendered somewhat differently by my patch, so I'm just going to updated the expected output there.

I'll also add my testdata as a testcase (and maybe see if I can fix sorting). I probably will not have time for that today, hopefully somewhere in the coming days.

[skyper]

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

[matthijs]

I fixed up the failing testcase, added testcases and fixed sorting along with some other minor changes. See the PR for details.

[simon04]

In 16886/josm:

fix #19633 - Route relations with split start do not show links as expected in the relation editor (patch by matthijskooijman)

[Klumbumbus]

There is a new PMD warning: https://josm.openstreetmap.de/jenkins/job/JOSM/6691/pmdResult/

[matthijs]

Hm, what's the policy about these warnings?

The warning is about code that looks like:

if (something)
  return true;
if (something_else)
  return true;
return false;

This can be simplified to:

if (something)
  return true;
return something_else;

However, I would actually prefer the code as it is right now, since the code clearly shows that there's two cases that are checked, defaulting to false otherwise. In the simplified version, the structure of the code is somewhat lost (because then one condition is inside the if and the other after the return).

If strict PMD-warning-free code is the policy, maybe the code should be restructured something like this:

boolean nodeSplits = ...;
boolean nodeIsLoopStart = ...;
return nodeSplits || nodeIsLoopStart;

(names are up for discussion, but can actually improve code readability if chosen properly).

Note that the code could be even more simplified to return something || something_else, but since the actual expressions are way longer, that would be really hard to read IMHO.

[simon04]

In 16890/josm:

see #19633 - PMD

[simon04]

Replying to Matthijs Kooijman <matthijs@…>:

Hm, what's the policy about these warnings?

We fix them and keep the warning count at 0. Done.

[matthijs]

Ok, thanks!

[Klumbumbus]

Replying to simon04:

In 16890/josm:

see #19633 - PMD

That however created java warnings :) https://josm.openstreetmap.de/jenkins/job/JOSM/6694/warnings9Result/

[simon04]

In 16896/josm:

see #19633 - Java Warnings

[skyper]

Sadly, I found a situation where it still does not work, see #20605.