Add a separate layer group in debug UI for elevator edges and vertices #6885

hjvuor · 2025-09-23T13:26:18Z

Summary

This PR adds a layer group called Elevators to the debug UI that contains instances of ElevatorVertex and ElevatorHopEdge
To properly draw ElevatorHopEdges in case their start and end points are not at the same location, this adds a getGeometry method where the geometry is computed from the start and end vertices of the edge.

Issue

This is related to improving indoor navigation #6829

Unit tests

style.json was regenerated by DebugStyleSpecTest.java

Bumping the serialization version id

Minor changes made to both ElevatorHopEdge and ElevatorVertex which requires to bump serialization id

leonardehrenfried · 2025-09-23T13:35:06Z

...ion/src/main/java/org/opentripplanner/routing/algorithm/mapping/StatesToWalkStepsMapper.java

      return;
    }

+    if (edge instanceof ElevatorHopEdge) {


Is this still necessary?

In case of ElevatorHopEdges, the condition for early return used to happen the line before because they didn't return a geometry. So this is something I had to add as a special case.

leonardehrenfried · 2025-09-23T13:35:53Z

application/src/main/java/org/opentripplanner/street/model/edge/ElevatorHopEdge.java

+  @Override
+  public LineString getGeometry() {
+    List<Coordinate> segmentCoordinates = Arrays.asList(fromv.getCoordinate(), tov.getCoordinate());
+    return GeometryUtils.getGeometryFactory()


You can use GeometryUtils.makeLineString here.

application/src/main/java/org/opentripplanner/street/model/edge/ElevatorHopEdge.java

application/src/main/java/org/opentripplanner/street/model/vertex/ElevatorVertex.java

codecov · 2025-09-23T13:38:46Z

Codecov Report

❌ Patch coverage is 60.46512% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.10%. Comparing base (d1ef037) to head (c8b8de7).
⚠️ Report is 602 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...tripplanner/street/model/edge/ElevatorHopEdge.java	0.00%	7 Missing ⚠️
...nner/inspector/vector/edge/EdgePropertyMapper.java	0.00%	6 Missing ⚠️
...lanner/inspector/vector/edge/EdgeLayerBuilder.java	0.00%	2 Missing ⚠️
.../inspector/vector/vertex/VertexPropertyMapper.java	0.00%	1 Missing ⚠️
...ripplanner/street/model/vertex/ElevatorVertex.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6885      +/-   ##
=============================================
- Coverage      72.10%   72.10%   -0.01%     
- Complexity     19666    20054     +388     
=============================================
  Files           2125     2177      +52     
  Lines          79522    80966    +1444     
  Branches        8049     8135      +86     
=============================================
+ Hits           57341    58378    +1037     
- Misses         19345    19713     +368     
- Partials        2836     2875      +39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

leonardehrenfried · 2025-09-24T08:17:40Z

Fix the formatting please.

leonardehrenfried · 2025-10-01T11:40:54Z

You need to resolve conflicts.

…fined geometry

optionsome · 2025-10-03T10:09:06Z

application/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java

+      StyleBuilder.ofId("elevator-hop-edge")
+        .group(ELEVATORS_GROUP)
+        .typeLine()
+        .vectorSourceLayer(edges)
+        .edgeFilter(ElevatorHopEdge.class)
+        .lineColor(ORANGE)
+        .lineWidth(LINE_HALF_WIDTH)
+        .lineOffset(LINE_OFFSET)
+        .minZoom(6)
+        .maxZoom(MAX_ZOOM)
+        .intiallyHidden(),


These are slightly problematic as they are used for all elevators, but only visible for the horizontal elevators currently since the line has 0 length usually. I was thinking should we rename this so it's more clear that these will only show up for the horizontal elevators or somehow draw them as circles when they have 0 length and as lines when they don't. However, it's also possible that we don't do anything about it now.

I somewhat agree but I still think that since we don't do any special filtering for these edges we'd want the name to represent all of them. Then the ui just draws what it can

application/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java

hjvuor · 2025-11-07T09:01:29Z

I've modified this PR to add a getGeometryForDebugUi method with a default implementation in the Edge class. This way I can implement the special case for ElevatorHopEdge without the risk of breaking something by implementing getGeometry. In case the elevator edge has no horizontal component, I've also added logic to move the other end a little to make it visible in the UI. I'm not sure if that's the cleanest way to achieve what we want but I'd like to hear some opinions.

optionsome · 2025-11-07T09:54:10Z

application/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java

+          kv("permission", e.getPermission()),
+          kv("levels", e.getLevels()),
+          kv("wheelchairAccessible", e.isWheelchairAccessible()),
+          kv("travelTime", e.getTravelTime())


The default value for the travel time is 0 which means that it's not actually being used. Maybe it would be less confusing to not expose this field for the edge if it's 0 or expose it as null as otherwise it leads to confusion.

Mapping zero to null can be a problem if the value from OSM is zero. Having a default value of -1 is also not good imo because it would represent traveling back in time. Maybe travelTime could be defined as a Nullable Duration instead of an int. We could also just leave it as it is and expect the user to be able to look up what the value represents. What does @leonardehrenfried think of this?

The method is only used in tests and now here, so we don't have to adjust any calling code. I would indeed return Optional<Duration> and an empty optional in the 0 case.

leonardehrenfried · 2025-11-07T10:23:16Z

application/src/main/java/org/opentripplanner/street/model/edge/ElevatorHopEdge.java

  }

+  @Override
+  public LineString getGeometryForDebugUi() {


I would like us to think if we consider how the edge should look like in the debug UI to be part of its domain model. If it is, then this code can stay here. It it isn't we should create a new mapper inside the inspector package.

But I generally agree that there can be debug-specific geometries.

This could be easily moved into inspector.vector.edge.EdgeLayerBuilder where it is called from and have a mapper there. I'm not sure which is better.

I think @t2gran suggested this current solution (although I'm not sure if he gave any name suggestions for the method name).

Okay then, lets call it getDebugGeometry and keep it here. Please add a bit of Javadoc about what it is.

hjvuor requested a review from a team as a code owner September 23, 2025 13:26

leonardehrenfried reviewed Sep 23, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/street/model/edge/ElevatorHopEdge.java Show resolved Hide resolved

leonardehrenfried reviewed Sep 23, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/street/model/vertex/ElevatorVertex.java Show resolved Hide resolved

hjvuor requested a review from leonardehrenfried September 24, 2025 07:51

leonardehrenfried previously approved these changes Sep 24, 2025

View reviewed changes

tkalvas added the +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR label Sep 25, 2025

leonardehrenfried requested a review from optionsome September 25, 2025 13:35

leonardehrenfried added +OTP Debug UI OTP bundled client, the OTP server is normally not changed and removed +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Sep 25, 2025

hjvuor added 9 commits October 3, 2025 09:09

add elevator vertices as a dedicated layer in debug ui

bb469d6

add geometry to elevatorHopEdge and visualize it as a debug layer

57eedbd

remove unused imports

4da3aaf

add early return for elevatorhopedge that was previously done on unde…

c5f7ea3

…fined geometry

move hop edge geometry to only be calculated on getGeometry call

fdb5bf5

swap layers so vertices get drawn over edges, add level information

67aac2b

simplify getGeometry and add javadoc

d765661

remove whitespace

633cf78

fix rebase

14f0bdb

hjvuor dismissed leonardehrenfried’s stale review via 14f0bdb October 3, 2025 07:45

hjvuor force-pushed the debug-elevators branch from 3e9e6dd to 14f0bdb Compare October 3, 2025 07:45

optionsome reviewed Oct 3, 2025

View reviewed changes

habrahamsson-skanetrafiken marked this pull request as draft October 16, 2025 13:51

add getGeometryForDebugUi and implement for elevatorhopedge

c8b8de7

hjvuor marked this pull request as ready for review November 7, 2025 09:28

optionsome reviewed Nov 7, 2025

View reviewed changes

leonardehrenfried reviewed Nov 7, 2025

View reviewed changes

Add a separate layer group in debug UI for elevator edges and vertices #6885

Are you sure you want to change the base?

Add a separate layer group in debug UI for elevator edges and vertices #6885

Uh oh!

Conversation

hjvuor commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Bumping the serialization version id

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leonardehrenfried commented Sep 24, 2025

Uh oh!

leonardehrenfried commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hjvuor commented Nov 7, 2025

Uh oh!

optionsome Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hjvuor commented Sep 23, 2025 •

edited

Loading

codecov bot commented Sep 23, 2025 •

edited

Loading

optionsome Nov 7, 2025 •

edited

Loading