Extract stairs handling into separate edge #5177

leonardehrenfried · 2023-06-08T10:42:08Z

Summary

As discussed in the A* meeting a few weeks ago, this extracts the handling of the stairs out of the StreetEdge and into its own edge.

This makes it easier to reason about the logic of StreetEdge and as the conceptual weight of it is decreased.

The newly created abstract base class OsmEdge will also make it easier to implement the escalator handling that @optionsome and @nurmAV are working on at #5046.

Unit tests

Added.

Bumping the serialization version id

Automatically.

codecov · 2023-06-08T11:02:29Z

Codecov Report

Patch coverage: 78.46% and project coverage change: +0.02 🎉

Comparison is base (499eb1c) 65.67% compared to head (5fbcca6) 65.69%.

❗ Current head 5fbcca6 differs from pull request most recent head 28c7ded. Consider uploading reports for the commit 28c7ded to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5177      +/-   ##
=============================================
+ Coverage      65.67%   65.69%   +0.02%     
- Complexity     14723    14739      +16     
=============================================
  Files           1769     1772       +3     
  Lines          68568    68609      +41     
  Branches        7288     7281       -7     
=============================================
+ Hits           45035    45076      +41     
- Misses         21037    21040       +3     
+ Partials        2496     2493       -3

Impacted Files	Coverage Δ
...ntripplanner/framework/geometry/GeometryUtils.java	`75.18% <ø> (ø)`
...raph_builder/module/osm/SafetyValueNormalizer.java	`94.44% <ø> (ø)`
...ector/raster/TraversalPermissionsEdgeRenderer.java	`15.87% <0.00%> (+1.38%)`	⬆️
.../opentripplanner/street/model/TurnRestriction.java	`72.72% <ø> (ø)`
...pplanner/street/model/edge/ElevatorAlightEdge.java	`93.33% <ø> (-0.42%)`	⬇️
...entripplanner/street/model/edge/EscalatorEdge.java	`92.30% <ø> (ø)`
...er/street/model/edge/StreetElevationExtension.java	`77.41% <ø> (-1.05%)`	⬇️
.../opentripplanner/street/model/edge/StreetEdge.java	`87.25% <54.54%> (+0.40%)`	⬆️
...org/opentripplanner/street/model/edge/OsmEdge.java	`60.00% <60.00%> (ø)`
...ipplanner/street/model/edge/BarrierCalculator.java	`83.33% <83.33%> (ø)`
... and 7 more

... and 1 file 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-08T11:22:40Z

...st/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/TransitSnapshotTest.snap

@@ -6314,10 +6314,62 @@ org.opentripplanner.routing.algorithm.mapping.TransitSnapshotTest.test_trip_plan
              "absoluteDirection": "WEST",
              "area": false,
              "bogusName": false,
-              "distance": 1470.24,
+              "distance": 5.54,


I tried hard to figure out why the results are different in this case but I failed. The new path takes a shortcut through a park which seems like an improvement to me.

optionsome · 2023-06-13T17:30:23Z

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

