[patch] Add railway junction check for missing switches and crossings

[gaben]

If there is a Y connection on a railway, there is usually a ​Tag:railway=switch or ​Tag:railway=railway_crossing. The following lines will issue a warning if neither is on the junction node.

way[/railway$/] >[index=1] node[/railway$/!~/switch|railway_crossing/]:connection,
way[/railway$/] >[index=-1] node[/railway$/!~/switch|railway_crossing/]:connection {
        set .railway_first_last_connection;
}

way[/railway$/] >[index!=1][index!=-1] node.railway_first_last_connection {
        group: tr("missing tag");
        throwWarning: tr("use one of railway features: switch, railway_crossing");
}

It's only a partial solution as I couldn't think out a rule which works when the railway ways split at the Y junction. I wanted this to be entirely MapCSS for other services benefit, like osmose.

[taylor.smock]

Would it help if I dusted off my patch for #16998? I think the test would look like the following, but I'd have to check.

way[/railway$/] > node[/railway$/!~/switch|railway_crossing/]:connection {
    set .railway_first_last_connection;
}

way[/railway$/] > node[count(parent_osm_primitives("railway")) > 1].railway_first_last_connection {
    group: tr("missing tag");
    throwWarning: tr("use one of railway features: {0}, {1}", "switch", "railway_crossing");
}

[skyper]

railway=razed, railway=abandoned and railway=construction might lead to false positives. Personally, I use prefix for switches and crossings, too, e.g. razed:railway=switch or construction:railway=railway_crossing.

I guess a warning for situations where two or more ways are crossing with one common middle node is missing. This should be the default for railway=railway_crossing as there is usually no need to split the ways at the intersection.

I'd prefer a throwWarning: tr("Railways connection node without {0} or {1}, "railway=switch", "railway=railway_crossing");

[gaben]

Replying to taylor.smock:

Would it help if I dusted off my patch for #16998?

Yes, I think. The patch I provided is catching the most common errors (try it out on a country size railway data), but as skyper pointed out, there is room for improvement. With your added function from #16998 all cases can be covered.

Replying to skyper:
Probably I didn't get what you mean by default. Anyway, here is a PDF from the State of the Map 2018 conference with ​examples on various crossings. It shows that a simple X junction can be a switch and a railway_crossing as well.

[skyper]

Replying to gaben:

Replying to skyper:
Probably I didn't get what you mean by default.

I mean, the tags of the tracks on both sides of railway_crossing usually do not change and unlike switches relations will never lead to splitting the ways at a railway_crossing. Therefore railway_crossings should be on middle nodes (e.g. the default). We could even think about a (informal) warning for railway=railway_crossing on end nodes.

[gaben]

Good idea. I think it can be even a warning level message.

Another false positive: turntable <> rail connections.

[gaben]

New version, please test.

way[/railway$/!~/turntable|traverser|roundhouse|workshop/] >[index=1] node[/railway$/!~/switch|railway_crossing/]:connection,
way[/railway$/!~/turntable|traverser|roundhouse|workshop/] >[index=-1] node[/railway$/!~/switch|railway_crossing/]:connection {
        set .railway_first_last_connection;
}

way[/railway$/][/railway$/!~/turntable|traverser|roundhouse|workshop/] >[index!=1][index!=-1] node.railway_first_last_connection {
        group: tr("missing tag");
        throwWarning: tr("use one of railway features: {0}, {1}", "switch", "railway_crossing");
}

node[railway=~/switch|railway_crossing/]!:connection {
        throwWarning: tr("disconnected railway junction");
}

Also ping @taylor.smock for #16998.

[taylor.smock]

Replying to gaben:

Also ping @taylor.smock for #16998.

Sorry -- I've been banging my head against kendzi3d (I'm porting it from JOGL to LWJGL, so it can (a) run on Java 9+ and (b) run on ARM Macs so that when we move to Java 17, we don't have users staying back because that one plugin doesn't work). I've gone ahead and added reminder to update and merge #16998 on March 10 in my calendar.

As a heads up, I'm treating the JOSM source code as if it were in "Feature Freeze" ("Stabilization") right now for anything I merge -- we've been releasing on the first weekend of the month, when we have releases, but the wiki:/DevelopersGuide/Schedule page says that it should be the last weekend of the month. So I'm kind of leery about doing anything that isn't specific to a bug fix around those two weekends right now. I should probably ping Don-vip and see if we have officially changed the release schedule.

[gaben]

This time it's pong. The validation could be a good candidate for the next release.

