If configured, add subway station entrances from OSM to walk steps #6076

HenrikSundell · 2024-09-18T14:25:00Z

Summary

This PR gets subway station entrance names from osm and adds the to walk steps.

Issue

leonardehrenfried · 2024-09-20T07:48:53Z

We need to harmonise our approaches as #6082 does something similar.

# Conflicts: # application/src/main/java/org/opentripplanner/model/plan/Entrance.java # application/src/main/java/org/opentripplanner/model/plan/StepEntity.java # application/src/main/java/org/opentripplanner/street/model/vertex/StationEntranceVertex.java # application/src/main/java/org/opentripplanner/street/model/vertex/VertexFactory.java # application/src/main/java/org/opentripplanner/transit/service/datafetchers/StepEntityTypeResolver.java # doc/user/BuildConfiguration.md

...ication/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/StepEntityTypeResolver.java

optionsome · 2024-10-24T11:17:05Z

...ava/org/opentripplanner/graph_builder/module/osm/parameters/OsmExtractParametersBuilder.java

@@ -61,6 +72,10 @@ public ZoneId getTimeZone() {
    return timeZone;
  }

+  public boolean getIncludeOsmSubwayEntrances() {


This could probably be just named includeOsmSubwayEntrances.

optionsome · 2024-10-24T11:21:05Z

application/src/main/java/org/opentripplanner/model/plan/StepEntity.java

@@ -0,0 +1,3 @@
+package org.opentripplanner.model.plan;
+
+public abstract class StepEntity {}


We could discuss in a dev meeting if this is the best approach.

application/src/main/java/org/opentripplanner/osm/model/OsmNode.java

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

optionsome · 2024-10-24T11:26:54Z

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

+  public StationEntranceVertex(double x, double y, long nodeId, String entranceName) {
+    super(x, y, nodeId);
+    this.entranceName = entranceName;
+  }
+
+  public String getEntranceName() {
+    return entranceName;
+  }


I think this should be called entranceCode for the purpose we use. In the future, if we include entrances from GTFS with the same model, the name and code will be different things.

Or perhaps just code.

Why not publicCode ?

In GTFS terminology, it's called stop_code in stops.txt. I'm not a native english speaker, but public code doesn't make it more understandable for me vs code.

public say it is used as information to the public. There are also other codes used for other things, like privateCode used by agencies for internal references.

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

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

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

t2gran · 2024-10-24T16:04:35Z

application/src/main/java/org/opentripplanner/model/plan/WalkStep.java

+  public Object getEntity() {
+    return entity;
+  }
+


I would like to se a diagram with boxes - forget about the class design for a moment and just draw the things we want to add in the future (next 1- 2 years). Then we can discuss how to group them and map the relationship.

Return an Object is no-go, and so is Entrance extends StepEntity.

I drew some sort of a process diagram with boxes together with @HenrikSundell.

I think this method should probably still return StepEntity instead of Object but I think the step impl must return an Object for the entity which is then handled by StepEntityTypeResolver. It's possible that instead of doing the instanceof checks that are currently done to return correct schema types, we could cast the object to a StepEntity and then check if entrance field is non-null, or escalator field is non-null etc., but I'm not sure how much cleaner that solution is.

I just quickly tested and seems like it's possible to define a class that is used for an union from GraphQL (through codegen configuration). So with this current setup where the different entity types would have a shared base class, you could use that for the union. However, if they don't, then you need to return an Object from the stepImpl#entity() method.

New diagram

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

Co-authored-by: Joel Lappalainen <[email protected]>

application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/EntranceImpl.java

optionsome · 2024-11-08T09:54:27Z

application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/EntranceImpl.java

+      return entrance.getName() != null
+        ? org.opentripplanner.framework.graphql.GraphQLUtils.getTranslation(
+          entrance.getName(),
+          environment
+        )
+        : null;
+    };


import org.opentripplanner.framework.graphql.GraphQLUtils and just use GraphQLUtils.getTranslation. Also, you can mark the input parameter in the getTranslation method as nullable and just pass in the entrance's name to the method as it does the null checks.

The reason for the full path is that org.opentripplanner.apis.gtfs.GraphQLUtils is already imported.

optionsome · 2024-11-08T09:58:36Z

application/src/main/java/org/opentripplanner/model/plan/WalkStep.java

@@ -130,6 +134,13 @@ public String getExit() {
    return exit;
  }

