Disallow travel through barrier nodes

[abyrd]

This fixes a problem where underground platforms at Delft station were not connected to the station stairs in the input OSM data, but were connected to an emergency exit at the far end of the platform. This PR checks a few tags that make a location impassable when they are present on a node. It's simplistic but a good start.

Tested locally on Delft OSM data. This seems to have no impact on network build time. R5 reports finding and removing 95 additional disconnected components after this change (out of 2596 total in this tiny area). Isochrones look much better now.

[trevorgerhardt]

Do we know how this will effect the average OSM street network? While it seems like a positive improvement, I'm not sure what the extended effects will be. Being able to check how many more "additional disconnected components" are removed across every network used on Conveyal recently would be interesting to see.

Additionally, it'd be great to see additional unit tests that are fixed or related to testing this issue (also for #838). A unit test would probably be easier to add than a creating pipeline for analyzing recently used OSM networks. Can a unit test be added within the scope of this PR?

[abyrd]

The immediate goal here was to get a patch in place from which we could build a new worker version in case it is needed on any projects in Delft. But coming back to this, I think the approach is only good as a stopgap so I've converted it to a draft PR. Skipping creating whole edges is the easiest thing to do but it has too many potential bad side effects.

Consider two edges between three nodes ---A---B---C--- where B is a barrier node and dashes are edges. The edges AB and BC might be quite long, and grid cells along them would end up being reached by straight lines from A and C nodes instead of going along AB or BC and perpendicularly over to the cell. Or worse, cells might be linked to some parallel road farther away. It's clearly better to keep the edges and just prevent B from being shared by the two ways.

Alternative approaches that I considered include: a) creating a separate r5 vertex for each time a barrier node is referenced by a way (splitting the node into N different vertices that are not connected to each other) or b) storing the barrier characteristic on the vertex in the VertexStore and refusing traversal on the fly. The latter involves checking this characteristic on every single edge traversal, though it won't take up any more space since we have plenty of unused bit flags. Option a seems like a good way to go.

The "disconnected components" here are what we otherwise refer to as "islands". So it's kind of surprising that by just removing edges that are blocked at one end, 95 new small pieces of the network become inaccessible. So 95 places were only connected to the rest of the world via nodes (such as this emergency exit) that are barriers in reality. Everything points to these being genuinely disconnected islands, which means that without this patch we are routing into places that are inaccessible in the real world. The net effect of their removal may be minor though, as once such disconnected roads are removed we still allow travel through 'free space' to reach any cells inside. But it would be interesting to at least run one accessibility analysis to see the difference, and map those removed components to see what they are.

I agree it makes sense to include some unit tests with the final implementation of this PR. Not just checking for collateral damage, but ensure that genuinely disconnected circuitous paths like the one in Delft are eliminated and transit stops are not linked to them. It should be straightforward to create a synthetic little OSM network to test this.

[ansoncfit]

My commits here implement approach a) in @abyrd's previous comment: instead of skipping the creation of segments, impassable OSM nodes are not registered as vertices, which leads to duplicate vertices when multiple ways reference an impassable node.

Worth discussing:

1. This approach assumes impassable nodes connect different ways, but does not yet handle impassable nodes along a single way.
2. The vertexIndexForOsmNode map is also referenced in turn restriction code. I did not do a thorough check, but my initial thinking is the approach here is still sound -- disconnecting the ways at an impassible node should supersede any turn restrictions.

[abyrd]

Thanks @ansoncfit. I was looking at this branch again yesterday in anticipation of reviewing your commits, and noted the following:

