-
Notifications
You must be signed in to change notification settings - Fork 2
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
Describe bike infra in instructions #169
Conversation
Very basic version of showing bike infra in instructions. If between 25 and 50 percent of the step has an infra type, it's prefixed with "partial"; between 50 and 75, "mostly"; if over 75 percent we just say it's that infra type with no qualifier. Example: Turn right onto Cesar Chavez Street bike lane Turn left onto Guerrero Street mostly bike lane, partial protected bike lane Keep left on Arlington Street shared road In some cases these instructions are weird and would be more helpful if GraphHopper had split the instructions into segments differently. Filed an issue on GraphHopper repo: bikehopper/graphhopper#94 In the example described in that issue, BikeHopper will misleadingly tell you "Turn left onto Collingwood St / bike lane", even though there is no bike lane on Collingwood St, because the router is having you turn onto Market St (which has a bike lane) and counting that as the same instruction for some reason. Lots to iterate on and improve here, but I think this is a good start.
src/lib/geometry.js
Outdated
if (cycleway === 'shared_lane') return 'shared road'; | ||
if (cycleway === 'sidepath') return 'sidepath'; | ||
if (cycleway === 'shoulder') return 'shoulder'; | ||
if (roadClass === 'primary' || roadClass === 'secondary') return 'main road'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (roadClass === 'primary' || roadClass === 'secondary') return 'main road'; | |
if (['primary', 'primary_link', 'secondary', 'secondary_link', 'tertiary', 'tertiary_link'].contains(roadClass)) return 'main road'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this. Maybe there's a way to make this a set instead of a list, but you get the idea. There's a lot of roads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And unfortunately there's also motorway, motorway_link, trunk, trunk_link. I really hope this would never come up but might as well have a case for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also cyclestreet
, which though rare is seen in the bay area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add motorway, trunk, and the link variants. good call
I don't think of tertiary as a main road. Per openstreetmap wiki, "Use only for roads with low to moderate traffic."
Do you have examples of cyclestreets in the Bay Area? How would you describe it? I would rather err on the side of saying nothing when the app encounters a street type we aren't familiar enough with to confidently usefully describe.
@@ -142,3 +146,87 @@ export function curveBetween(start, end, options, angle = 30) { | |||
options, | |||
); | |||
} | |||
|
|||
function _describeBikeInfraFromCyclewayAndRoadClass(cycleway, roadClass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a map/object? It should be marginally faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean. If you mean replace this whole function with an object, that wouldn't allow the ordering below, which is significant: for example we don't check "cycleway" if road class is path, we don't return "main road" if there is a protected bike lane.
In any event, I expect there will be lots of iteration on this logic as we dogfood BikeHopper and look at how useful and accurate these labels feel in practice. This is only a first pass.
src/lib/geometry.js
Outdated
@@ -17,6 +19,8 @@ export const EMPTY_GEOJSON = { | |||
|
|||
export const BIKEABLE_HIGHWAYS = ['cycleway', 'footway', 'pedestrian', 'path']; | |||
|
|||
const METERS_PER_FOOT = 0.3048; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of converting ourselves, let's use https://turfjs.org/docs/#convertLength ?
merging after addressing comments above! |
Very basic version of showing bike infra in instructions. If between 25 and 50 percent of the step has an infra type, it's prefixed with "partial"; between 50 and 75, "mostly"; if over 75 percent we just say it's that infra type with no qualifier.
Example:
Turn right onto Cesar Chavez Street
bike lane
Turn left onto Guerrero Street
mostly bike lane, partial protected bike lane
Keep left on Arlington Street
shared road
In some cases these instructions are weird and would be more helpful if GraphHopper had split the instructions into segments differently. Filed an issue on GraphHopper repo: bikehopper/graphhopper#94
In the example described in that issue, BikeHopper will misleadingly tell you "Turn left onto Collingwood St / bike lane", even though there is no bike lane on Collingwood St, because the router is having you turn from Collingwood onto Market St (which has a bike lane) and counting that as the same instruction for some reason.
Lots to iterate on and improve here, but I think this is a good start.
Closes #170