Modify

Opened 5 months ago

Last modified 9 days ago

#16803 reopened defect

[Patch RFC] Validator: Wrong warning Highway link is not linked to adequate highway/link

Reported by: GerdP Owned by: team
Priority: normal Milestone: 19.02
Component: Core validator Version:
Keywords: template_report Cc:

Description (last modified by Klumbumbus)

What steps will reproduce the problem?

  1. Open attached sample OSM file
  2. Run validator

What is the expected result?

No warning

What happens instead?

Highway link is not linked to adequate highway/link

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

It is a common mapping method to draw one way which describes two separate link roads and I think it is correct. When I split the way at the node on the primary road the warning disappears. Maybe the validator code could "simulate" that split?


URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-10-01 23:59:36 +0200 (Mon, 01 Oct 2018)
Build-Date:2018-10-01 22:08:47
Revision:14289
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14289 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 2665 MB / 5376 MB (1522 MB allocated, but free)
Java version: 1.8.0_162-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)
+ jts (34524)
+ measurement (34529)
+ merge-overlap (34664)
+ o5m (34405)
+ opendata (34675)
+ pbf (34576)
+ poly (34546)
+ reverter (34552)
+ utilsplugin2 (34506)

Last errors/warnings:
- W: Conflicts detected - <html>There were 8 conflicts detected.</html>
- W: Conflicts detected - <html>There was 1 conflict detected.</html>
- W: Conflicts detected - <html>There was 1 conflict detected.</html>
- W: Unsaved changes - <html>The relation has been changed.<br><br>Do you want to save your changes?</html>
- E: Handled by bug report queue: java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location
- E: org.openstreetmap.josm.io.OsmApiException: ResponseCode=504, Error Header=<timeout. The server is probably too busy to handle your request.>, Error Body=<<?xml version="1.0" encoding="UTF-8"?>
- E: Communication with OSM server failed - ResponseCode=504, Error Header=&lt;timeout. The server is probably too busy to handle your request.&gt;, Error Body=&lt;&lt;?xml version="1.0" encoding="UTF-8"?&gt;
- W: java.net.SocketException: Unexpected end of file from server
- E: org.openstreetmap.josm.io.OsmTransferException: java.net.SocketException: Unexpected end of file from server. Cause: java.net.SocketException: Unexpected end of file from server
- E: Network exception - <html>Failed to open a connection to the remote server<br>'https://api.openstreetmap.org/api/0.6/'.<br>Please check your internet connection.</html>

Attachments (9)

sample.osm (25.1 KB) - added by GerdP 5 months ago.
16803.png (18.8 KB) - added by Klumbumbus 5 months ago.
sample2.osm (161.8 KB) - added by GerdP 4 months ago.
funny.osm (4.6 KB) - added by GerdP 3 months ago.
Sample to play with different road types
16803.patch (4.9 KB) - added by GerdP 2 months ago.
16803-v2.patch (10.5 KB) - added by GerdP 2 months ago.
16803-v3.patch (10.5 KB) - added by GerdP 2 months ago.
16803-v4.patch (10.7 KB) - added by GerdP 3 weeks ago.
use named constant for angle 60
partially_outside_download_area.osm (2.4 KB) - added by taylor.smock 3 weeks ago.
A sample of what I would expect to be flagged (and is, under current code)

Download all attachments as: .zip

Change History (33)

Changed 5 months ago by GerdP

Attachment: sample.osm added

comment:1 Changed 5 months ago by GerdP

Thought again about this. Although it is a common way to map these roads with a single way the way needs to be split when a destination is added. Therefore JOSM should recognize this special case and propose a split instead of claiming that the wrong link type is used.
Esp. in this sample case JOSM doesn't complain when the way is tagged highway=unclassified_link, which is for sure a change to the worse (it was tagged like this in the past)

Changed 5 months ago by Klumbumbus

Attachment: 16803.png added

comment:2 Changed 5 months ago by Klumbumbus

Description: modified (diff)

Changed 4 months ago by GerdP

Attachment: sample2.osm added

comment:3 Changed 4 months ago by GerdP

I've just added another sample that - IMHO - also produces two wrong warnings. I don't yet understand the code that is responsible for this:

            // It is possible to have a class_link between 2 segments of same class
            // in roundabout designs that physically separate a specific turn from the main roundabout
            // But if we have more than a single adjacent class, and one of them is a roundabout, that's an error
            for (Way w : sameClass) {
                if (w.hasTag("junction", "roundabout")) {
                    return false;
                }
            }

Last edited 4 months ago by Don-vip (previous) (diff)

comment:4 Changed 4 months ago by GerdP