• Edges currently are only created between nodes that are intersections, so if we want to honor the barrier status of a node that is present on only one way (like an emergency exit) we need to include those in the set of intersection nodes, or maintain two sets and check (barrier || intersection). We may not want to redefine the set of intersections to include barriers, to retain generality and avoid tying it too tightly to this one use case.
• We currently have a one-to-one mapping between R5 vertices and OSM nodes (though not all OSM nodes have a corresponding R5 vertex). Although this fact (and the Map that materializes it) is apparently only used for turn restrictions at the moment, we may not want to break this simple mapping that could be useful in the future - for itinerary display, debugging, or just reasoning about algorithms. On the other hand, for most of these use cases, having a one-to-many mapping from OSM nodes to R5 vertices may be harmless as long as each R5 node maps to exactly one OSM node. But again, we may want to retain simplicity and generality rather than bend it to this one special case.
• Unlike intersection detection which looks only at the numeric node references in the ways, detecting barriers involves inspecting the tags of every node, which would take some additional time.
• We already access each node in the method that creates an edge pair, just to ensure they're non-null, so could inspect the tags at that point. Unfortunately we need to know whether each node is (barrier || intersection) to know whether it's an edge endpoint before we call this edge creation method.
• We might not want to fetch every node twice: slowdown will be negligible on small networks but could possibly be significant on whole countries.
• Fetching the nodes to check tags could be hoisted out of the edge creation method. This implies bailing out of entire ways (instead of individual edges) when a way references a missing node, which seems fine because this is just a soft failure mode for bad input data.
• If we don't want to break the one-to-one node-vertex mapping, checking the barrier status of nodes at each traversal seems cleaner. It still requires pre-detecting barriers and treating them like intersections, but does not create the potentially confusing 2+ nodes at the same exact location with the same corresponding OSM ID. Slowdown could be negligible since we're talking about checking one bit, but memory accesses sometimes add up.
• Building egress tables involves a lot of street searches and runs in parallel to use all cores, so timing a large egress table build operation is probably a good way to establish whether a change has a perceptible effect on street routing performance.
• To avoid adding any new mechanisms, we could also code barriers as turn restrictions. However, turn restrictions are thought to be a performance bottleneck, and are currently only processed in car and bike routing. It probably doesn't make sense to enable them for walk routing just to model barriers.

[abyrd]

Now reviewing the PR - thanks for including the test @ansoncfit. In addition to validating the fix, looking at the test fixture OSM data is helping think through the problem. The test demonstrates that creating separate vertices each time an OSM barrier node is referenced does handle this particular case in Delft, but also demonstrates why it would not handle similar problems in other places.

The OSM contains several barrier=gate nodes that are not detected by our isBarrierNode method, and (as you alluded to) are not shared by multiple ways.

First, it seems like a method with the name isBarrierNode should really detect barrier= nodes. I think I didn't check for them in the original PR because a gate is not necessarily locked or private, so I checked for the additional access=no|private tags. But the Delft data show OSM contains impassable barrier=gate nodes with no access tags. So this method should probably check for an node with access=no|private, as well as barrier=gate nodes without any access=*.

Second, as you mentioned, these barrier=gate nodes exist along a single way rather than at intersections with other ways. It's just a coincidence that in Delft the emergency exit node was reused by the ways representing platforms 3 and 4 themselves, so we can't count on this elsewhere.

I think to consider this fix complete, we'll need to pre-detect the barrier nodes and fragment the ways into edges at those nodes as well as intersections. For the reasons described in my previous comment, I would like to check whether this causes any perceptible slowdown in network building and egress table construction. As long as I'm checking speed, I will also check the alternative approach using a vertex flag to block traversal during routing.

Simply not putting the barrier nodes into the vertexIndexForOsmNode map is a nice trick to keep the code simple. However my gut feeling is that it's better for the data structures to reflect simple consistent relationships. If the map means "osm node X is represented by vertex Y", it follows that "absence of key X means there's no corresponding vertex", and this could become harder to reason about outside the present special case if we add the contradictory meaning "absence of key X means there are multiple corresponding vertices". Of course it may be sufficient to add a clear comment as you've done.

[abyrd]

Maybe what we want for isBarrierNode is:

String access = node.getTag("access");
if (access == null) {
    return node.hasTag("entrance", "emergency") || node.hasTag("barrier");
} else {
    return access.equals("no") || access.equals("private");
}

[ansoncfit]

