Skip to content

Apply correct traversal permissions to barrier vertex - #5369

Merged
vesameskanen merged 10 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:fix-bollard
Sep 26, 2023
Merged

vesameskanen merged 10 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:fix-bollard

Conversation

@vesameskanen

Copy link
Copy Markdown
Contributor

Summary

Two bug fixes in barrier vertex:

  • Bicicycle=no tag prevented car traversal, not bike traversal. Walk restriction had similar errors. Such errors are now fixed.
  • Any kind of access restriction at the end vertex of a street (dead end alley, doorway etc.) caused failures in street routing, because route search could snap to an unreachable location. Routing could get infinitely close to the vertex but not to the final position. This problematic non-continuous behaviour is fixed by pruning the permissions of barrier vertices at the graph build stage where the street edge connections are known.

Issue

Closes #5353

Unit tests

Tests updated to reflect correct barrier permissions. One new unit test which tests barrier pruning added.

Documentation

Javadoc updated

Osm contains nodes tagged as access=no and which represent an end point of a way.
For example, a doorway leading to a locked gate is sometimes modeled like that.
If routing request snaps to such end point, routing fails. Barrier end vertices do not
add any kind of useful functionality but just cause trouble.
@vesameskanen
vesameskanen requested a review from a team as a code owner September 21, 2023 11:35
@vesameskanen vesameskanen added the !Bug Apply to issues describing a bug and PRs witch fixes it. label Sep 21, 2023
@codecov

codecov Bot commented Sep 21, 2023 •

Copy link
Copy Markdown

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (f664f77) 66.50% compared to head (5e424de) 66.53%.
Report is 31 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5369      +/-   ##
=============================================
+ Coverage      66.50%   66.53%   +0.02%     
- Complexity     15235    15255      +20     
=============================================
  Files           1785     1787       +2     
  Lines          69271    69303      +32     
  Branches        7291     7301      +10     
=============================================
+ Hits           46069    46110      +41     
+ Misses         20733    20719      -14     
- Partials        2469     2474       +5     
Files Coverage Δ
...ripplanner/graph_builder/module/osm/OsmModule.java 93.86% <100.00%> (+0.09%) ⬆️
...g/opentripplanner/openstreetmap/model/OSMNode.java 95.23% <100.00%> (+1.90%) ⬆️
...entripplanner/openstreetmap/model/OSMWithTags.java 88.73% <100.00%> (ø)
...tripplanner/street/model/vertex/BarrierVertex.java 78.26% <68.75%> (-21.74%) ⬇️

... and 16 files with indirect coverage changes

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

Comment thread src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java Outdated

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One small thing, otherwise this looks good.

Comment thread src/main/java/org/opentripplanner/openstreetmap/model/OSMNode.java Outdated

/**
* Checks if this node is bollard
static final Set<String> motorVehicleBarriers = Set.of("bollard", "bar", "chain");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think static variables belong to the top of the class and should be named like MOTOR_VEHICLE_BARRIERS

public boolean isBollard() {
return isTag("barrier", "bollard");
public boolean isMotorVehicleBarrier() {
var barrier = this.getTag("barrier");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's too late now because I don't want to hold up the merge of this PR but whenever we touch this code again, there is the following new helper method for checking if it's one of a set of tag values:

/**
* Takes a tag key and checks if the value is any of those in {@code oneOfTags}.
*/
public boolean isOneOfTags(String key, Set<String> oneOfTags) {
return oneOfTags.stream().anyMatch(value -> isTag(key, value));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I forgot that new method. Let's fix this in some upcoming OSM related PR.

@vesameskanen
vesameskanen merged commit ba78519 into opentripplanner:dev-2.x Sep 26, 2023
t2gran pushed a commit that referenced this pull request Sep 26, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Oct 10, 2023
@vesameskanen
vesameskanen deleted the fix-bollard branch April 11, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bicycle=no tag prevents car routing

4 participants