Prevent bicycles from using stairs #4614

leonardehrenfried · 2022-11-18T14:19:37Z

Summary

This hard-bans bicycles from using stairs even as a last resort.

Whether this is too strict or there are nuances we need to discuss in the developer meeting.

Issue

Fixes #4613

Unit tests

Added.

cc @tsobuskerudbyen

codecov-commenter · 2022-11-18T14:25:37Z

Codecov Report

Base: 60.63% // Head: 60.65% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (542b4a5) compared to base (4a0796c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4614      +/-   ##
=============================================
+ Coverage      60.63%   60.65%   +0.02%     
- Complexity     12229    12251      +22     
=============================================
  Files           1580     1580              
  Lines          63461    63491      +30     
  Branches        6994     6997       +3     
=============================================
+ Hits           38481    38513      +32     
  Misses         22801    22801              
+ Partials        2179     2177       -2

Impacted Files	Coverage Δ
...outing/api/request/preference/BikePreferences.java	`88.88% <100.00%> (+0.69%)`	⬆️
...dalone/config/routerequest/RouteRequestConfig.java	`97.94% <100.00%> (+0.02%)`	⬆️
.../opentripplanner/street/model/edge/StreetEdge.java	`87.25% <100.00%> (+0.05%)`	⬆️
...va/org/opentripplanner/api/mapping/ModeMapper.java	`11.11% <0.00%> (-0.66%)`	⬇️
...g/opentripplanner/api/mapping/RouteTypeMapper.java	`0.00% <0.00%> (ø)`
.../opentripplanner/api/parameter/ApiRequestMode.java	`100.00% <0.00%> (ø)`
...legacygraphqlapi/generated/LegacyGraphQLTypes.java	`0.00% <0.00%> (ø)`
.../standalone/config/framework/json/NodeAdapter.java	`95.23% <0.00%> (+1.19%)`	⬆️
...entripplanner/transit/model/basic/TransitMode.java	`95.65% <0.00%> (+1.20%)`	⬆️
...g/opentripplanner/api/parameter/QualifiedMode.java	`68.42% <0.00%> (+2.63%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

leonardehrenfried · 2022-11-22T10:02:02Z

At today's developer meeting we talked about it: we want to not totally ban bicycles on stairs but we want to give a very high default penalty so it's only used as a last resort. This cost is configurable so you can also exclude bikes on stairs.

optionsome · 2022-11-22T11:24:52Z

I discussed this with some people at HSL and they thought it might make sense to perhaps modify the cost based on https://wiki.openstreetmap.org/wiki/Key:ramp but would we then need two new parameters (bicycle walking on stairs with and without ramps)? I don't want to force you into doing this now but it's something we could consider and perhaps I (or someone else from Digitransit) could implement it at some point.

Also, I wonder if the stairs reluctance is actually a client specific thing or something we should configure in the "safety" values as it would be more flexible in taking into account different tagging? However, the stairs also affect the traversal time so it's a bit more complicated.

leonardehrenfried · 2022-11-24T11:18:44Z

I discussed this with some people at HSL and they thought it might make sense to perhaps modify the cost based on https://wiki.openstreetmap.org/wiki/Key:ramp but would we then need two new parameters (bicycle walking on stairs with and without ramps)? I don't want to force you into doing this now but it's something we could consider and perhaps I (or someone else from Digitransit) could implement it at some point.

Also, I wonder if the stairs reluctance is actually a client specific thing or something we should configure in the "safety" values as it would be more flexible in taking into account different tagging? However, the stairs also affect the traversal time so it's a bit more complicated.

At the moment it's not possible to assign a bike safety value to a set of stairs as bikes are not allowed on it:

OpenTripPlanner/src/main/java/org/opentripplanner/graph_builder/module/osm/tagmapping/DefaultMapper.java

Line 62 in ad9f951

props.setProperties("highway=steps", pedestrianWayProperties);

That doesn't mean that we cannot change it. Let's discuss it in the dev meeting.

leonardehrenfried · 2022-12-01T15:45:58Z

At today's meeting we discussed this. @hannesj and I were of the opinion that it's best to have a separate reluctance parameter, which I already added last week.

@optionsome, if you disagree, lets discuss this in the issue.

src/main/java/org/opentripplanner/routing/api/request/preference/BikePreferences.java

hannesj · 2022-12-01T16:52:39Z

src/main/java/org/opentripplanner/street/model/edge/StreetEdge.java

+    if (mode.isCycling() && isStairs()) {
+      return false;
+    }


Can we set this already in the WayPropetySet, then we wouldn't need to check it during runtime?

Yes, this can go away. The tag mapper already does that (

OpenTripPlanner/src/main/java/org/opentripplanner/graph_builder/module/osm/tagmapping/DefaultMapper.java

Line 62 in ad9f951

props.setProperties("highway=steps", pedestrianWayProperties);

) but my test doesn't reflect this.

t2gran · 2022-12-04T18:41:42Z

src/main/java/org/opentripplanner/routing/api/request/preference/BikePreferences.java

+    // very high reluctance to carry the bike up/down a flight of stairs
+    this.stairsReluctance = 100;


Would you bike for 1h40m to avoid walking the stairs for 1 minute? I think 10 probably are better. We should try to use sensible values - if not, we undermine what "reluctance" mean.

I was thinking more like: cycling 100 meters to avoid a one meter stairs. I would definitely take the long way.

Am I thinking about it the wrong way?

We usually use time as a unit, if not it you must adjust when the speed changes. Generaflized-cost is equivalent of spending 1 second on a transit vehicle.

I think a reason for this is that by default the time and cost of switching between walking and biking is 0, we should have some more reasonable defaults. That way the multiplier here can be more realistic

I tested it with the original case in Kongsberg and even with a comparatively low reluctance of 10 the stairs are avoided. That is what we want.

Before

After

Actually, I realised that I already do that.

Or are you talking about the bike switch cost/time?

I think is correct, multiplying the weigh is a reluctance. Strictly is a scalar, but the weight in transit seconds. See StreetEdge

Should we have a separate PR, where we change the default switch time & cost?

Yes, that's a good idea.

t2gran · 2022-12-05T12:54:45Z

Should I be using Units.reluctance to make it easier to read?

We have a way to go here, but yes I think that would be nice. Some day we might use over own types instead of ints and doubles, then we can enforce "unit-safty" as well. You may leave it for now.

leonardehrenfried · 2022-12-05T12:55:39Z

I realised that I already did use Units.reluctance.

leonardehrenfried added the Bug label Nov 18, 2022

leonardehrenfried requested a review from a team as a code owner November 18, 2022 14:19

leonardehrenfried force-pushed the bike-stairs branch 3 times, most recently from 4cbafac to dd56873 Compare November 24, 2022 11:10

leonardehrenfried marked this pull request as draft November 29, 2022 09:04

t2gran added this to the 2.3 milestone Dec 1, 2022

hannesj requested review from hannesj and optionsome December 1, 2022 14:47

leonardehrenfried marked this pull request as ready for review December 1, 2022 14:48

leonardehrenfried added 5 commits December 1, 2022 15:48

Prevent bicycles from using stairs

8137aa5

Replace a hard ban of bikes on stairs with a reluctance

d780faa

Update docs

025df66

Remove unneeded code

d94a030

Forbid traversal on bike

01c929f

leonardehrenfried force-pushed the bike-stairs branch from dd56873 to 01c929f Compare December 1, 2022 14:48

Adjust to upstream code

0b18487

hannesj reviewed Dec 1, 2022

View reviewed changes

src/main/java/org/opentripplanner/routing/api/request/preference/BikePreferences.java Outdated Show resolved Hide resolved

hannesj reviewed Dec 1, 2022

View reviewed changes

leonardehrenfried added 2 commits December 2, 2022 10:53

Add reluctance to hashcode

390c600

Make tests more realistic, remove extra check

cd2ae58

leonardehrenfried requested a review from hannesj December 2, 2022 10:56

hannesj previously approved these changes Dec 2, 2022

View reviewed changes

t2gran reviewed Dec 4, 2022

View reviewed changes

leonardehrenfried dismissed hannesj’s stale review via 023d832 December 5, 2022 08:29

leonardehrenfried force-pushed the bike-stairs branch from 023d832 to b85ec40 Compare December 5, 2022 08:31

Reduce default bike stairs reluctance

542b4a5

leonardehrenfried force-pushed the bike-stairs branch from b85ec40 to 542b4a5 Compare December 5, 2022 08:38

leonardehrenfried requested review from t2gran and hannesj December 5, 2022 09:04

leonardehrenfried enabled auto-merge December 5, 2022 09:15

leonardehrenfried force-pushed the bike-stairs branch from b0d8c1a to 542b4a5 Compare December 5, 2022 09:30

t2gran approved these changes Dec 5, 2022

View reviewed changes

hannesj approved these changes Dec 5, 2022

View reviewed changes

leonardehrenfried merged commit 13fadaf into opentripplanner:dev-2.x Dec 5, 2022

t2gran pushed a commit that referenced this pull request Dec 5, 2022

Add changelog entry for #4614 [ci skip]

7b4f8f7

leonardehrenfried deleted the bike-stairs branch December 5, 2022 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent bicycles from using stairs #4614

Prevent bicycles from using stairs #4614

leonardehrenfried commented Nov 18, 2022

codecov-commenter commented Nov 18, 2022 •

edited

Loading

leonardehrenfried commented Nov 22, 2022

optionsome commented Nov 22, 2022

leonardehrenfried commented Nov 24, 2022

leonardehrenfried commented Dec 1, 2022 •

edited

Loading

hannesj Dec 1, 2022

leonardehrenfried Dec 2, 2022

leonardehrenfried Dec 2, 2022

t2gran Dec 4, 2022

leonardehrenfried Dec 4, 2022

t2gran Dec 4, 2022

hannesj Dec 5, 2022

leonardehrenfried Dec 5, 2022

leonardehrenfried Dec 5, 2022

leonardehrenfried Dec 5, 2022

t2gran Dec 5, 2022

hannesj Dec 5, 2022

leonardehrenfried Dec 5, 2022

t2gran commented Dec 5, 2022

leonardehrenfried commented Dec 5, 2022

		// very high reluctance to carry the bike up/down a flight of stairs
		this.stairsReluctance = 100;

Prevent bicycles from using stairs #4614

Prevent bicycles from using stairs #4614

Conversation

leonardehrenfried commented Nov 18, 2022

Summary

Issue

Unit tests

codecov-commenter commented Nov 18, 2022 • edited Loading

Codecov Report

leonardehrenfried commented Nov 22, 2022

optionsome commented Nov 22, 2022

leonardehrenfried commented Nov 24, 2022

leonardehrenfried commented Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran commented Dec 5, 2022

leonardehrenfried commented Dec 5, 2022

codecov-commenter commented Nov 18, 2022 •

edited

Loading

leonardehrenfried commented Dec 1, 2022 •

edited

Loading