Modify

Opened 8 years ago

Last modified 9 months ago

#9819 needinfo defect

[PATCH] stricter checking for highways crossing waterways without bridges

Reported by: RicoZ Owned by: RicoZ
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 6 years ago.
example of missing crossing ways warnings
9819.patch (825 bytes) - added by Gabe 14 months 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@…> 14 months 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 14 months ago.
added discussed changes
9819_highwayCrossingWaterwayOnly.patch (3.3 KB) - added by reichg 14 months 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 14 months 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 14 months ago.
added comment for ticket number. Future exclusions can be added if complained about.

Download all attachments as: .zip

Change History (57)

comment:1 Changed 8 years ago by mkoniecz

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

comment:2 Changed 6 years ago by 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).

comment:3 Changed 6 years ago by 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.

comment:4 in reply to:  3 ; Changed 6 years ago by 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.

comment:5 in reply to:  4 ; Changed 6 years ago by 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

Changed 6 years ago by ssprunk

Attachment: nowarn.osm added

example of missing crossing ways warnings

comment:6 in reply to:  5 ; Changed 6 years ago by 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.

comment:7 in reply to:  6 ; Changed 6 years ago by 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

comment:8 in reply to:  7 ; Changed 6 years ago by 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=*.

comment:9 in reply to:  8 ; Changed 6 years ago by 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.

comment:10 Changed 6 years ago by Klumbumbus

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

comment:11 in reply to:  9 Changed 6 years ago by 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

comment:12 Changed 6 years ago by skyper

Cc: Penegal added

comment:13 Changed 6 years ago by skyper

Keywords: crossing layer bridge tunnel added

Changed 14 months ago by Gabe

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 Changed 14 months ago by anonymous

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

comment:15 Changed 14 months ago by 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.

comment:16 in reply to:  15 Changed 14 months ago by 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?

comment:17 Changed 14 months ago by 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"

comment:18 Changed 14 months ago by 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)

comment:19 Changed 14 months ago by 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.

comment:20 Changed 14 months ago by 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.

comment:21 in reply to:  19 ; Changed 14 months ago by 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=*?

comment:22 in reply to:  21 Changed 14 months ago by 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.

Last edited 14 months ago by skyper (previous) (diff)

Changed 14 months ago by 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)

comment:23 Changed 14 months ago by 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)

Last edited 14 months ago by skyper (previous) (diff)

comment:24 in reply to:  23 ; Changed 14 months ago by Gabe Reichenberger <v-garei@…>

deleted

Last edited 14 months ago by skyper (previous) (diff)

comment:25 in reply to:  24 ; Changed 14 months ago by Gabe Reichenberger <v-garei@…>

deleted

Last edited 14 months ago by skyper (previous) (diff)

comment:26 in reply to:  25 Changed 14 months ago by 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

Last edited 14 months ago by skyper (previous) (diff)

comment:27 in reply to:  23 ; Changed 14 months ago by 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.

Changed 14 months ago by reichg

added discussed changes

Changed 14 months ago by reichg

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

comment:28 Changed 14 months ago by 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.

comment:29 in reply to:  27 ; Changed 14 months ago by 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.

comment:30 in reply to:  29 ; Changed 14 months ago by 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.

comment:31 in reply to:  30 ; Changed 14 months ago by 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.

comment:32 in reply to:  31 Changed 14 months ago by 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.

Last edited 14 months ago by reichg (previous) (diff)

comment:33 Changed 14 months ago by 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?

Changed 14 months ago by reichg

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 Changed 14 months ago by skyper

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 14 months ago by skyper (previous) (diff)

Changed 14 months ago by reichg

Attachment: 9819_final_v1.patch added

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

comment:35 in reply to:  34 Changed 14 months ago by 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.

Last edited 14 months ago by reichg (previous) (diff)

comment:36 Changed 13 months ago by reichg

Can this ticket be resolved?

comment:37 Changed 13 months ago by 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

Last edited 13 months ago by skyper (previous) (diff)

comment:38 Changed 13 months ago by reichg

Still need a maintainer to merge this patch

comment:39 Changed 13 months ago by reichg

Cc: Klumbumbus added

comment:40 Changed 12 months ago by reichg

Can we get this merged?

comment:41 Changed 12 months ago by skyper

Milestone: 21.06

Set milestone as told by Dirk.

comment:42 Changed 12 months ago by Don-vip

Milestone: 21.0621.07

comment:43 in reply to:  41 Changed 12 months ago by anonymous

Replying to skyper:

Set milestone as told by Dirk.

What is the purpose of these milestones?

Not anonymous, from Gabe

Last edited 12 months ago by reichg (previous) (diff)

comment:44 Changed 12 months ago by 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.

comment:45 Changed 11 months ago by Don-vip

Milestone: 21.0721.08

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

comment:46 Changed 10 months ago by reichg

can we get this looked at for 21.08?

comment:47 Changed 10 months ago by Don-vip

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 Changed 9 months ago by 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.

comment:49 Changed 9 months ago by Don-vip

Milestone: 21.0921.10

Milestone renamed

comment:50 Changed 9 months ago by GerdP

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

@reichg: Seems my comment came to late?

Modify Ticket

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

Add Comment


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

 
Note: See TracTickets for help on using tickets.