+/**
+ * Abstract base class for edges derived from OSM data.
+ */
+public abstract class OsmEdge extends Edge {


This should have different name and description but unfortunately I have no idea what.

Adding classes in a class hierarchy that "only" provide some common code is usually a bad idea, it lead to fragmented code. An in this case also extra memory usage. The only logic I can see is in the constructor, that could be extracted and shared in other ways. Can the many setters and the lack of polymorphic behaviour be replaced with a builder instead - or just revert?

I will wait with the review until this is resolved.

I agree that these setters are ugly but the problem here is that these setters are making the code more polymorphic, not less. If we removed these, then I have to put instanceOf in many places to figure out if that edge handles them or not. These setters are not only used to construct the object but also to modify the edges later on in the build process.

I also want to get rid of them but that would be a larger refactoring. We have two options here:

Always use builders until the last possible moment in the build process and then build the graph at the very end.

Make edges more immutable but allow it (perhaps via a builder) to replace them in a graph with a version with updated values.

All in all, I think that this is a topic for a dev meeting when Thomas is back from holiday.

I will be there today, if not I will try to join next Thursday. I think the second option is the best choice. To do that you can do the following (psudo code):

abstract class AbstractEdge<T> { /** Default implementation does nothing!!! */ T withX(x : X) : T { return this; } } class ElevatorEdge extends AbstractEdge<ElevatorEdge> { ElevatorEdge withX(x : T) : ElevatorEdge { var newEdge = new ElevatorEdge(..., x); replace_ this_with_newEdge(); return newEdge; } }

optionsome · 2023-06-13T17:31:00Z

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

+        inAngle = calculateAngle(DirectionUtils.getFirstAngle(geometry));
+      } catch (IllegalArgumentException iae) {
+        LOG.error(
+          "exception while determining street edge angles. setting to zero. there is probably something wrong with this street segment's geometry."


This isn't necessarily a street edge.

optionsome · 2023-06-13T17:32:14Z

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

+  private final I18NString name;
+  private final byte[] compactGeometry;
+  private final double distance;
+  private float walkSafetyFactor = 1f;


I don't think we should consider safety for stairs as we have a separate reluctance for stairs.

optionsome · 2023-06-13T17:34:46Z

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

+import org.opentripplanner.street.search.state.State;
+
+/**
+ * Represents a flight of stairs that are derived from OSM data.


Do we actually have to have different classes for stairs coming from OSM and GTFS pathways. What is the difference between stairs coming from different data sources? This is also a more general question, does it make sense to have the PathwayEdge class which can be stairs etc and then these specific edges coming from OSM data?

optionsome · 2023-06-13T17:41:27Z

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

This class has slightly confusing name as this is used by StreetEdge class but also by other classes that have no relation to that class.

leonardehrenfried · 2023-06-15T09:10:42Z

I'm going to put this PR on hold until we have worked out a strategy for the clean up when Thomas is back from holiday.

leonardehrenfried · 2024-04-02T15:50:25Z

I'm no longer going to work on this.

leonardehrenfried added 14 commits June 8, 2023 10:03

Extract stairs edge

62d8567

Make implementation more elaborate

1afda7a

Extract more stairs details

b492ded

Remove isStairs() method

770ce09

Extract angle handling

f9c716e

Properly set up test data

22695c3

Move barrier handling to stairs

37d1715

Allow shortcut through park

3edd485

Renumber flags

c7fc9f3

Clean up and documentation

b4e1edd

Remove some code

b1fa4ec

Move checks into OSMWithTags

2f61fa5

Use compact geometry

ec19bcf

Move angle handling into base class

487e372

leonardehrenfried added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Jun 8, 2023

leonardehrenfried requested a review from a team as a code owner June 8, 2023 10:42

Add test for StairsEdge

40f7e9e

leonardehrenfried force-pushed the stairs-edge branch from 4fc9378 to 40f7e9e Compare June 8, 2023 10:49

leonardehrenfried commented Jun 8, 2023

View reviewed changes

leonardehrenfried added the IBI Developed by or important for IBI Group label Jun 8, 2023

leonardehrenfried requested review from t2gran and optionsome June 8, 2023 14:37

optionsome reviewed Jun 13, 2023

View reviewed changes

t2gran added this to the 2.4 (next release) milestone Jun 14, 2023

leonardehrenfried marked this pull request as draft June 22, 2023 13:32

leonardehrenfried added 2 commits July 17, 2023 15:44

Merge remote-tracking branch 'upstream/dev-2.x' into stairs-edge

1661744

Resolve merge conflicts

94d21a6

leonardehrenfried added 2 commits July 17, 2023 15:47

Fix transit test

5fbcca6

Revert using of OsmVertex

28c7ded

t2gran modified the milestones: 2.4, 2.5 (next release) Sep 13, 2023

t2gran modified the milestones: 2.5, 2.6 (next release) Mar 13, 2024

leonardehrenfried closed this Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract stairs handling into separate edge #5177

Extract stairs handling into separate edge #5177

leonardehrenfried commented Jun 8, 2023

codecov bot commented Jun 8, 2023 •

edited

Loading

leonardehrenfried Jun 8, 2023

optionsome Jun 13, 2023

t2gran Jun 13, 2023 •

edited

Loading

leonardehrenfried Jun 15, 2023

t2gran Jun 15, 2023 •

edited

Loading

optionsome Jun 13, 2023

optionsome Jun 13, 2023

optionsome Jun 13, 2023

optionsome Jun 13, 2023

leonardehrenfried commented Jun 15, 2023

leonardehrenfried commented Apr 2, 2024

Extract stairs handling into separate edge #5177

Extract stairs handling into separate edge #5177

Conversation

leonardehrenfried commented Jun 8, 2023

Summary

Unit tests

Bumping the serialization version id

codecov bot commented Jun 8, 2023 • edited Loading

Codecov Report

leonardehrenfried Jun 8, 2023

Choose a reason for hiding this comment

optionsome Jun 13, 2023

Choose a reason for hiding this comment

t2gran Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

leonardehrenfried Jun 15, 2023

Choose a reason for hiding this comment

t2gran Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

optionsome Jun 13, 2023

Choose a reason for hiding this comment

optionsome Jun 13, 2023

Choose a reason for hiding this comment

optionsome Jun 13, 2023

Choose a reason for hiding this comment

optionsome Jun 13, 2023

Choose a reason for hiding this comment

leonardehrenfried commented Jun 15, 2023

leonardehrenfried commented Apr 2, 2024

codecov bot commented Jun 8, 2023 •

edited

Loading

t2gran Jun 13, 2023 •

edited

Loading

t2gran Jun 15, 2023 •

edited

Loading