[taylor.smock]

I just merged #16998. Was there anything from it that you wanted to use?

[gaben]

Yes, probably I was waiting for the parent_osm_primitives function. I have to refresh my memory :)

[gaben]

So with the new feature from r18829 I created this rule:

way[/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=1] node[/railway$/ !~ /switch|railway_crossing/]:connection,
way[/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=-1] node[/railway$/ !~ /switch|railway_crossing/]:connection {
        set .side_connection;
        throwWarning: "selected side connection";
}

way[/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=1] node[/railway$/ !~ /switch|railway_crossing/][count(parent_osm_primitives("railway")) > 2]:connection,
way[/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=-1] node[/railway$/ !~ /switch|railway_crossing/][count(parent_osm_primitives("railway")) > 2]:connection {
        set .three_way_connection;
        throwWarning: "selected full Y connection";
}

way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index!=1][index!=-1] node.side_connection,
way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] > node.three_way_connection:connection {
        group: tr("missing tag");
        throwError: tr("Railways connection node without {0} or {1}", "railway=switch", "railway=railway_crossing");
}

node[/railway$/ =~ /switch|railway_crossing/]!:connection {
        throwWarning: tr("disconnected railway junction");
}

Please test the functionality, I know it's not up to standards but easier to debug. Also it's throwing error intentionally for testing.

Edit: first false positive issue; tag a building with railway=workshop and break the railway at the building entrance.

[gaben]

resources/data/validator/geometry.mapcss

@@ -424 +424 @@
 way[route=ferry]!:closed >[index=-1] node[amenity!=ferry_terminal][man_made!=pier]!.node_in_terminal_pier!.node_in_ferry_bridge_tunnel:in-downloaded-area { 
   throwWarning: tr("Ferry route is not connected to a ferry terminal or branches.");
 }
+
+/* #21801 */
+way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=1] node[/railway$/ !~ /switch|railway_crossing/]:connection,
+way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=-1] node[/railway$/ !~ /switch|railway_crossing/]:connection {
+  set .side_connection;
+}
+way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=1] node[/railway$/ !~ /switch|railway_crossing/][count(parent_osm_primitives("railway")) > 2]:connection,
+way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index=-1] node[/railway$/ !~ /switch|railway_crossing/][count(parent_osm_primitives("railway")) > 2]:connection {
+  set .three_way_connection;
+}
+way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] >[index!=1][index!=-1] node.side_connection,
+way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] > node.three_way_connection:connection {
+  group: tr("missing tag");
+  throwError: tr("Railways connection node without {0} or {1}", "railway=switch", "railway=railway_crossing");
+}
+node[/railway$/ =~ /switch|railway_crossing/]!:connection:in-downloaded-area {
+  throwWarning: tr("disconnected railway junction");
+}
+ No newline at end of file

Fine tuned the rule, should be okay now. @skyper, @taylor.smock could you please test it? I'll leave the warning level up to discussion.

[Famlam]

I'm not Skyper nor Taylor, and I like the idea, but nevertheless, some feedback:

1. the following code is nowadays unnecessary: it is already detected since r18914

   node[/railway$/ =~ /switch|railway_crossing/]!:connection:in-downloaded-area {
     throwWarning: tr("disconnected railway junction");
   }
   

2. this code will give a bad warning with railway=abandoned railways (e.g. railways where the rails have been removed, but other infrastructure still exists, such as overhead lines or the railway stones). If the rail has been removed, there's very likely no switch anymore either. Example attached (left side of the example data). It would be weird to have to tag removed:railway=switch or so just because there's some overhead lines left, preferably one wouldn't have to tag removed items.

3. I'm not sure why it tests for /railway$/ rather than (only non-prefixed) railway keys for the ways. Most prefixed railways (proposed:railway, construction:railway etc.) have a railway=proposed or similar anyway; and also, it's also likely a not-yet-existing/former structure if there's a life cycle prefix, meaning there's not necessarily a switch (yet/anymore).

4. Probably the regexes for values are more efficient if they have ^ and $, because the matcher can stop matching if the first character isn't found at the first position, rather than having to test all characters. Example: /turntable|traverser|roundhouse|workshop/ -> /^(turntable|traverser|roundhouse|workshop)$/ (optionally with ?:). I'm not 100% sure though whether my statement is correct for Java

5. way[/railway$/][/railway$/ !~ /turntable|traverser|roundhouse|workshop/] > node.three_way_connection:connection looks like unnecessary double checking of the tags on the way, as the three_way_connection class is already on the node and there's no new filtering (unless I'm missing something)

