Add escalator reluctance parameter #5046

nurmAV · 2023-04-13T13:09:38Z

Summary

This PR adds a new parameter escalatorReluctance so that escalators can be treated differently than regular stairs in routing.
Also disables bicycle walking in escalators.

Unit tests

Write a few words on how the new code is tested.

The changes were tested manually

Documentation

The parameter escalatorReluctance was added to documentation

leonardehrenfried · 2023-04-13T14:12:35Z

We discussed this at today's dev meeting and we have come to the conclusion that we want to have a meeting to come up with a plan for further features around the StreetEdge first. @optionsome will fill you in.

optionsome

We discussed this issue today in a meeting and came to the conclusion that we should at least try to implement this as a EscalatorEdge instead of using the StreetEdge with escalator flag as the StreetEdge implements BikeWalkableEdge and it makes no sense in this case and the StreetEdge is already a big file so adding more logic into it is not ideal. Contrary to what I suggested in a private message a couple of weeks ago, we shouldn't create a BikeUnwalkableEdge but instead put the logic directly into EscalatorEdge.

@leonardehrenfried suggested that we should also ideally handle the other conveying tag values listed in https://wiki.openstreetmap.org/wiki/Key:conveying to handle one way escalators. conveying:reversible and conveying:yes should be handled both as bidirectional.

