Modify

Opened 10 years ago

Last modified 12 months ago

#9819 needinfo defect

[PATCH] stricter checking for highways crossing waterways without bridges

Reported by: RicoZ Owned by: reichg
Priority: normal Milestone:
Component: Core validator Version:
Keywords: crossing layer bridge tunnel Cc: Penegal, AlfonZ, mkoniecz, Klumbumbus

Description

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

Attachments (7)

nowarn.osm (358.7 KB ) - added by ssprunk 7 years ago.
example of missing crossing ways warnings
9819.patch (825 bytes ) - added by Gabe 3 years ago.
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.
9819_highway_waterway_railway.patch (1.2 KB ) - added by Gabe Reichenberger <v-garei@…> 3 years ago.
this patch covers highways/waterways/railways with a lone layer tag. Either add complimentary tags (bridge/tunnel or other necessary tag)
9819_highway_water_railway_v2.patch (1.3 KB ) - added by reichg 3 years ago.
added discussed changes
9819_highwayCrossingWaterwayOnly.patch (3.3 KB ) - added by reichg 3 years ago.
This covers the original issue with highways crossing waterways which have a layer tag but not a bridge/tunnel tag.
9819_combined_v1.patch (4.4 KB ) - added by reichg 3 years ago.
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.
9819_final_v1.patch (4.5 KB ) - added by reichg 3 years ago.
added comment for ticket number. Future exclusions can be added if complained about.

Download all attachments as: .zip

Change History (60)

comment:1 by mkoniecz, 10 years ago

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

comment:2 by ssprunk, 7 years ago

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).

comment:3 by RicoZ, 7 years ago

@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.

in reply to:  3 ; comment:4 by ssprunk, 7 years ago

@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.

in reply to:  4 ; comment:5 by skyper, 7 years ago

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

by ssprunk, 7 years ago

Attachment: nowarn.osm added

example of missing crossing ways warnings

in reply to:  5 ; comment:6 by ssprunk, 7 years ago

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.

in reply to:  6 ; comment:7 by Klumbumbus, 7 years ago

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

in reply to:  7 ; comment:8 by ssprunk, 7 years ago

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=*.

in reply to:  8 ; comment:9 by skyper, 7 years ago

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.

comment:10 by Klumbumbus, 7 years ago

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

in reply to:  9 comment:11 by Klumbumbus, 7 years ago

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

comment:12 by skyper, 7 years ago

Cc: Penegal added

comment:13 by skyper, 7 years ago

Keywords: crossing layer bridge tunnel added

by Gabe, 3 years ago

Attachment: 9819.patch added

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.

comment:14 by anonymous, 3 years ago

Summary: stricter checking for highways crossing waterways without bridges[PATCH] stricter checking for highways crossing waterways without bridges

comment:15 by GerdP, 3 years ago

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.

in reply to:  15 comment:16 by Gabe Reichenberger <v-garei@…>, 3 years ago

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?

comment:17 by GerdP, 3 years ago

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"

comment:18 by anonymous, 3 years ago

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)

comment:19 by GerdP, 3 years ago

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.

comment:20 by skyper, 3 years ago

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.

in reply to:  19 ; comment:21 by Gabe Reichenberger <v-garei@…>, 3 years ago

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=*?

in reply to:  21 comment:22 by skyper, 3 years ago

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.

Last edited 3 years ago by skyper (previous) (diff)

by Gabe Reichenberger <v-garei@…>, 3 years ago

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

comment:23 by Gabe Reichenberger <v-garei@…>, 3 years ago

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)

Last edited 3 years ago by skyper (previous) (diff)

in reply to:  23 ; comment:24 by Gabe Reichenberger <v-garei@…>, 3 years ago

deleted

Last edited 3 years ago by skyper (previous) (diff)

in reply to:  24 ; comment:25 by Gabe Reichenberger <v-garei@…>, 3 years ago

deleted

Last edited 3 years ago by skyper (previous) (diff)

in reply to:  25 comment:26 by skyper, 3 years ago

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

Last edited 3 years ago by skyper (previous) (diff)

in reply to:  23 ; comment:27 by skyper, 3 years ago

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.

by reichg, 3 years ago

added discussed changes

by reichg, 3 years ago

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

comment:28 by reichg, 3 years ago

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.

in reply to:  27 ; comment:29 by skyper, 3 years ago

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.

in reply to:  29 ; comment:30 by anonymous, 3 years ago

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.

in reply to:  30 ; comment:31 by skyper, 3 years ago

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.

in reply to:  31 comment:32 by reichg, 3 years ago

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.

Last edited 3 years ago by reichg (previous) (diff)

comment:33 by reichg, 3 years ago

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?

by reichg, 3 years ago

Attachment: 9819_combined_v1.patch added

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.

comment:34 by skyper, 3 years ago

Cc: AlfonZ mkoniecz added

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

Last edited 3 years ago by skyper (previous) (diff)

by reichg, 3 years ago

Attachment: 9819_final_v1.patch added

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

in reply to:  34 comment:35 by reichg, 3 years ago

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.

Last edited 3 years ago by reichg (previous) (diff)

comment:36 by reichg, 3 years ago

Can this ticket be resolved?

comment:37 by skyper, 3 years ago

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

Last edited 3 years ago by skyper (previous) (diff)

comment:38 by reichg, 3 years ago

Still need a maintainer to merge this patch

comment:39 by reichg, 3 years ago

Cc: Klumbumbus added

comment:40 by reichg, 3 years ago

Can we get this merged?

comment:41 by skyper, 3 years ago

Milestone: 21.06

Set milestone as told by Dirk.

comment:42 by Don-vip, 3 years ago

Milestone: 21.0621.07

in reply to:  41 comment:43 by anonymous, 3 years ago

Replying to skyper:

Set milestone as told by Dirk.

What is the purpose of these milestones?

Not anonymous, from Gabe

Last edited 3 years ago by reichg (previous) (diff)

comment:44 by skyper, 3 years ago

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.

comment:45 by Don-vip, 3 years ago

Milestone: 21.0721.08

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

comment:46 by reichg, 2 years ago

can we get this looked at for 21.08?

comment:47 by Don-vip, 2 years ago

Milestone: 21.0821.09

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.

comment:48 by GerdP, 2 years ago

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.

comment:49 by Don-vip, 2 years ago

Milestone: 21.0921.10

Milestone renamed

comment:50 by GerdP, 2 years ago

Milestone: 21.10
Owner: changed from team to RicoZ
Status: newneedinfo

@reichg: Seems my comment came to late?

comment:51 by skyper, 12 months ago

Owner: changed from RicoZ to gaben

Comment 48 is addressed to Gabe.

comment:52 by gaben, 12 months ago

Entschuldigung, what I have to do here? :)

in reply to:  52 comment:53 by skyper, 12 months ago

Owner: changed from gaben to reichg

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as needinfo The owner will remain reichg.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from reichg to the specified user. Next status will be 'new'.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from reichg to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.