Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH #68: Add corridor to pushingSectionHighways #145

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rsarathy
Copy link

Fixes #68.

Issue and Root Cause

When given an OSM way with highway=corridor, BikeCommonFlagEncoder was applying EncodingManager.Access.CAN_SKIP in getAccess(). This was because corridor wasn't being included in the pushingSectionHighways list.

Since the way had CAN_SKIP set, BikeCommonFlagEncoder.avgSpeedEnc was never initialized (even though BikeCommonFlagEncoder.getSpeed() would return a positive value). Therefore, when the edge was collected as part of a route, it was unblocked but had a speed of zero.

What we tried initially was adding the line

addPushingSection("corridor");

to the BikeFlagEncoder constructor.

But BikeFlagEncoder is a subclass of BikeCommonFlagEncoder, and the latter is the class that calculates average speeds for edges. So functionally, the addPushingSection() calls in the BikeFlagEncoder do nothing as the pushingSectionHighways list is only used in BikeCommonFlagEncoder, where it is always empty.

Fix

Moving addPushingSection() to the BikeCommonFlagEncoder will ensure that all pushing sections receive a positive speed, and won't throw the "speed ≠ 0 unblocked edge" exception when collected as part of a desired route.

@rsarathy rsarathy requested a review from abhumbla May 21, 2024 21:34
@graue
Copy link

graue commented May 21, 2024

This seems incorrect.

  1. addPushingSection("corridor") in BikeFlagEncoder does work around the problem, according to my testing locally. But we didn't deploy that change, because...
  2. This is not the root cause of the issue. CAN_SKIP is supposed to mean bike routing will not use highway=corridor ways. Yet interpolated transfer legs are using them anyway. That is the problem, and we still don't know why it's occurring.

I maintain that not routing bicycles down corridors is correct behavior, and should be kept. We should find the actual root cause of the bug instead of merging this.

@graue
Copy link

graue commented May 21, 2024

Essentially, for interpolated transfer legs, it seems GraphHopper is using foot routing instead of bike routing, or is using some Frankenstein mix of foot routing and bike routing. This corridor thing is therefore only the most obvious manifestation of what's probably a whole class of bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed cannot be 0 for unblocked edge
3 participants