optionsome · 2023-04-20T12:10:00Z

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

  ) {
    if (edgeIsStairs) {
      return pref.walk().stairsReluctance();
+    } else if (edgeIsEscalator) {
+      if (traverseMode == TraverseMode.BICYCLE) return Double.POSITIVE_INFINITY;


I think we shouldn't use really high cost but instead prevent "bicycle walking" and cycling in escalators completely.

…ator-reluctance

optionsome

I quickly tested this and it did seem to work so that only walking works. However, it also allowed traversal on wheelchairs which is wrong. The traversal permission map layer on the debug client showed the escalators as link (or maybe the edge was completely missing there). That code should be updated so it renders these edges with escalator text and maybe pink color like the stairs.

optionsome · 2023-05-23T14:05:38Z

src/main/java/org/opentripplanner/graph_builder/module/osm/EscalatorProcessor.java

+    return (
+      "steps".equals(osmWay.getTag("highway")) &&
+      osmWay.getTag("conveying") != null &&
+      Set.of("yes", "forward", "backward", "reversible").contains(osmWay.getTag("conveying"))


I think we should set the escalator as one directional when the value is either forward or backward. There are probably some examples how it's done for one way streets in code.

Also, I think you should have the method in the OSM way for checking if the way is an escalator (like you already have but slightly modify it).

optionsome · 2023-05-23T14:23:58Z

src/main/java/org/opentripplanner/graph_builder/module/osm/EscalatorProcessor.java

+/**
+ * Contains the logic for extracting elevators out of OSM data
+ */
+class EscalatorProcessor {


I'm not 100% sure if having an processor that goes through all the ways is the best solution or having some common top level processor for each way that then delegates the handling to separate processor based on some information. We can discuss this at a dev meeting.

Yes, it makes sense not to go through all the ways for the escalators separately if it can be avoided. This could probably be done in the same function that generates the street edges as well

You could move the code that generates the StreetEdges into an own class (or classes) from the OsmModule and just initialize these processors in the OsmModule. I think you can iterate over the ways there and then just call either buildEscalatorEdge or buildStreetEdge method from one of those processor depending when is appropriate for the way.

optionsome · 2023-05-23T14:29:00Z

src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java

@@ -252,7 +259,9 @@ private void buildBasicGraph() {
        params.banDiscouragedBiking(),
        issueStore
      );
-      if (!OsmFilter.isWayRoutable(way) || permissions.allowsNothing()) continue;
+      if (
+        !OsmFilter.isWayRoutable(way) || permissions.allowsNothing() || way.hasTag("conveying")


I think it's better to use the isEscalator method here as well from the OSMWay class. Also, wrap the continue with {} and put it on an own line (I know it was like this before but it shouldn't be).

src/main/java/org/opentripplanner/openstreetmap/model/OSMWay.java

optionsome · 2023-05-23T14:34:18Z

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

  private WalkPreferences() {
    this.speed = 1.33;
    this.reluctance = 2.0;
    this.boardCost = 60 * 10;
    this.stairsReluctance = 2.0;
    this.stairsTimeFactor = 3.0;
    this.safetyFactor = 1.0;
+    this.escalatorReluctance = 2.0;


I think this value should be lower. Not quite sure what though. Kinda weird that the default for reluctance and stairsReluctance are the same. Maybe the stairsReluctance is added on top of the normal reluctance but I'm not sure.

We investigated the stairsReluctance a bit in the dev meeting and the cost comes from both the stairsReluctance and the stairsTimeFactor so the default reluctance value is probably approriate as is.

For the escalatorReluctance, maybe set this default value as 1.5.

optionsome · 2023-05-23T14:39:39Z

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

+  @Override
+  public State[] traverse(State s0) {
+    // Only allow traversal by walking
+    if (s0.getNonTransitMode() == TraverseMode.WALK) {


Is the getNonTransitMode WALK also when one is walking a bicycle or BICYCLE. Maybe it's just some flag set in the state data and the mode is still BICYCLE. I'm not sure. We should also block traversal on wheelchairs here.

We think that the getNonTransitMode returns BICYCLE even when walking a bicycle so this code is probably right but the wheelchair should still be blocked somehow (there are probably examples in the street edge).

optionsome · 2023-05-23T14:40:51Z

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

+    // Only allow traversal by walking
+    if (s0.getNonTransitMode() == TraverseMode.WALK) {
+      var s1 = s0.edit(this);
+      var time = getDistanceMeters() / s0.getPreferences().walk().speed();


I wonder if we should consider that walk speed preference here at all? We could just have some static (maybe configurable) speed. I wonder if the speed is ever set through tagging in OSM.

You could try to search the internet if you can find what is usually the horizontal velocity of escalators and just use that as a static speed for the escalator instead of using the walk speed from preferences.

This still needs to be done.

src/main/java/org/opentripplanner/graph_builder/module/osm/EscalatorProcessor.java

leonardehrenfried · 2023-05-23T18:35:42Z

src/main/java/org/opentripplanner/standalone/config/routerequest/RouteRequestConfig.java

@@ -778,6 +778,7 @@ private static void mapWalkPreferences(NodeAdapter c, WalkPreferences.Builder wa
            "Value should be between 0 and 1." + " If the value is set to be 0, safety is ignored."
          )
          .asDouble(dft.safetyFactor())
-      );
+      )
+      .withEscalatorReluctance(c.of("escalatorReluctance").asDouble(dft.escalatorReluctance()));


Please add a summary and also an example to https://github.com/opentripplanner/OpenTripPlanner/blob/8db191804459bdb027559361b19ef78c60cdadc4/src/test/resources/standalone/config/router-config.json#L1-L0 (which will then show up in the documentation).

leonardehrenfried · 2023-05-23T18:41:39Z

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

+import org.opentripplanner.street.search.state.State;
+
+/** Represents an escalator. An escalator edge can only be traversed by walking */
+public class EscalatorEdge extends Edge {


Please add a test for this class. You can use PathwayEdgeTest as a template.

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

codecov · 2023-05-25T12:27:57Z

Codecov Report

Patch coverage: 75.32% and project coverage change: -0.02 ⚠️

Comparison is base (de5c168) 65.60% compared to head (4d870cc) 65.59%.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5046      +/-   ##
=============================================
- Coverage      65.60%   65.59%   -0.02%     
- Complexity     14656    14668      +12     
=============================================
  Files           1765     1767       +2     
  Lines          68455    68521      +66     
  Branches        7280     7291      +11     
=============================================
+ Hits           44911    44947      +36     
- Misses         21055    21078      +23     
- Partials        2489     2496       +7

Impacted Files	Coverage Δ
...er/graph_builder/module/osm/ElevatorProcessor.java	`54.43% <0.00%> (-2.54%)`	⬇️
...ector/raster/TraversalPermissionsEdgeRenderer.java	`14.49% <0.00%> (-0.66%)`	⬇️
.../opentripplanner/street/model/edge/StreetEdge.java	`86.96% <ø> (ø)`
...rg/opentripplanner/openstreetmap/model/OSMWay.java	`85.36% <50.00%> (-6.07%)`	⬇️
...r/graph_builder/module/osm/EscalatorProcessor.java	`64.00% <64.00%> (ø)`
...outing/api/request/preference/WalkPreferences.java	`85.10% <90.00%> (+0.40%)`	⬆️
...entripplanner/street/model/edge/EscalatorEdge.java	`92.30% <92.30%> (ø)`
...ripplanner/graph_builder/module/osm/OsmModule.java	`92.94% <100.00%> (+0.17%)`	⬆️
...dalone/config/routerequest/RouteRequestConfig.java	`97.83% <100.00%> (+0.02%)`	⬆️

... and 4 files with indirect coverage changes

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

leonardehrenfried · 2023-06-08T10:43:20Z

@nurmAV I did a refactoring which will allow you to remove the EscalatorProcessor and be a lot more efficient.

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

nurmAV · 2023-06-29T08:05:24Z

On top of my review comments, you also need to regenerate the documentation.

Is there some command for this? I'm a bit confused by this error as when I run mvn test locally, the tests pass and it looks like to me that neither of the expected or found lines match the line in the actual document (RouteRequest line 575)

leonardehrenfried · 2023-06-29T08:42:30Z

CI merges your branch and dev-2.x. It might be that that is the problem. To solve it merge dev-2.x into this branch and run mvn test.

leonardehrenfried · 2023-06-29T13:48:50Z

src/main/resources/WayProperties_de.properties

@@ -22,6 +22,7 @@ name.ramp=Auffahrrampe
 name.link=Verbindung


One last request: please add a default translation for English. Then I'm happy to approve.

…ance

leonardehrenfried · 2023-07-10T08:56:43Z

src/test/java/org/opentripplanner/street/model/edge/EscalatorEdgeTest.java

+
+class EscalatorEdgeTest {
+
+  Graph graph = new Graph();


Suggested change

Graph graph = new Graph();

You can remove the graph completely.

Please remove this and I will approve.

viljaminurminen-cgi added 4 commits April 11, 2023 20:06

Add escalator reluctance

1949f8c

Disable traversing an escalator when walking bike

f9460ef

Clean up code

f005b80

Add escalatorReluctance parameter to documentation

22efec1

nurmAV requested a review from a team as a code owner April 13, 2023 13:09

nurmAV changed the title ~~Escalator reluctance~~ Add escalator reluctance parameter Apr 13, 2023

leonardehrenfried marked this pull request as draft April 18, 2023 08:18

optionsome reviewed Apr 20, 2023

View reviewed changes

t2gran added this to the 2.4 milestone Apr 24, 2023

viljaminurminen-cgi added 6 commits May 23, 2023 09:27

Remove escalator related logic from street edge

95bfd98

Add escalator edge and escalator processor

7cf952c

Merge remote-tracking branch 'origin/escalator-reluctance' into escal…

459bc7d

…ator-reluctance

Merge branch 'otp-dev-2.x' into escalator-reluctance

1062213

Fix escalator edge and processor

7594af1

Fix formatting and disable street edge construction for escalators

d205133

nurmAV marked this pull request as ready for review May 23, 2023 12:08

optionsome reviewed May 23, 2023

View reviewed changes

src/main/java/org/opentripplanner/graph_builder/module/osm/EscalatorProcessor.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed May 23, 2023

View reviewed changes

Apply suggestions from code review

8a4dffa

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

viljaminurminen-cgi added 3 commits June 1, 2023 08:36

Add EscalatorEdgeTest

f214eba

Add support for one-way escalators

5dfc97b

Make escalator edges not traversable by wheelchair

872e8c5

leonardehrenfried mentioned this pull request Jun 8, 2023

Extract stairs handling into separate edge #5177

Closed

viljaminurminen-cgi added 2 commits June 13, 2023 09:03

Add summary and example on escalatorReluctance

6e2425f

Allow bicycles into elevators unless explicitly forbidden

024012c

nurmAV and others added 4 commits June 27, 2023 12:39

Apply suggestions from code review

615d66e

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

Add documentation and naming for escalator edges

2623666

Remove unused variables and parameters

56f19ce

Add test for configurable escalator reluctance

3594e1f

viljaminurminen-cgi added 2 commits June 29, 2023 12:18

Merge branch 'otp-dev-2.x' into escalator-reluctance

f25875e

Update documentation

47b2e94

leonardehrenfried requested review from leonardehrenfried and optionsome June 29, 2023 13:09

leonardehrenfried reviewed Jun 29, 2023

View reviewed changes

optionsome added 3 commits July 10, 2023 11:23

Add default name

df7472e

Merge remote-tracking branch 'upstream/dev-2.x' into escalator-reluct…

6f08ae8

…ance

Update tests

73fc0f3

leonardehrenfried reviewed Jul 10, 2023

View reviewed changes

optionsome previously approved these changes Jul 10, 2023

View reviewed changes

optionsome requested a review from leonardehrenfried July 10, 2023 09:25

Remove unnecessary variable

e935c8a

optionsome dismissed their stale review via e935c8a July 10, 2023 11:27

leonardehrenfried previously approved these changes Jul 10, 2023

View reviewed changes

optionsome previously approved these changes Jul 10, 2023

View reviewed changes

Fix formatting

4d870cc

optionsome dismissed stale reviews from leonardehrenfried and themself via 4d870cc July 10, 2023 11:30

optionsome approved these changes Jul 10, 2023

View reviewed changes

leonardehrenfried approved these changes Jul 10, 2023

View reviewed changes

optionsome merged commit fb8a777 into opentripplanner:dev-2.x Jul 10, 2023
5 checks passed

optionsome deleted the escalator-reluctance branch July 10, 2023 12:13

t2gran pushed a commit that referenced this pull request Jul 10, 2023

Add changelog entry for #5046 [ci skip]

b0f6c95

leonardehrenfried mentioned this pull request Jul 21, 2023

Insert escalators into graph after construction #5256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add escalator reluctance parameter #5046

Add escalator reluctance parameter #5046

nurmAV commented Apr 13, 2023

leonardehrenfried commented Apr 13, 2023

optionsome left a comment •

edited

optionsome Apr 20, 2023

optionsome left a comment

optionsome May 23, 2023

optionsome May 23, 2023

optionsome May 23, 2023

nurmAV May 25, 2023

optionsome May 25, 2023

optionsome May 23, 2023

optionsome May 23, 2023

optionsome May 25, 2023

optionsome May 23, 2023

optionsome May 25, 2023

optionsome May 23, 2023

optionsome May 25, 2023

optionsome Jun 20, 2023

leonardehrenfried May 23, 2023

leonardehrenfried May 23, 2023

codecov bot commented May 25, 2023 •

edited

leonardehrenfried commented Jun 8, 2023

nurmAV commented Jun 29, 2023

leonardehrenfried commented Jun 29, 2023

leonardehrenfried Jun 29, 2023

leonardehrenfried Jul 10, 2023

leonardehrenfried Jul 10, 2023

optionsome Jul 10, 2023

		@@ -22,6 +22,7 @@ name.ramp=Auffahrrampe
		name.link=Verbindung

Add escalator reluctance parameter #5046

Add escalator reluctance parameter #5046

Conversation

nurmAV commented Apr 13, 2023

Summary

Unit tests

Documentation

leonardehrenfried commented Apr 13, 2023

optionsome left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optionsome left a 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

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

Choose a reason for hiding this comment

codecov bot commented May 25, 2023 • edited

Codecov Report

leonardehrenfried commented Jun 8, 2023

nurmAV commented Jun 29, 2023

leonardehrenfried commented Jun 29, 2023

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

optionsome left a comment •

edited

codecov bot commented May 25, 2023 •

edited