Skip to content

Conversation

miklcct
Copy link
Contributor

@miklcct miklcct commented Jul 31, 2025

Summary

Improve support for more kinds of barriers, including point barriers and linear barriers, which can block travel for multiple modes of traversal.

In implementation terms, it does the following:

  • Barrier node detection is improved to handle more kinds of barriers and traversal modes
  • If a way crosses a linear barrier, the vertices are split with an edge added to cross the barrier only for allowable traversal modes. If the node is tagged with barrier=entrance, the restrictions for the barrier is ignored. Note that if two linear barriers join end to end, it is possible to cross between them at the joining node, unless the node itself blocks access. Update 2025-08-06: After checking some real-world mapping, I have decided to disallow passing through the end of a barrier as well, like the photo shown below but with two areas.
  • Areas are not joined if they share the same barrier which blocks traversal for some street modes for the area (sharing at least two nodes of the barrier), so that a linear barrier can block area routing between two areas and force the user to go via the end. Also, areas are only joined if they share at least two consecutive nodes (when two areas join at one node, it is possible for a wall to block access between them).

What this PR does not do

Note that barrier blocking access within a single area (whether the barrier effectively splits the area into two or not), like the following picture is not implemented.
barrier in area

Issue

Closes #6689

The following screenshots show the fixed routing to avoid walking into a wall inside a train station:

original:
original
fixed:
image

Unit tests

Added for area splitting, vertex handling and barrier permission detection including wheelchair accessibility.

Documentation

Changelog

Bumping the serialization version id

Definitely required. A new kind of vertex is added to prevent duplicate labels for split vertices.

Copy link

codecov bot commented Jul 31, 2025

Codecov Report

❌ Patch coverage is 94.53376% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.08%. Comparing base (b5f5faf) to head (7ba8b42).
⚠️ Report is 139 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...r/graph_builder/module/osm/BarrierEdgeBuilder.java 88.63% 1 Missing and 4 partials ⚠️
...nner/graph_builder/module/osm/VertexGenerator.java 93.54% 2 Missing and 2 partials ⚠️
...aph_builder/issues/BarrierIntersectingHighway.java 50.00% 2 Missing ⚠️
..._builder/issues/DifferentLevelsSharingBarrier.java 50.00% 2 Missing ⚠️
...pplanner/graph_builder/module/osm/OsmNodePair.java 81.81% 0 Missing and 2 partials ⚠️
...planner/graph_builder/module/osm/OsmAreaGroup.java 98.21% 0 Missing and 1 partial ⚠️
...ripplanner/graph_builder/module/osm/OsmModule.java 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6774      +/-   ##
=============================================
+ Coverage      71.96%   72.08%   +0.12%     
- Complexity     19423    19517      +94     
=============================================
  Files           2099     2105       +6     
  Lines          78756    78966     +210     
  Branches        7970     8003      +33     
=============================================
+ Hits           56680    56926     +246     
+ Misses         19257    19225      -32     
+ Partials        2819     2815       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@miklcct miklcct marked this pull request as ready for review July 31, 2025 16:59
@miklcct miklcct requested a review from a team as a code owner July 31, 2025 16:59
@optionsome optionsome requested a review from vesameskanen August 5, 2025 09:14
An example in the wiki page https://wiki.openstreetmap.org/wiki/Guidelines_for_pedestrian_navigation shows that when a wall ends on the edge of a pedestrian area, a pedestrian can't go through that node.

This has been verified with real-world mapping in Farringdon station.
Copy link
Contributor Author

@miklcct miklcct left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment on barrier after OSM ground survey

@t2gran t2gran added this to the 2.8 (next release) milestone Aug 11, 2025
Copy link
Contributor

@vesameskanen vesameskanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is somewhat complex and it is a bit hard to understand what kind of effects this PR has, although there is a very nice test set. It may be that areas surrounded by barriers get some sort of redundant edge set, which does not lead anywhere, at every boundary vertex. The screenshots below indicate that there are regressions in area processing. Maybe the code should more carefully process only vertices between two areas.

before after

Location: https://hsl-debug.digitransit.fi/?variables=%257B%2522from%2522%253A%257B%257D%252C%2522to%2522%253A%257B%257D%252C%2522dateTime%2522%253A%25222025-08-12T12%253A06%253A49.257Z%2522%257D#17.27/60.16861/24.959832

