Opened 6 years ago
Last modified 21 months ago
#16803 reopened defect
[Patch needs rework] Validator: Wrong warning Highway link is not linked to adequate highway/link
| Reported by: | GerdP | Owned by: | team |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | Core validator | Version: | |
| Keywords: | template_report | Cc: |
Description (last modified by )
What steps will reproduce the problem?
- Open attached sample OSM file
- 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=<timeout. The server is probably too busy to handle your request.>, Error Body=<<?xml version="1.0" encoding="UTF-8"?> - 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)
Change History (37)
by , 6 years ago
| Attachment: | sample.osm added |
|---|
comment:1 by , 6 years ago
by , 6 years ago
comment:2 by , 6 years ago
| Description: | modified (diff) |
|---|
by , 6 years ago
| Attachment: | sample2.osm added |
|---|
comment:3 by , 6 years ago
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; } }
comment:4 by , 6 years ago
@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 by , 6 years ago
Any comments? I really think that JOSM produces false error messages and thus forces mappers to change correct data to bad data.
comment:6 by , 6 years ago
| Component: | Core → Core validator |
|---|
comment:7 by , 6 years ago
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.
by , 6 years ago
| Attachment: | 16803.patch added |
|---|
comment:8 by , 6 years ago
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.
by , 6 years ago
| Attachment: | 16803-v2.patch added |
|---|
by , 6 years ago
| Attachment: | 16803-v3.patch added |
|---|
comment:10 by , 6 years ago
| 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 by , 6 years ago
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.
follow-up: 13 comment:12 by , 6 years ago
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.
follow-up: 14 comment:13 by , 6 years ago
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:
ForisSharpAngle(node, node, node), why did you pick60? (And now I now aboutGeometry.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 beoneway=yes, but shouldn'tway.isOneway()return true if it is ajunction=roundaboutwith noonewaytag?
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
splitinstead ofsplitted.
Yes, the patch doesn't try to improve the roundabout check. I just corrected the spelling.
follow-up: 15 comment:14 by , 6 years ago
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.
comment:15 by , 6 years ago
One option would be to overload
Way.isOneway()to also haveWay.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.
follow-up: 17 comment:16 by , 6 years ago
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.
comment:17 by , 6 years ago
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?
by , 6 years ago
| Attachment: | partially_outside_download_area.osm added |
|---|
A sample of what I would expect to be flagged (and is, under current code)
comment:18 by , 6 years ago
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 by , 6 years ago
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 by , 6 years ago
| Milestone: | → 19.02 |
|---|
OK, so I think I'll commit the patch after release of 19.1
comment:23 by , 6 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → reopened |
comment:24 by , 6 years ago
Patch causes some new false positives for correct roundabouts and link roads.
comment:25 by , 6 years ago
| Milestone: | 19.02 → 19.03 |
|---|
comment:26 by , 6 years ago
| Milestone: | 19.03 |
|---|
comment:27 by , 4 years ago
| Summary: | [Patch RFC] Validator: Wrong warning Highway link is not linked to adequate highway/link → [Patch needs rework] Validator: Wrong warning Highway link is not linked to adequate highway/link |
|---|
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)