6. It doesn't find two railways that cross each other without the crossing node being an end node of either railway road

Just a quick thought, but can't it be simplified to:

way[railway][railway!~/^(turntable|traverser|roundhouse|workshop|platform)$/] > node[/railway$/!~/^(switch|railway_crossing)$/][count(parent_osm_primitives("railway")) > 2]:connection,
way[railway][railway!~/^(turntable|traverser|roundhouse|workshop|platform)$/] >[index!=1][index!=-1] node[/railway$/!~/^(switch|railway_crossing)$/][count(parent_osm_primitives("railway")) == 2]:connection {
  set missing_switch_railwaycrossing;
}
node.missing_switch_railwaycrossing {
  throwError: tr("Railways connection node without {0} or {1}", "railway=switch", "railway=railway_crossing");
  group: tr("missing tag");
}

(This doesn't fix the item at 2 yet, which requires filtering on the counted parent tags).

[Famlam]

Example data with railway=abandoned, switch has been removed already

[gaben]

Wow, that's quite a review, thank you!

1. There is a slight difference.

:connection - true for nodes that are used by more than one way
:unconnected - true for nodes that are not used by any way

The rule currently in core not detecting e.g. switches on end nodes without further connection.

2. Somewhat makes sense and I would say it's somewhat "intentional". If I get it right, the rails are mapped but not there, switches neither, but the switch not worth the razed/abandoned prefix. In that case, should the old track connected to the new infrastructure?
3. Same as point 2., if a proposed:railway can exist, switches should live that way as well, at least in my opinion.
4. Good point.
5. It's required with my version, see the attached version attachment:examples_21801_v2.osm​
6. Good point.

Anyway, your version works much better and yet simpler, I like it :)

[Famlam]

Thanks!

Regarding 1: I see your point. Probably it should have !:unconnected then to avoid giving the user a duplicate warning (or remove the existing rule in favor of this one)

Regarding 2 and 3: I tend to say the abandoned track should be connected in at least some cases. For example, if the overhead lines are still fully intact, then there's no reason to introduce an artificial gap between the active and "rail-removed" line. In addition, there's the risk that users add the suggested railway=switch rather than removed:railway=switch for an abandoned railway without switch infrastructure (but with intact other structures that warrant railway=abandoned)

[taylor.smock]

For Famlams points in comment:13:

1. If we had a better way to do tests, it wouldn't be a bad idea to have them so that it is clear what the difference is. Maybe a comment?

4. The only way to be "certain" would be to profile it. With that said, I would expect ^(word1|word2|word3)$ to be more efficient than (word1|word2|word3) since I would anticipate that ^(abc|bcd|cde)$ would check to see if the first character is a, b, or c, and then return. It is also possible that we could parse those regexes out and do a if list.contains(value) if it turns out to be a significant performance hit. I think we already do that for ^start, end$, and ^full$ matches, but I'd have to check.

Unfortunately, I've been rather busy with other things, so (like always), if no responds after a week or two, poke me/the ticket again.

[gaben]

Replying to Famlam:

Regarding 1: I see your point. Probably it should have !:unconnected then to avoid giving the user a duplicate warning (or remove the existing rule in favor of this one)

It would raise error for the object which is part of a way. Probably the best option is to replace the current core check with this "extended" one.

[Famlam]

@comment:16 point 1 and @comment:17

Sorry, apparently I was confusing in comment 15 point 1. After reading comment 14 I realized my mistake (which I made in comment 13, point 1), and in comment 15 (point 1) I meant I would fully support to replace the current rule with a rule with a wider coverage, e.g.:

• Removing from geometry.mapcss

  node:unconnected:in-downloaded-area[railway=railway_crossing],
  node:unconnected:in-downloaded-area[railway=switch],
  

• Subsequently adding either the slightly modified* node[/railway$/=~/^(switch|railway_crossing)$/]!:connection:in-downloaded-area rule proposed by Gaben (thus including life cycle prefixed switches/crossings), or maybe something with a more re-usable string, for which I'd propose something like:

  node[railway=railway_crossing]!:connection:in-downloaded-area,
  node[railway=switch]!:connection:in-downloaded-area {
    throwWarning: tr("{0}", "{0.tag}");
    group: tr("Node should be connected to two or more ways");
  }
  

*to exclude e.g. note:railway=Something something switch something something

[taylor.smock]

Ticket retargeted after milestone closed