// make a separate vertex if the node is on a barrier
if (nodesInBarrierWays.containsKey(node)) {
return getSplitVertexOnBarrier(node, way);
}
// single-level case
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has side effects, which are hard to predict. Vertex is no longer added to intersectionNodes map, which is later used in area processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended. intersectionNodes track nodes which are used in multiple OSM ways, however, if a node is on a barrier, a separate node is created for each way so it is not used in multiple OSM ways.

The reasoning is the same as splitting nodes on areas on multiple levels into different vertices.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit concerned how WalkableAreaBuilder selects OSM way entities in vertex generation. Sometimes way is picked randomly, sometimes parent area is used. I suspect that this occasionally creates duplicated vertices and somewhat strange edge connections.

In the past, intersection vertex map eliminated duplicates in a simple way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some logging and found out that BarrierEdgeBuilder tries to connect distant sections of area groups, not just the two at opposite side of a barrier.

I believe use of this entity reference is the reason:

The code should consistently use parent entity of the currently processed area, not a random pick from the area group.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miklcct I fixed this problem in #6826

@miklcct
Copy link
Contributor Author

miklcct commented Aug 12, 2025

I have found the regression. The intention of isNodeBelongsToWay is to check if a node belongs to a routable way, but now it also includes nodes on barrier way as a result.

this fixes a regression where area edges are generated to nowhere
@miklcct miklcct requested a review from vesameskanen August 22, 2025 13:28
Copy link
Contributor

@vesameskanen vesameskanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graph build works now much better. Barriers no longer remove important bridges in Helsinki.

// make a separate vertex if the node is on a barrier
if (nodesInBarrierWays.containsKey(node)) {
return getSplitVertexOnBarrier(node, way);
}
// single-level case
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit concerned how WalkableAreaBuilder selects OSM way entities in vertex generation. Sometimes way is picked randomly, sometimes parent area is used. I suspect that this occasionally creates duplicated vertices and somewhat strange edge connections.

In the past, intersection vertex map eliminated duplicates in a simple way.

@vesameskanen
Copy link
Contributor

vesameskanen commented Aug 27, 2025

One more discovery: you should consider no through traversal limitations when adding connections through a barrier, the same way as here:

https://github.com/HSLdevcom/OpenTripPlanner/blob/fix-area-edge-props/application/src/main/java/org/opentripplanner/routing/linking/VertexLinker.java#L744

If areas (or a way connecting to an area) have private access and the connecting edges are not marked as no through, traversal is not possible - the edges are added for no reason.

This is to ensure that local traffic can get across the barrier between two no through traffic areas.
Copy link
Contributor

@vesameskanen vesameskanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite happy with the functionality now, especially when #6826 is included as well.

We are still considering if the two new issues are useful in their current form, or should the logic examine the data more carefully. For example, highways crossing a barrier tagged as barrier=retaining wall at a bridge structure seem to be very common. An example: https://www.openstreetmap.org/node/444209183

var from = vs[i];
var to = vs[j];

if (inDegrees[i] > 0 && outDegrees[j] > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the fix presented in #6826, degree check seems unnecessary. I tested that this condition was always true when building helsinki area street graph.

Copy link
Contributor Author

@miklcct miklcct Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check is still needed to prevent unnecessary edges being built when a one-way street joins an area at a gate through a wall. The end vertex of the one-way street will only have incoming edge but not outgoing edge, so this block will skip it when attempting to join from the vertex on the area to the vertex at the end of a one-way street which doesn't have outgoing edges.

@miklcct
Copy link
Contributor Author

miklcct commented Sep 1, 2025

I just found out that I made a mistake in a previous PR, which was surprising untested. The method had a test case but it didn't contain this specific case.

https://github.com/opentripplanner/OpenTripPlanner/pull/6764/files#diff-91193fccae4c9a19c4e80aafdc3c8ec8ac4de356c86e42331c6728f85459b2a6R786

I forgot to add a return false; statement after checking all access was denied.

I have just corrected it and added a test for it.

Copy link
Contributor

@vesameskanen vesameskanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple of minor request. We are close getting this merged.

@optionsome
Copy link
Member

@miklcct you can merge this if you think this is ready

@optionsome optionsome added bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Improvement A functional improvement labels Sep 9, 2025
@miklcct miklcct merged commit 9427bdc into opentripplanner:dev-2.x Sep 9, 2025
7 checks passed
t2gran pushed a commit that referenced this pull request Sep 9, 2025
t2gran pushed a commit that referenced this pull request Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Improvement A functional improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTripPlanner attempts to route me through a wall
5 participants