+  /**
+   * Get information about building entrance or exit.


Maybe we should say "subway stations's entrance" instead of building here. Also since there is already getExit here, maybe rename this and the highway exit rename methods so it's really clear that one is for highways, and the other is for stations.

optionsome · 2024-11-08T10:00:22Z

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

+    // don't care what came before or comes after
+    var step = createWalkStep(forwardState, backState);
+
+    step.withRelativeDirection(RelativeDirection.ENTER_OR_EXIT_STATION);


You could add a comment here to explain that figuring out if something is an entrance or exit is difficult here.

optionsome · 2024-11-08T10:04:57Z

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

+  private final String code;
+  private final boolean accessible;
+
+  public StationEntranceVertex(double x, double y, long nodeId, String code, boolean accessible) {


There is already a class called TransitEntranceVertex. I think we could probably use that instead of this. Although, I don't know if we should reserve the use of it just for pathways? Thoughts @leonardehrenfried ?

We decided we try to reuse the TransitEntranceVertex.

optionsome · 2024-11-08T10:05:58Z

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -444,6 +444,18 @@ type Emissions {
  co2: Grams
 }

+"Station entrance or exit."


You could clarify here that this can originate either from GTFS or from OSM.

optionsome · 2024-11-08T10:06:25Z

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

+type Entrance {
+  "Short text or a number that identifies the entrance or exit for passengers. For example, `A` or `B`."
+  code: String
+  "ID of the entrance in the format of `FeedId:EntranceId`."


Same here, you could mention that if the feed is osm, the entrance is from OSM data.

optionsome · 2024-11-08T10:06:39Z

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

+  "Short text or a number that identifies the entrance or exit for passengers. For example, `A` or `B`."
+  code: String
+  "ID of the entrance in the format of `FeedId:EntranceId`."
+  entranceId: String


Suggested change

entranceId: String

entranceId: String!

optionsome · 2024-11-08T10:10:57Z

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -2645,6 +2657,8 @@ type step {
  distance: Float
  "The elevation profile as a list of { distance, elevation } values."
  elevationProfile: [elevationProfileComponent]
+  "Information about an station entrance or exit"
+  entrance: Entrance


Unless I'm totally mistaken, I think on this API level we still want to have the entity field which returns an entity union.

codecov · 2024-11-13T09:12:39Z

Codecov Report

Attention: Patch coverage is 59.00000% with 41 lines in your changes missing coverage. Please review.

Project coverage is 69.70%. Comparing base (2ead4da) to head (18b84f0).
Report is 143 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ing/algorithm/mapping/StatesToWalkStepsMapper.java	0.00%	15 Missing and 1 partial ⚠️
...ner/street/model/vertex/StationEntranceVertex.java	0.00%	8 Missing ⚠️
...nner/graph_builder/module/osm/VertexGenerator.java	33.33%	3 Missing and 1 partial ⚠️
...ipplanner/apis/gtfs/datafetchers/EntranceImpl.java	78.57%	3 Missing ⚠️
...pis/gtfs/datafetchers/StepFeatureTypeResolver.java	66.66%	1 Missing and 1 partial ⚠️
..._builder/module/configure/GraphBuilderModules.java	0.00%	2 Missing ⚠️
...ner/graph_builder/module/osm/OsmModuleBuilder.java	33.33%	2 Missing ⚠️
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java	0.00%	1 Missing ⚠️
...main/java/org/opentripplanner/osm/OsmProvider.java	66.66%	1 Missing ⚠️
...in/java/org/opentripplanner/osm/model/OsmNode.java	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6076      +/-   ##
=============================================
- Coverage      69.75%   69.70%   -0.06%     
- Complexity     17657    17704      +47     
=============================================
  Files           2007     2011       +4     
  Lines          75573    75899     +326     
  Branches        7734     7764      +30     
=============================================
+ Hits           52716    52902     +186     
- Misses         20144    20279     +135     
- Partials        2713     2718       +5

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

🚨 Try these New Features:

JS Bundle Analysis - Avoid shipping oversized bundles

optionsome

We discussed yesterday in a developer meeting if ENTER_OR_EXIT_STATION needs to be exposed through the API at all or should we just use CONTINUE. I've thought about this a bit more and I think I would be ok with just using CONTINUE but we need to document on the enum in the schema that it's used also when going through entrances and we don't know into what direction. We can still keep the ENTER_OR_EXIT_STATION in the internal model but just map it into CONTINUE on API level.

optionsome · 2024-11-14T14:51:58Z

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

+  private final String code;
+  private final boolean accessible;
+
+  public StationEntranceVertex(double x, double y, long nodeId, String code, boolean accessible) {


We decided we try to reuse the TransitEntranceVertex.

optionsome · 2024-11-15T10:33:46Z

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -3328,6 +3345,7 @@ enum RelativeDirection {
  CONTINUE


Add description for this enum value. Maybe we could add descriptions for some other values in this enum as well as not all of them are clear.

optionsome · 2024-12-03T08:49:42Z

application/src/main/java/org/opentripplanner/graph_builder/module/osm/VertexGenerator.java

+          .withCoordinate(new WgsCoordinate(coordinate))
+          .withCode(ref)
+          .withWheelchairAccessibility(
+            accessible ? Accessibility.POSSIBLE : Accessibility.NOT_POSSIBLE


Should we also check if the tag is defined? If not, the value should be NO_INFORMATION.

optionsome · 2024-12-03T08:51:51Z

application/src/main/java/org/opentripplanner/routing/linking/VertexLinker.java

@@ -356,19 +354,19 @@ private StreetVertex link(
      ll.getSegmentIndex() == 0 &&
      (ll.getSegmentFraction() < 1e-8 || ll.getSegmentFraction() * length < 0.1)
    ) {
-      start = (IntersectionVertex) edge.getFromVertex();
+      start = (Vertex) edge.getFromVertex();


Are these casts necessary here?

optionsome · 2024-12-03T08:52:13Z

application/src/main/java/org/opentripplanner/routing/linking/VertexLinker.java

@@ -380,7 +378,7 @@ else if (
          if (!linkedAreas.add(ael)) {
            return null;
          }
-          if (vertex instanceof IntersectionVertex iv) {
+          if (vertex instanceof Vertex iv) {


Are there vertices which are not instance of vertex?

optionsome · 2024-12-03T08:55:05Z

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

  protected StreetTransitEntityLink(
-    StreetVertex fromv,
-    T tov,
+    Vertex fromv,
+    TransitEntranceVertex tov,
    Accessibility wheelchairAccessibility
  ) {


Would it be enough to have a constructor that accepts (Vertex, Vertex, Accessibility)?

optionsome · 2024-12-03T08:58:03Z

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -3328,6 +3345,7 @@ enum RelativeDirection {
  CONTINUE
  DEPART
  ELEVATOR
+  ENTER_OR_EXIT_STATION


We don't want to expose this enum value to users, it should be mapped to CONTINUE.

t2gran · 2024-12-03T14:50:49Z

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

+import org.opentripplanner.street.search.TraverseMode;
 import org.opentripplanner.street.search.state.State;
+import org.opentripplanner.transit.model.basic.Accessibility;
+import org.opentripplanner.transit.model.site.AreaStop;


These imports (o.o.street.search and o.o.transit) is to me a "code smell". Does it create circular dependencies?

t2gran · 2024-12-03T15:32:00Z

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

@@ -35,6 +42,8 @@ public abstract class Vertex implements AStarVertex<State, Edge, Vertex>, Serial

  private transient Edge[] outgoing = new Edge[0];
  private RentalRestrictionExtension rentalRestrictions = RentalRestrictionExtension.NO_RESTRICTION;
+  private static final Set<AreaStop> EMPTY_SET = Set.of();
+  private Set<AreaStop> areaStops = EMPTY_SET;


Very few Vertexes have areaStops, I am not an expert on the AStar routing and design, but this does not look right to me.

Add subway station entrances to walk steps

1d19a05

HenrikSundell marked this pull request as ready for review September 19, 2024 09:31

HenrikSundell requested a review from a team as a code owner September 19, 2024 09:31

cedarbaum mentioned this pull request Sep 20, 2024

Expose station entrance/exits in routing and directly #6081

Open

leonardehrenfried mentioned this pull request Sep 20, 2024

Expose station entrances in GraphQL API #6082

Draft

HenrikSundell added 3 commits October 2, 2024 16:03

Add entity to walk steps

67f4b1b

Add more parameters to Entrance

9d18269

Move StepEntity classes

3b88288

t2gran added this to the 2.7 (next release) milestone Oct 16, 2024

HenrikSundell added 4 commits October 16, 2024 18:19

Remove default name for subway station entrances

0a62bf8

Add option to turn on osm subway entrances in osmDefaults

528ab55

Fix walk step generation

401a405

optionsome self-requested a review October 22, 2024 08:41

Add step entity to graphql tests

438bc31

optionsome added Config Change bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Oct 24, 2024

optionsome changed the title ~~Add subway station entrances to walk steps~~ If configured, add subway station entrances from OSM to walk steps Oct 24, 2024

optionsome reviewed Oct 24, 2024

View reviewed changes

t2gran reviewed Oct 24, 2024

View reviewed changes

HenrikSundell added 2 commits October 25, 2024 15:15

Rename variables to match with graphql

97c2de6

Rename variables

d844561

optionsome reviewed Oct 25, 2024

View reviewed changes

...ion/src/main/java/org/opentripplanner/routing/algorithm/mapping/StatesToWalkStepsMapper.java Outdated Show resolved Hide resolved

HenrikSundell and others added 5 commits October 27, 2024 12:39

Rename function

02a07ea

Fix comments

5dc74dd

Co-authored-by: Joel Lappalainen <[email protected]>

Remove println

5d55646

Co-authored-by: Joel Lappalainen <[email protected]>

Remove unnecessary imports

d1067c6

Add accessibilty information to entrances

5c97ad1

habrahamsson-skanetrafiken requested a review from t2gran October 29, 2024 09:24

habrahamsson-skanetrafiken requested a review from optionsome October 29, 2024 09:24

HenrikSundell added 7 commits November 7, 2024 13:22

Use existing entrance class for walk steps

e10e0a2

Fix EntranceImpl

34761fe

Add id to walk step entrances

c84b7cf

Remove old file

2ea8a52

Fix otp version

39b0db3

Remove unused import

3b6bf3f

Merge remote-tracking branch 'otp/dev-2.x' into station-entrances

36be3dc

optionsome requested changes Nov 8, 2024

View reviewed changes

HenrikSundell added 5 commits November 10, 2024 17:02

Require entranceId

c9df52d

Rename methods

7a9a8f6

Update dosumentation

c9139e3

Update documentation

7b21024

Remove redundant null check

ce7719c

HenrikSundell added 3 commits November 14, 2024 09:49

Add feature union to steps

b737411

Return feature based on relativeDirection

f547e07

Remove StepFeature class

18b84f0

optionsome reviewed Nov 15, 2024

View reviewed changes

habrahamsson-skanetrafiken requested a review from leonardehrenfried November 21, 2024 14:06

Reuse transit entrance vertex

2060016

optionsome reviewed Dec 3, 2024

View reviewed changes

t2gran reviewed Dec 3, 2024

View reviewed changes

leonardehrenfried mentioned this pull request Dec 18, 2024

If configured, add subway station entrances from OSM to walk steps #6343

Open

optionsome marked this pull request as draft December 19, 2024 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If configured, add subway station entrances from OSM to walk steps #6076

If configured, add subway station entrances from OSM to walk steps #6076

HenrikSundell commented Sep 18, 2024 •

edited

Loading

leonardehrenfried commented Sep 20, 2024

optionsome Oct 24, 2024

optionsome Oct 24, 2024

optionsome Oct 24, 2024

optionsome Oct 24, 2024

t2gran Oct 25, 2024 •

edited

Loading

optionsome Oct 25, 2024

t2gran Dec 3, 2024

t2gran Oct 24, 2024 •

edited

Loading

optionsome Oct 25, 2024

optionsome Oct 25, 2024

optionsome Oct 25, 2024

optionsome Oct 29, 2024

optionsome Nov 8, 2024

HenrikSundell Nov 10, 2024

optionsome Nov 8, 2024

optionsome Nov 8, 2024

optionsome Nov 8, 2024

optionsome Nov 14, 2024

optionsome Nov 8, 2024

optionsome Nov 8, 2024

optionsome Nov 8, 2024

optionsome Nov 8, 2024

codecov bot commented Nov 13, 2024 •

edited

Loading

optionsome left a comment

optionsome Nov 14, 2024

optionsome Nov 15, 2024

optionsome Dec 3, 2024

optionsome Dec 3, 2024

optionsome Dec 3, 2024

optionsome Dec 3, 2024

optionsome Dec 3, 2024

t2gran Dec 3, 2024

t2gran Dec 3, 2024

		@@ -0,0 +1,3 @@
		package org.opentripplanner.model.plan;

		public abstract class StepEntity {}

If configured, add subway station entrances from OSM to walk steps #6076

Are you sure you want to change the base?

If configured, add subway station entrances from OSM to walk steps #6076

Conversation

HenrikSundell commented Sep 18, 2024 • edited Loading

Summary

Issue

leonardehrenfried commented Sep 20, 2024

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 Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran Oct 24, 2024 • 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

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 Nov 13, 2024 • edited Loading

Codecov Report

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

HenrikSundell commented Sep 18, 2024 •

edited

Loading

t2gran Oct 25, 2024 •

edited

Loading

t2gran Oct 24, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading