[PATCH] stricter checking for highways crossing waterways without bridges

[RicoZ]

Currently many mappers tag whole rivers with layer=-1 because it is an easy way to avoid validator warnings of the type "highway crossing waterway".

Enhance the check so that keepright warns whenever a highway crosses a waterway where there is no bridge or tunnel even if the layer is different. A layer tag itself does not make a valid crossing.

This has been discussed in the wiki ( ​https://wiki.openstreetmap.org/wiki/Talk:Key:layer#layer.3D-1_erroneously_used_for_many_italian_rivers ) and previously the update to ​https://wiki.openstreetmap.org/wiki/Key:layer has been discussed in the mailing list ( ​https://lists.openstreetmap.org/pipermail/talk/2014-March/069219.html )

Related but more general: #9182

[mkoniecz]

It may be more general - with multiple crossing highway=* with different layers no more than one should be without tunnel=*/bridge=*

[ssprunk]

I agree it should be stricter. However, even the basic checking appears to be broken now; I removed layer=-1 from several waterways near me, resulting in dozens of same-layer crossings, yet I still get no warnings. I know it worked a few months ago because I spent hours fixing all the warnings I got (in a different part of town).

[RicoZ]

@ssprunk: I believe this warning message was reclassified to "info" message so you may need to enable it explicitly. There were just too many of them and many are unfixable without local knowledge but were annoying and repeatedly prompted users to hide the messages by adding layer=-1 to whole rivers. Running in circles.

[ssprunk]

@RicoZ: I enabled "info" messages and still didn't get anything. I did get one "warning" yesterday, but only one; there were dozens of bad crossings in the current layer. High-res satellite (available everywhere I map) is enough to decide bridge vs culvert, so I had no problem quickly fixing hundreds of other problems before the warnings disappeared a while back.

[skyper]

Replying to ssprunk:

@RicoZ: I enabled "info" messages and still didn't get anything. I did get one "warning" yesterday, but only one; there were dozens of bad crossings in the current layer. High-res satellite (available everywhere I map) is enough to decide bridge vs culvert, so I had no problem quickly fixing hundreds of other problems before the warnings disappeared a while back.

Did you make sure to run validator manually and on the whole data layer (nothing selected) ?
Please upload an example file where validator is not producing any warning or info about the two crossing ways. Thanks

[ssprunk]

example of missing crossing ways warnings

[ssprunk]

Replying to skyper:

Did you make sure to run validator manually and on the whole data layer (nothing selected) ?
Please upload an example file where validator is not producing any warning or info about the two crossing ways. Thanks

Yes, I did it with nothing selected (or all selected). I've uploaded a file (nowarn.osm) that has lots of crossings that should warn but don't; it took me a while to find some I hadn't already fixed. Confirmed again that it doesn't warn, even with such a small dataset.

[Klumbumbus]

Replying to ssprunk:

Yes, I did it with nothing selected (or all selected). I've uploaded a file (nowarn.osm) that has lots of crossings that should warn but don't; it took me a while to find some I hadn't already fixed. Confirmed again that it doesn't warn, even with such a small dataset.

Atm JOSM doesn't warn for crossing ways if a layer tag is present. However in your attached file for the most of crossings there is neither a layer tag nor a bridge/tunnel tag. These are reported by validator. I get 39 warnings about crossing highway/waterway. If it doesn't warn for you check the following

• use the newest stable version of JOSM
• check all the checkboxes at the validator preferences
• check that the file ignorederrors in your JOSM preference folder is empty

[ssprunk]

Replying to Klumbumbus:

• check that the file ignorederrors in your JOSM preference folder is empty

Doh! That was it, thanks. It finds most of the problems now, including where someone put bridge=* or tunnel=* with no layer=*, but I'm guessing (back to the original point) it's still missing cases where there is a layer=* with no bridge=* or tunnel=*.

[skyper]

Replying to ssprunk:

(back to the original point) it's still missing cases where there is a layer=* with no bridge=* or tunnel=*.

#11256 seems to be a duplicate, then.

[Klumbumbus]

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

[Klumbumbus]

Replying to skyper:

Replying to ssprunk:

(back to the original point) it's still missing cases where there is a layer=* with no bridge=* or tunnel=*.

#11256 seems to be a duplicate, then.

yes

[Gabe]

This rule checks ways with layer tag that is missing both bridge or tunnel tag. Message tells user to add at least one of them.

[GerdP]

I think that's too simple.

1. layer=0 should be ignored here (they are flagged in another test)
2. it will tell the user to add bridge or tunnel to e.g. this way ​https://www.openstreetmap.org/way/263030872 instead of telling him that the layer tag is possibly obsolete.

[Gabe Reichenberger <v-garei@…>]

Replying to GerdP:

I think that's too simple.

1. layer=0 should be ignored here (they are flagged in another test)
2. it will tell the user to add bridge or tunnel to e.g. this way ​https://www.openstreetmap.org/way/263030872 instead of telling him that the layer tag is possibly obsolete.

Ignoring layer=0 is an easy addition.
To address your second point, throwWarning with a message stating either bridge/tunnel needed with layer tag OR layer tag is obsolete?

Would this resolve the patch properly?

[GerdP]

In fact I was surprised about the idea that a highway must have bridge=* or tunnel=* when there is a need for a layer tag.
I thought there must be other cases like service ways in different levels of an undergroud parking but maybe those are tagged with level=* instead of layer=*.
It's difficult to flag those problems without risking that users do the wrong thing.
Maybe something less aggressive like "tag layer=* might be obsolete" and use the group "suspicious tag combination"

[anonymous]

Note the title - it is only for roads crossing waterways.

Road crossing road/rail is a trickier case and in some rare cases they may cross with neither having bridge or tunnel tag (footway/road on overhang, in building etc)

[GerdP]

OK, I read comment:9 as "highway + layer=* should only be used with bridge or tunnel".
If this should be restricted to highways crossing waterways it cannot be done with mapcss.

[skyper]

Depending on the way length, I get an informal or "normal" warning about waterway=* + layer=* but no tunnel or bridge. Maybe, similar is useful for highway=*. Plus, I would raise the level of both by one, e.g. warning for short ways and error for long ways.

Additionally, there could be a warning about solo layer=* without other useful tags. I wounder why this was not implemented, yet, see #20902.

[Gabe Reichenberger <v-garei@…>]

so just to get this all straight we want the following:

Specifically for highways crossing waterways -->

• if there is layer=* on the highway=*/waterway=* way an error/warning should be thrown saying bridge=* or tunnel=* needs to be added?

Solo layer tag warning -->

• if there is any feature with only a layer=* throw a warning suggesting other useful tags?
• This is referring to a feature that has only one tag, layer=*?

[skyper]

Replying to Gabe Reichenberger <v-garei@…>:

so just to get this all straight we want the following:

Specifically for highways crossing waterways -->

• if there is layer=* on the highway=*/waterway=* way an error/warning should be thrown saying bridge=* or tunnel=* needs to be added?

So far there is the discussion if:

• this test should do what is requested for in the ticket description: Only checking highway crossing waterway (only possible in java code)
• high-/rail-/waterway with layer but no bridge or tunnel (probably few more keys) should be warned about, in general.

In the second case, the warning should not demand to add tunnel or bridge but to inspect the situation as e.g. removing layer might be the correct solution.

Solo layer tag warning -->

• if there is any feature with only a layer=* throw a warning suggesting other useful tags?
• This is referring to a feature that has only one tag, layer=*?

Yes, sadly, I did not figure out, how to only count "interesting" keys and exclude e.g. fixme, comment and description from counting but that is better discussed on #20902.

[Gabe Reichenberger <v-garei@…>]

this patch covers highways/waterways/railways with a lone layer tag. Either add complimentary tags (bridge/tunnel or other necessary tag)

[Gabe Reichenberger <v-garei@…>]

Replying to skyper:

• high-/rail-/waterway with layer but no bridge or tunnel (probably few more keys) should be warned about, in general.

9819_highway_waterway_railway.patch​ should cover this scenario and has been moderately tested in each scenario (highway/waterway/railway)

[Gabe Reichenberger <v-garei@…>]

deleted

[Gabe Reichenberger <v-garei@…>]

deleted

[skyper]

Replying to Gabe Reichenberger <v-garei@…>:

…
I WISH WE COULD EDIT COMMENTS! Sorry for the comment explosion, just wanted to make sure the link was understandable.

This should be possible if you are logged in. I am even able to edit any comment but maybe I have some more rights than "normal" users

[skyper]

Replying to Gabe Reichenberger <v-garei@…>:

Replying to skyper:

• high-/rail-/waterway with layer but no bridge or tunnel (probably few more keys) should be warned about, in general.

9819_highway_waterway_railway.patch​ should cover this scenario and has been moderately tested in each scenario (highway/waterway/railway)

Thanks, looks like mapcss isn't that hard to learn.

Some thoughts:

• combinations.mapcss​ is probably the better place for this test
• layer=0 should be ignored. There is already a warning about it, I think, and it is a false positive for this test.
• Do we want to include areas?
  • all including MPs?
• There might be a few exceptions for waterway but I have to check.
• I would split the test in three lines, for each *way one, which makes it easier to add the exceptions which might be different per *way.
• If using placeholder in the warning why not using it all the way?

way!:closed[highway][layer][layer!=0][!bridge][!tunnel][!covered][highway!~/steps|elevator/],
way!:closed[railway][layer][layer!=0][!bridge][!tunnel][!covered],
way!:closed[waterway][layer][layer!=0][!bridge][!tunnel][!covered] {
  throwWarning: tr("inspect to confirm a {1} tag is necessary on this {0}. If so, add either {2}, {3}, {4} or any other tags complimentary to {5}", "{2.key}", "{3.key}", "{4.key}=*", "{5.key}=*", "{6.key}=*", "{3.key}=*");

• Could be added to group: tr("suspicious tag combination"); or own group like suspicious layer tag instead of the big more general group.

[reichg]

added discussed changes

[reichg]

This covers the original issue with highways crossing waterways which have a layer tag but not a bridge/tunnel tag.

[reichg]

9819_highwayCrossingWaterwayOnly.patch​ covers the following:

• Specifically highway crossing waterway with a layer tag but missing bridge/tunnel tag. There is probably a better way to do this in the java file but this currently addresses the original post.

[skyper]

Replying to skyper:

Some thoughts:

• Do we want to include areas?
  • all including MPs?
• There might be a few exceptions for waterway but I have to check.

Good news, I did not find any obvious value for waterway and railway as false positive, so far.
This changes if we allow areas like railway=station. At the moment, I would vote for including all areas but that can be added later on once this patch was accepted.

[anonymous]

Replying to skyper:

Replying to skyper:

Some thoughts:

• Do we want to include areas?
  • all including MPs?
• There might be a few exceptions for waterway but I have to check.

Good news, I did not find any obvious value for waterway and railway as false positive, so far.
This changes if we allow areas like railway=station. At the moment, I would vote for including all areas but that can be added later on once this patch was accepted.

Nice! Looks like everything is addressed for this patch in the 2 patch files 9819_highway_water_railway_v2.patch and 9819_highwayCrossingWaterwayOnly.patch within this ticket and then in ticket 20902 the lone layer tag is still being addressed.

[skyper]

I am not sure if both patches are needed and they will produce duplicate warnings.
Additionally the two informal warnings for waterways with layer depending on way length should be removed as they are covered by the patch for *ways.

[reichg]

Replying to skyper:

I am not sure if both patches are needed and they will produce duplicate warnings.
Additionally the two informal warnings for waterways with layer depending on way length should be removed as they are covered by the patch for *ways.

I will see if I can sync both patches somehow to not produce duplicate warnings. Does the logic in each patch make sense?
I will address the 2 informal warnings.

[reichg]

Seems like the original issue is covered with the mapcss patch. Without my java patch highways crossing waterways with layer=* but no complimentary tags is being flagged.

Conclusion:

• Keep mapcss additions but remove the way length tests?

[reichg]

This has the mapcss addition necessary to cover original post issue AND some new validations for crossing ways. Also moved areLayerOrLevelDifferent() function to visit function so future validations for crossing ways will be less restricted by the ignoreWaySegmentCombination() function.

[skyper]

Sorry, I did not take a look at the code of the "waylenght" waterway warning. Now I spotted some more exclusions which I am not sure about:
[culvert!=yes][covered!=yes][pipeline!=yes][location!=underground]

The discussion on #9182 should be taken into account, too, though some might have changed over the years.
Anyway, adding some more exclusions can be done after we get complains.

@Gabe
On top of the mapcss test, we usually add a short comment about it including the ticket numbers. Especially, the numbers are really helpful. Would you mind to add one. Thanks

[reichg]

added comment for ticket number. Future exclusions can be added if complained about.

[reichg]

Replying to skyper:

Sorry, I did not take a look at the code of the "waylenght" waterway warning. Now I spotted some more exclusions which I am not sure about:
[culvert!=yes][covered!=yes][pipeline!=yes][location!=underground]

The discussion on #9182 should be taken into account, too, though some might have changed over the years.
Anyway, adding some more exclusions can be done after we get complains.

@Gabe
On top of the mapcss test, we usually add a short comment about it including the ticket numbers. Especially, the numbers are really helpful. Would you mind to add one. Thanks

Yup, added comment. 9819_final_v1.patch is ready and can easily be modified if other exclusions are necessary in the future.

[reichg]

Can this ticket be resolved?

[skyper]

Waiting some days after a new testing release is needed for hotfixes. Additionally, a core maintainer needs to have some time.

Found another ticket which would be solved by this general test: #10875

[reichg]

Still need a maintainer to merge this patch

[reichg]

Can we get this merged?

[skyper]

Set milestone as told by Dirk.

[anonymous]

Replying to skyper:

Set milestone as told by Dirk.

What is the purpose of these milestones?

Not anonymous, from Gabe

[skyper]

Milestones are usually in sync with releases (usually at the end of each month) and tickets with a matching milestones should be closed before the release. Dirk Stoecker wrote on another ticket, that for tickets with patches ready for commit a milestone should be set as an reminder. Prior, milestone was only set by core developers.

[Don-vip]

Sorry, I didn't have time to look at it yet, will do for next release.

[reichg]

can we get this looked at for 21.08?

[Don-vip]

The amount of comments on this ticket frightens me each time I look at it and I again did not have enough time to see what it is all about, sorry.

[GerdP]

What's the idea of the change in the java code?

I don't fully understand the message produced by the mapcss rule:
"suspicious tag combination - inspect to confirm a layer tag is necessary on this layer. If so, add either bridge=*, tunnel=*, covered=* or any other tags complimentary to layer=*"
What is meant with "or any other tags complimentary to layer=*"?

I still fear that inexperienced mappers might not understand that the layer tag might be obsolete and instead add one of the listed tags to get rid of the warning.

[Don-vip]

Milestone renamed

[GerdP]

@reichg: Seems my comment came to late?

[skyper]

Comment 48 is addressed to Gabe.

[gaben]

Entschuldigung, what I have to do here? :)

[skyper]

Replying to gaben:

Entschuldigung, what I have to do here? :)

Entschuldigung, seems we have several persons with first name Gabe and I thought it is a single person with multiple accounts. I am sorry for the mess.