1. We want many barriers to be traversable though (especially turnstile, among others at https://wiki.openstreetmap.org/wiki/Key:barrier#Values)

2. we'll need to pre-detect the barrier nodes and fragment the ways into edges at those nodes as well as intersections.

Agreed, given that OSM guidance is not to locate barriers at intersections (https://wiki.openstreetmap.org/wiki/Key:barrier#How_to_map_barrier_nodes)

3. Simply not putting the barrier nodes into the vertexIndexForOsmNode map is a nice trick to keep the code simple. However my gut feeling is that it's better for the data structures to reflect simple consistent relationships.

That's also my sense -- a more substantial change would probably be worth it to keep things easier to reason about.

[abyrd]

I implemented vertex-flag-based barrier nodes, supporting barrier nodes at non-intersection locations. I discovered a few snags and existing problems including:

• Inability to travel around the edge of an area via the barrier node.
• Island pruning is not consistent with our normal street routing.

Again the Delft test data helps identify and understand the problems. In the Delft data there are only two barrier nodes on ways: the emergency exit stairs and path out of platform 3/4 start at the emergency exit node 3377954642 and contain gate node 5591069148 at a non-intersection point. The rest of the barrier nodes are free-floating gate nodes that are not on any way. It looks like someone was trying to situate the gate nodes on the platforms or paths but failed to do so topologically and left them free-floating. We can imagine what would happen if they were on the platform ways like the emergency exit is. This raises the question of what exactly it means when a barrier node is along a way. Presumably if it's a linear way, one cannot pass through that point at all. However, if it's an areal way like a platform, presumably the barrier does not block passage through that part of the area, but only passage to other ways via that point on the area's edge. To fit a graph-based routing model, R5 is treating platforms as linear features instead of true areas (we usually just route along the edge of the platform). Bocking traversal of the emergency exit node (or other nodes like gates) then erroneously prevents passing through that point on the platform (when remaining on the platform) as well as correctly preventing passage to other ways such as the emergency exit stairs.

Secondly, the island pruning code contains its own separate search implementation (distinct from edge.traverse) and does not use the edge-based routing needed for handling turn restrictions and barrier nodes. So even when regular routing is modified to not pass through barrier nodes, island pruning still routes through them. Duplicating the same checks in the island pruner (besides being ugly style-wise) still does not work because the island pruning algorithm remains vertex-based, while all other routing is essentially edge-based (states are situated at the end of edges before entering the vertex, not at vertices - see Javadoc on StreetRouter#bestStatesAtEdge).

For these reasons, even though I still see the node-based barrier approach as being more consistent with source OSM data, it becomes quite messy with our current code. I think we should keep @ansoncfit's duplicate node approach.

I would like to keep some changes from my branch though:

1. A more complete definition of isBarrierNode()
2. Identifying barrier nodes along ways and treating them like intersections
3. A new edge-based island removal that reuses our standard search algorithm, handing turn restrictions or other restrictions (addressing Turn restrictions create islands #806).

The third item is the most disruptive and should probably not go into the current release. I'll finish checking the performance of the above. I have pushed my draft changes to another branch no-emergency-exits-abyrd for reuse/comparison.

[abyrd]

1. We want many barriers to be traversable though (especially turnstile, among others at https://wiki.openstreetmap.org/wiki/Key:barrier#Values)

Thanks for pointing this out. I now realize that I already looked at this list when originally writing the method, and almost all the barrier= values are things you can pass through, including gate if it's not marked access=private. So the original absence of barrier= checks was intentional. I'll update the method Javadoc with this link and some explanation.

[abyrd]

I merged changes from @ansoncfit with some of my own, keeping @ansoncfit's approach of splitting OSM nodes into multiple vertices, but also flagging those vertices as IMPASSABLE for debugging/visualization/future routing purposes and ensuring the source OSM nodes appear as keys in the vertexIndexForOsmNode map. I also added some explanation in the Javadoc. The test continues to pass.

I built some medium-sized networks with and without these changes, and don't see any appreciable change in the time taken to create network edges from OSM data.

I did not yet update the island pruning code to be edge-based - we can do that when solving #806, perhaps using an approach like the one drafted in branch no-emergency-exits-abyrd.

[ansoncfit]

Looks good, thanks for the follow-up changes, especially to add intersections at barrier nodes along single ways.

[ansoncfit]

We should consider tags beyond access=no. OSM examples include setting that as a blanket policy, then overriding for specific modes. For example, this cycle barrier in Leiden and similar ones lead to undesired breaks in the network, reducing access:

Other examples showing the expected behavior with simpler tags:
✔ Around Delft Station, with the emergency exit that originally motivated this PR

✔ access=no gate along a levee

✔ access=private gate along a farm track. In this example, the farm track just north of the highway is pruned, so areas link directly to Rijndijk. That leads to longer travel times in the red area, and shorter travel times in the blue area.