@Don-vip:
I found ticket #14891 which was the reason for this code. I don't agree with comment3. Why not highway=primary_link? The wiki for link roads explicitely mentions roundabouts: https://wiki.openstreetmap.org/wiki/Highway_link#Roundabouts
and I found no hint in the wiki for roundabouts that one should not use link roads for roundabout flares.

comment:5 Changed 4 months ago by GerdP

Any comments? I really think that JOSM produces false error messages and thus forces mappers to change correct data to bad data.

Changed 3 months ago by GerdP

Attachment: funny.osm added

Sample to play with different road types

comment:6 Changed 2 months ago by Don-vip

Component: CoreCore validator

comment:7 Changed 2 months ago by GerdP

I asked in the German forum(1) and my conclusion was that it is too complex to find a "good" value for the highway link.
I'll work on a patch.

(1) https://forum.openstreetmap.org/viewtopic.php?id=64327

Changed 2 months ago by GerdP

Attachment: 16803.patch added

comment:8 Changed 2 months ago by GerdP

The attached patch relaxes the isHighwayLinkOkay() method:

  • if one of the nodes of the way is outside of the download area it returns true
  • if a <class>_link way is connected to a <class> way it returns true
  • if it is connected to another way with the same <class>_link and the connection doesn't form a sharp angle it returns true.

(currently angles < 80 are considered sharp, maybe 60 would be better)

So, the major change is that it will also accept when e.g. a highway=tertiary_link is connected to a highway=secondary and a highway=tertiary or tertiary_link.
The wiki as well as some mappers in the german forum don't say that the <class> in a <class>_link MUST be the
highest connected <class>.
I am not sure if we should treat trunk_link and motorway_link different. In Germany those two are special, in other countries trunk is
used similar to primary.

comment:9 Changed 2 months ago by GerdP

Forget the patch, angle calculation needs more logic.

Changed 2 months ago by GerdP

Attachment: 16803-v2.patch added

Changed 2 months ago by GerdP

Attachment: 16803-v3.patch added

comment:10 Changed 2 months ago by GerdP

Summary: Validator: Wrong warning Highway link is not linked to adequate highway/link[Patch RFC] Validator: Wrong warning Highway link is not linked to adequate highway/link

Please review:
See also comment:8, I think with v3 I've got the angle calculations right. I've now implemented that a _link road that is connected to a motorway or trunk must have the higher class. Maybe this should depend on country (territories)?

I've also changed the unit test because I think the example in
josm\core\test\data\regress\14891\14891.osm.bz2 should not produce two warnings about wrong highway links.

comment:11 Changed 2 months ago by GerdP

No comments? Should I add a unit test similar to that for multipolygons so that we can collect some special cases and see how the code handles them? I think it is easier to maintain the units test in JOSM where you can visualize the data.

comment:12 Changed 3 weeks ago by taylor.smock

I don't see anything wrong with it.
That said, based off of the comments on my patches, I probably missed something, or am about to say something wrong.

Comments I have:
For isSharpAngle(node, node, node), why did you pick 60? (And now I now about Geometry.getCornerAngle)
Maybe use a pref for that, in case someone wants to have a sharper or gentler angle?

On line 250 for isSharpAngle(way, int, way), why do you have a check for a roundabout? I get that roundabouts are considered to be oneway=yes, but shouldn't way.isOneway() return true if it is a junction=roundabout with no oneway tag?

