bikehopper / graphhopper

Open source routing engine for OpenStreetMap. Use it as Java library or standalone web server.
https://www.graphhopper.com/open-source/
Apache License 2.0
3 stars 0 forks source link

GH #68: Add corridor to pushingSectionHighways #145

Open rsarathy opened 3 months ago

rsarathy commented 3 months ago

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.

graue commented 3 months ago

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 commented 3 months ago

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.