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

leonardehrenfried · 2024-12-18T11:24:02Z

Summary

This takes over from #6076. We led @HenrikSundell down a few dead-ends which I'm sorry about.

I have analysed the problem of the inheritance of TransitEntranceVertex and StationEntranceVertex and have concluded that it's a problem that is too big to be solved in this PR.

The big question we have to answer is if we want to flatten the Vertex hierarchy and rely less on instanceOf checks to describe vertex behaviour. If we have clarified what we want (of which I'm not sure myself) we can change the code and unify TransitEntranceVertex and StationEntranceVertex.

Issue

Closes #6078

Unit tests

Tests added.

Bumping the serialization version id

✔️

cc @optionsome

# 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

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

optionsome · 2024-12-19T12:37:31Z

Can you add a bit of documentation for the RelativeDirection enum in the schema as well to document this CONTINUE/ENTER_STATION/EXIT_STATION relationship.

codecov · 2025-01-02T22:05:52Z

Codecov Report

Attention: Patch coverage is 59.81308% with 43 lines in your changes missing coverage. Please review.

Project coverage is 69.79%. Comparing base (2d012a3) to head (74106b5).
Report is 51 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ing/algorithm/mapping/StatesToWalkStepsMapper.java	25.00%	11 Missing and 1 partial ⚠️
...ner/street/model/vertex/StationEntranceVertex.java	0.00%	12 Missing ⚠️
...ipplanner/apis/gtfs/datafetchers/EntranceImpl.java	78.57%	3 Missing ⚠️
...nner/graph_builder/module/osm/VertexGenerator.java	40.00%	2 Missing and 1 partial ⚠️
...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 ⚠️
...r/street/model/edge/StreetTransitEntranceLink.java	60.00%	1 Missing and 1 partial ⚠️
...r/apis/transmodel/model/plan/PathGuidanceType.java	50.00%	1 Missing ⚠️
...er/graph_builder/module/osm/ElevatorProcessor.java	50.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6343      +/-   ##
=============================================
- Coverage      69.85%   69.79%   -0.07%     
- Complexity     17922    17956      +34     
=============================================
  Files           2035     2045      +10     
  Lines          76495    76737     +242     
  Branches        7824     7834      +10     
=============================================
+ Hits           53435    53557     +122     
- Misses         20324    20434     +110     
- Partials        2736     2746      +10

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

leonardehrenfried · 2025-01-02T22:10:46Z

Can you add a bit of documentation for the RelativeDirection enum in the schema as well to document this CONTINUE/ENTER_STATION/EXIT_STATION relationship.

Done in 0b6a74a

optionsome · 2025-01-03T15:04:35Z

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

+    - Passing through a station entrance or exit when it is not know whether the passenger is
+      entering or exiting. If known then entrance information is in the `step.entity` field.


It's a bit vague here what the "known" refers here to. Currently if it's known that are we entering or exiting a station, this information is not exposed, right? However, I think you are trying to refer to the other station information here. You could cross reference to the ENTER/EXIT_STATION here so it's more clear that those enum values are related. The step field is now called feature, not entity.

Yes, correct, if we are entering/exiting we get the information from pathways but don't actually add the entity feature.

Actually, it wasn't a lot of work to extract the Entrance from the pathway data so I did it here instead.

Not sure if you want to keep the "if available" line then.

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

leonardehrenfried · 2025-01-10T15:27:13Z

@optionsome It doesn't appear possible to map two internal enum values to the same API value. For that reason I added a mapper that does this before it even gets to the GraphQL library code.

optionsome · 2025-01-10T19:17:35Z

application/src/test/java/org/opentripplanner/apis/transmodel/model/EnumTypesTest.java

@@ -75,6 +84,18 @@ void testMap() {
    assertEquals("DocumentedEnumMapping[apiName=iH, internal=Hi]", mapping.toString());
  }

+  @ParameterizedTest
+  @EnumSource(RelativeDirection.class)


Cool, didn't know this was possible.

t2gran · 2025-01-14T14:16:33Z

...ation/src/main/java/org/opentripplanner/apis/transmodel/mapping/RelativeDirectionMapper.java

+      case ENTER_STATION,
+        EXIT_STATION,
+        ENTER_OR_EXIT_STATION,
+        FOLLOW_SIGNS -> RelativeDirection.CONTINUE;


Sorry for the late response here, but this can be dropped. We will just add the new enum values to the Transmodel GraphQL API - I can do it if you want.

The JS GraphQL lib has a validator to compare to schema files for breaking changes - adding Enum values are considered a dangerous change - not a braking change. It should be handled by defensive programming on the client side. So, at entur we want to add the new enum values and we will cross fingers and hope our clients did the right thing.

I also want to add a better unit-test to the enums we have today - so I can do that in the same change.

Could/should we create a CI test for validating that we are not creating breaking schema changes with the JS GraphQL lib?

I think we still need the mapper so to we don't ever expose ENTER_OR_EXIT_STATION.

HenrikSundell and others added 30 commits September 18, 2024 17:22

Add subway station entrances to walk steps

1d19a05

Add entity to walk steps

67f4b1b

Add more parameters to Entrance

9d18269

Move StepEntity classes

3b88288

Remove default name for subway station entrances

0a62bf8

Add option to turn on osm subway entrances in osmDefaults

528ab55

Fix walk step generation

401a405

Add step entity to graphql tests

438bc31

Rename variables to match with graphql

97c2de6

Rename variables

d844561

Rename function

02a07ea

Fix comments

5dc74dd

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

Remove println

5d55646

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

Remove unnecessary imports

d1067c6

Add accessibilty information to entrances

5c97ad1

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

Require entranceId

c9df52d

Rename methods

7a9a8f6

Update dosumentation

c9139e3

Update documentation

7b21024

Remove redundant null check

ce7719c

Add feature union to steps

b737411

Return feature based on relativeDirection

f547e07

cedarbaum mentioned this pull request Dec 24, 2024

Expose station entrance/exits in routing and directly #6081

Open

Use three states for accessibility

713143b

leonardehrenfried force-pushed the station-entrances branch from 8c83970 to b621a6d Compare January 2, 2025 22:10

Update documentation

0b6a74a

leonardehrenfried force-pushed the station-entrances branch from b621a6d to 0b6a74a Compare January 2, 2025 22:16

leonardehrenfried requested a review from optionsome January 2, 2025 22:22

optionsome reviewed Jan 3, 2025

View reviewed changes

leonardehrenfried added 4 commits January 6, 2025 09:33

Update docs

5b12c7f

Extract entrance from transit link

8c6fda4

Add test for extracting entrance from pathway data

bd94b13

Update schema docs

74106b5

leonardehrenfried requested a review from optionsome January 6, 2025 09:08

Rename 'code' to 'publicCode'

628bf95

leonardehrenfried force-pushed the station-entrances branch from 95db61e to 628bf95 Compare January 7, 2025 16:55

Fix mapping in Transmodel API

4248ffe

optionsome approved these changes Jan 10, 2025

View reviewed changes

optionsome requested a review from tkalvas January 14, 2025 10:15

leonardehrenfried enabled auto-merge January 14, 2025 14:13

t2gran reviewed Jan 14, 2025

View reviewed changes

tkalvas approved these changes Jan 16, 2025

View reviewed changes

leonardehrenfried merged commit 2691404 into opentripplanner:dev-2.x Jan 16, 2025
5 checks passed

t2gran pushed a commit that referenced this pull request Jan 16, 2025

Add changelog entry for #6343 [ci skip]

dd5e23d

t2gran pushed a commit that referenced this pull request Jan 16, 2025

Bump serialization version id for #6343

1030408

leonardehrenfried deleted the station-entrances branch January 16, 2025 14:23

This was referenced Jan 16, 2025

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

Closed

Extract mapper for Transmodel bookWhen, new values for relativeDirection #6393

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

leonardehrenfried commented Dec 18, 2024

optionsome commented Dec 19, 2024

codecov bot commented Jan 2, 2025 •

edited

Loading

leonardehrenfried commented Jan 2, 2025 •

edited

Loading

optionsome Jan 3, 2025

leonardehrenfried Jan 6, 2025

leonardehrenfried Jan 6, 2025

leonardehrenfried Jan 6, 2025

leonardehrenfried commented Jan 10, 2025

optionsome Jan 10, 2025

t2gran Jan 14, 2025

t2gran Jan 14, 2025

optionsome Jan 14, 2025

leonardehrenfried Jan 14, 2025

		- Passing through a station entrance or exit when it is not know whether the passenger is
		entering or exiting. If known then entrance information is in the `step.entity` field.

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

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

Conversation

leonardehrenfried commented Dec 18, 2024

Summary

Issue

Unit tests

Bumping the serialization version id

optionsome commented Dec 19, 2024

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

leonardehrenfried commented Jan 2, 2025 • 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

leonardehrenfried commented Jan 10, 2025

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 Jan 2, 2025 •

edited

Loading

leonardehrenfried commented Jan 2, 2025 •

edited

Loading