I like how you check to make certain the entire way is inside the download area (shouldn't there be a method for that though? It seems like something that might come up in other tests).

I'm assuming it still doesn't support split roundabouts (#12841), since you just changed the line to have split instead of splitted.

comment:13 in reply to:  12 ; Changed 3 weeks ago by GerdP

Replying to taylor.smock:

I don't see anything wrong with it.

Thanks for review :)

That said, based off of the comments on my patches, I probably missed something, or am about to say something wrong.

Comments I have:
For isSharpAngle(node, node, node), why did you pick 60? (And now I now about Geometry.getCornerAngle)
Maybe use a pref for that, in case someone wants to have a sharper or gentler angle?

The value 60 just worked fine for me. If you see a need for an option I can implement that.

On line 250 for isSharpAngle(way, int, way), why do you have a check for a roundabout? I get that roundabouts are considered to be oneway=yes, but shouldn't way.isOneway() return true if it is a junction=roundabout with no oneway tag?

Yes, I also was surprised that way.isOneway() is not coded that way. I think junction=roundabout did not always imply oneway=yes.

I like how you check to make certain the entire way is inside the download area (shouldn't there be a method for that though? It seems like something that might come up in other tests).

Yes, good point. I'll double check this. Tests like this appear in many tests and also in many actions.

I'm assuming it still doesn't support split roundabouts (#12841), since you just changed the line to have split instead of splitted.

Yes, the patch doesn't try to improve the roundabout check. I just corrected the spelling.

comment:14 in reply to:  13 ; Changed 3 weeks ago by taylor.smock

From the wiki page on roundabouts (https://wiki.openstreetmap.org/wiki/Tag:junction%3Droundabout), I got

oneway=yes is implied and redundant. Though it is not wrong to add it explicitly, it is not needed. However it may be useful to tag the number of lanes=* in the ring (typically 2, where long vehicles will need to use both; 1-lane roundabouts are especially important to tag if they exist for large, but usually roundabouts the second lane, or are transformed to miniroundabouts whose central island is not blocking but can be used at very slow speed by long vehicles).

So I would think that way.isOneway() should return true when junction=roundabout and oneway!=no.

One option would be to overload Way.isOneway() to also have Way.isOneway(boolean include_implied) for backwards compatibility.

As far as the hardcoded value of 60, I was just wondering if it was the best idea to hardcode it. It is probably doesn't matter either way, I just figured it would be easier to change the value if needed later.

Changed 3 weeks ago by GerdP

Attachment: 16803-v4.patch added

use named constant for angle 60

comment:15 in reply to:  14 Changed 3 weeks ago by GerdP

One option would be to overload Way.isOneway() to also have Way.isOneway(boolean include_implied) for backwards compatibility.

I fear this will be difficult. Think about country specific rules for highway=trunk. In Germany it implies oneway=yes, in other countries it doesn't. I guess that's the reason why Way.isOneway() is so basic.

As far as the hardcoded value of 60, I was just wondering if it was the best idea to hardcode it. It is probably doesn't matter either way, I just figured it would be easier to change the value if needed later.

See new patch. I have problems to describe what the value really means ;) That's one reason why I did not create a preference for it.

comment:16 Changed 3 weeks ago by taylor.smock

Way.isOneway() could become very complex very fast if we do that, I suppose.

I still think that it should return true if it is always an implied oneway, but that should probably be a different bug report / feature request.

I have problems myself naming things, so I can see why you might not create a preference for it.

It turns out there is a Way.isOutsideDownloadArea() method that checks each node, but looking at the code, it doesn't make a difference (although it might be better to set a switch of some type, and if a higher classification road isn't found, return that), e.g., you have a link that starts outside the downloaded area and connects to a higher classification inside the downloaded area.

Last edited 3 weeks ago by taylor.smock (previous) (diff)

comment:17 in reply to:  16 Changed 3 weeks ago by GerdP

Replying to taylor.smock:

It turns out there is a Way.isOutsideDownloadArea() method that checks each node, but looking at the code, it doesn't make a difference (although it might be better to set a switch of some type, and if a higher classification road isn't found, return that), e.g., you have a link that starts outside the downloaded area and connects to a higher classification inside the downloaded area.

I fear I don't understand. The test can only produce a warning "Highway link is not linked to adequate highway/link". How do we know that this is the case when don't know all the connected ways? Can you add a unit test to show that there is a (new) problem?

Changed 3 weeks ago by taylor.smock

A sample of what I would expect to be flagged (and is, under current code)

comment:18 Changed 3 weeks ago by GerdP

I don't see that this is flagged without the patch. And it shouldn't be because there might be a highway=primary connected to the northern (end) node of the link road which is not visible because it wasn't downloaded.

comment:19 Changed 3 weeks ago by taylor.smock

Looks like you are right.
I probably have a file modified somewhere that was giving me erroneous results.

I withdraw my comment:

(although it might be better to set a switch of some type, and if a higher classification road isn't found, return that), e.g., you have a link that starts outside the downloaded area and connects to a higher classification inside the downloaded area.

comment:20 Changed 3 weeks ago by GerdP

Milestone: 19.02

OK, so I think I'll commit the patch after release of 19.1

comment:21 Changed 10 days ago by GerdP

Resolution: fixed
Status: newclosed

In 14772/josm:

fix #16803 Validator: Wrong warning Highway link is not linked to adequate highway/link
(16803-v4.patch)

comment:22 Changed 9 days ago by GerdP

In 14779/josm:

see #16803: revert r14777,r14775, and r14772: too many side effects, patch needs more review

comment:23 Changed 9 days ago by GerdP

Resolution: fixed
Status: closedreopened

comment:24 Changed 9 days ago by GerdP

Patch causes some new false positives for correct roundabouts and link roads.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to GerdP
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.