Add car ferry functionality by VillePihlava · Pull Request #5966 · opentripplanner/OpenTripPlanner

VillePihlava · 2024-07-11T13:36:43Z

Summary

This PR adds the carry ferry feature. Using this feature requires compatible GTFS data with the cars_allowed field in the trips.txt file. This field is used similarly to how bikes_allowed is used. Also { "modes": "CAR" } should be added to the transferRequests section of the build-config.json file.

Things that have been manually tested include:

Using a ferry on a trip with a car
Using multiple ferries on a trip with a car

Things that are not in the scope of this pr:

Compatibility with Netex.
More configurability for car transfers (e.g. changing transferRequests to allow for vehicle-specific maxTransferDuration).

Issue

Closes #5875

Unit tests

Added.

Documentation

No documentation has been added.

Bumping the serialization version id

Needed.

leonardehrenfried · 2024-07-11T14:20:32Z

This looks already quite promising.

Under which conditions should a stop be linked to a drivable edge? Do you want to make it explicit in the transit data or do you want infer it from the fact that a car-enabled route stops there?

VillePihlava · 2024-07-11T14:38:45Z

Under which conditions should a stop be linked to a drivable edge? Do you want to make it explicit in the transit data or do you want infer it from the fact that a car-enabled route stops there?

This is a good question. We have discussed this quite a lot and the question remains somewhat open. The information for stops can be inferred from a car-enabled ferry using the stop as you said. At least currently I'll try to go with that. However, there is some discussion on changing the gtfs standard related to this here: google/transit#466. Depending on how the discussion progresses will also affect this quite a lot. The changes might also affect the bikes_allowed field and others, so doing something like this could work:

Adding just cars_allowed to trips.txt in the onebusaway repository
-> initial implementation in OTP with just cars_allowed
-> waiting for the final version of the car ferry changes to the gtfs standard
-> changing everything related at the same time (cars, bikes, wheelchairs, etc)

VillePihlava · 2024-09-03T13:54:24Z

@t2gran @optionsome @leonardehrenfried assuming possible changes to transfers (e.g. changing transferRequests to allow for vehicle-specific maxTransferDuration) will be split into a separate pr, this pr is now essentially ready-to-review. I will open this pr when a new release has been created from the https://github.com/OneBusAway/onebusaway-gtfs-modules library and OTP uses it.

leonardehrenfried · 2024-09-03T14:18:53Z

Has been released: https://github.com/OneBusAway/onebusaway-gtfs-modules/releases

…into car-ferry-changes

codecov · 2024-09-11T04:56:59Z

Codecov Report

Attention: Patch coverage is 88.40580% with 8 lines in your changes missing coverage. Please review.

Project coverage is 69.95%. Comparing base (a574bc3) to head (02aaf42).
Report is 28 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...fs/mapping/routerequest/ModePreferencesMapper.java	0.00%	1 Missing and 1 partial ⚠️
...is/gtfs/mapping/routerequest/AccessModeMapper.java	0.00%	1 Missing ⚠️
...is/gtfs/mapping/routerequest/EgressModeMapper.java	0.00%	1 Missing ⚠️
.../gtfs/mapping/routerequest/TransferModeMapper.java	0.00%	1 Missing ⚠️
...er/apis/transmodel/mapping/RequestModesMapper.java	66.66%	0 Missing and 1 partial ⚠️
.../opentripplanner/gtfs/mapping/CarAccessMapper.java	83.33%	1 Missing ⚠️
...request/RouteRequestTransitDataProviderFilter.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #5966   +/-   ##
==========================================
  Coverage      69.94%   69.95%           
- Complexity     17744    17762   +18     
==========================================
  Files           1998     2000    +2     
  Lines          75454    75505   +51     
  Branches        7723     7728    +5     
==========================================
+ Hits           52778    52818   +40     
- Misses         20007    20012    +5     
- Partials        2669     2675    +6

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

leonardehrenfried · 2024-09-11T05:14:45Z

@optionsome I don't think we assigned reviewers yesterday, correct?

tsherlockcraig · 2024-09-11T14:40:39Z

I just posted to google/transit#466 in favor of the alternate approach that had been offered there by miklcct.

If the work for cars_allowed has already been performed, and the OTP developers don't believe switching to the alternate approach is feasible in the near term, we could potentially move forward with this as a "private extension" between GTFS producers and OTP. I do agree that cars_allowed would be slightly easier for WSDOT as a producer, but the boarding_permissions.txt approach is also reasonably simple, and more durable/extendable.

VillePihlava · 2024-09-11T15:26:21Z

@tsherlockcraig I posted a reply in the issue you mentioned. The arguments in that thread centered around adding changes to trips.txt + boarding_permissions.txt or just adding boarding_permissions.txt alone. For some cases, getting more detailed information from GTFS is nice.

This implementation should be enough for many cases. Extending this functionality, especially since it is based on an experimental onebusaway GTFS library addition (cars_allowed), would not be unreasonable :).

Also, for example, Netex support has not been implemented, so at some point development related to car ferries should come from that angle.

leonardehrenfried · 2024-09-13T07:43:11Z

  BICYCLE_RENTAL
  """
+  Driving a car from the origin to the destination.
+  TODO add more thorough description


Please resolve these TODOs.

TODOs are now resolved, the comments are based on the assumption that the access, egress, and transfer modes should all be equal when one is set to CAR. Once the issue brought up by Henrik is solved this might have to be changed

leonardehrenfried · 2024-09-13T07:43:46Z

  bikesAllowed: BikesAllowed
  blockId: String
+  "Whether cars are allowed on board the vehicle running this trip"
+  carsAllowed: CarsAllowed


Do you actually need this information in your UI? If not I would remove that field because we don't know how the standardisation process will play out.

I removed this

leonardehrenfried · 2024-09-13T07:44:49Z

+    switch (carsAllowed) {
+      case 1:
+        return CarAccess.ALLOWED;
+      case 2:
+        return CarAccess.NOT_ALLOWED;
+      default:
+        return CarAccess.UNKNOWN;
+    }


Suggested change

switch (carsAllowed) {

case 1:

return CarAccess.ALLOWED;

case 2:

return CarAccess.NOT_ALLOWED;

default:

return CarAccess.UNKNOWN;

}

return switch (carsAllowed) {

case 1 -> CarAccess.ALLOWED;

case 2 -> CarAccess.NOT_ALLOWED;

default -> CarAccess.UNKNOWN;

}

I changed it to this

leonardehrenfried · 2024-09-13T07:46:34Z

+    Set<StopLocation> stopLocationsUsedForCarsAllowedTrips = Set.of();
+    stopLocationsUsedForCarsAllowedTrips =
+      transitModel
+        .getAllTripPatterns()
+        .stream()
+        .filter(t ->
+          t
+            .getScheduledTimetable()
+            .getTripTimes()
+            .stream()
+            .anyMatch(tt -> tt.getTrip().getCarsAllowed() == CarAccess.ALLOWED)
+        )
+        .flatMap(t -> t.getStops().stream())
+        .collect(Collectors.toSet());
+
+    stopLocationsUsedForCarsAllowedTrips.addAll(
+      stopLocationsUsedForCarsAllowedTrips
+        .stream()
+        .filter(GroupStop.class::isInstance)
+        .map(GroupStop.class::cast)
+        .flatMap(g -> g.getChildLocations().stream().filter(RegularStop.class::isInstance))
+        .toList()
+    );


Can you extract that into a method and convert the line comments to Javadoc?

actually, can also extract a method for the flex stops?

I created methods for both in the transitModel. This will also be useful later when working on the transferRequest stuff related to cars on transit

leonardehrenfried · 2024-09-13T07:49:09Z

+    assertTrue(model.stopVertex().isConnectedToGraph());
+
+    // Because the stop is used by a carsAllowed trip it needs to be linked to both the walk and car edge
+    assertEquals(2, model.stopVertex().getOutgoing().size());


Suggested change

assertEquals(2, model.stopVertex().getOutgoing().size());

assertThat(model.stopVertex().getOutgoing()).hasSize(2);

Since I wrote this other test, we introduced the truth assertion library.

I changed all assertEquals in this file to assertThat

…into car-ferry-changes

leonardehrenfried · 2024-09-30T09:39:12Z

@optionsome can merge when HSL is ready.

optionsome · 2024-09-30T12:34:54Z

Lets wait a bit before merging this in. We need to do a production release of our fork first and then we will quickly test this in our dev to see that this doesn't cause any problems.

…into car-ferry-changes

VillePihlava · 2024-10-29T10:02:10Z

Final merge conflict resolve. According to @optionsome this can now be merged.

VillePihlava force-pushed the car-ferry-changes branch 2 times, most recently from 7292d80 to fb9e985 Compare September 3, 2024 13:31

VillePihlava added 3 commits September 3, 2024 16:39

Change GraphQL schema for carsAllowed and generate files.

b70c9d6

Add functionality for cars on trips.

3efe676

Add tests for cars on trips.

a555f3d

VillePihlava force-pushed the car-ferry-changes branch from fb9e985 to a555f3d Compare September 3, 2024 13:40

VillePihlava changed the title ~~Add car ferry feature [DRAFT]~~ Add car ferry functionality Sep 3, 2024

VillePihlava added 2 commits September 5, 2024 12:36

Remove changes to QualifiedModeSet.

5afc63c

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

0f574e8

…into car-ferry-changes

VillePihlava marked this pull request as ready for review September 9, 2024 11:41

VillePihlava requested a review from a team as a code owner September 9, 2024 11:41

Fix format.

c644034

optionsome added +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR !New Feature A functional feature targeting the end user. labels Sep 10, 2024

Fix format.

54752ba

habrahamsson-skanetrafiken requested review from habrahamsson-skanetrafiken and leonardehrenfried September 12, 2024 14:13

leonardehrenfried reviewed Sep 13, 2024

View reviewed changes

VillePihlava added 3 commits September 27, 2024 13:40

Change location of function and make if statement chain more compact.

1d36e7c

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

8c8ad82

…into car-ferry-changes

Change test to conform with new changes.

39634b5

leonardehrenfried previously approved these changes Sep 30, 2024

View reviewed changes

habrahamsson-skanetrafiken previously approved these changes Sep 30, 2024

View reviewed changes

VillePihlava mentioned this pull request Oct 15, 2024

DT-5921 Add car ferry functionality HSLdevcom/digitransit-ui#5121

Merged

VillePihlava added 2 commits October 17, 2024 12:09

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

232eaec

…into car-ferry-changes

Rename transitModel to timetableRepository.

9958ffc

VillePihlava dismissed stale reviews from habrahamsson-skanetrafiken and leonardehrenfried via 9958ffc October 17, 2024 10:08

VillePihlava requested review from habrahamsson-skanetrafiken and leonardehrenfried October 17, 2024 10:19

leonardehrenfried previously approved these changes Oct 17, 2024

View reviewed changes

habrahamsson-skanetrafiken previously approved these changes Oct 18, 2024

View reviewed changes

VillePihlava mentioned this pull request Oct 24, 2024

[Do not merge] DT-5921 Fix transfer cache requests HSLdevcom/OpenTripPlanner-data-container#416

Closed

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

02aaf42

…into car-ferry-changes

VillePihlava dismissed stale reviews from habrahamsson-skanetrafiken and leonardehrenfried via 02aaf42 October 29, 2024 10:01

VillePihlava requested review from habrahamsson-skanetrafiken and leonardehrenfried October 29, 2024 10:02

leonardehrenfried approved these changes Oct 29, 2024

View reviewed changes

optionsome approved these changes Oct 29, 2024

View reviewed changes

optionsome merged commit 1377722 into opentripplanner:dev-2.x Oct 29, 2024

optionsome deleted the car-ferry-changes branch October 29, 2024 21:37

t2gran pushed a commit that referenced this pull request Oct 29, 2024

Add changelog entry for #5966 [ci skip]

d49c4a6

t2gran pushed a commit that referenced this pull request Oct 29, 2024

Bump serialization version id for #5966

53a2132

VillePihlava mentioned this pull request Nov 1, 2024

Add 'transferParametersForMode' build config field #6215

Merged

	assertEquals(2, model.stopVertex().getOutgoing().size());
	assertThat(model.stopVertex().getOutgoing()).hasSize(2);

Uh oh!

Conversation

VillePihlava commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

Uh oh!

leonardehrenfried commented Jul 11, 2024

Uh oh!

VillePihlava commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VillePihlava commented Sep 3, 2024

Uh oh!

leonardehrenfried commented Sep 3, 2024

Uh oh!

codecov Bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leonardehrenfried commented Sep 11, 2024

Uh oh!

tsherlockcraig commented Sep 11, 2024

Uh oh!

VillePihlava commented Sep 11, 2024

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!

leonardehrenfried Sep 13, 2024 • 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!

leonardehrenfried commented Sep 30, 2024

Uh oh!

optionsome commented Sep 30, 2024

Uh oh!

VillePihlava commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

VillePihlava commented Jul 11, 2024 •

edited

Loading

VillePihlava commented Jul 11, 2024 •

edited

Loading

codecov Bot commented Sep 11, 2024 •

edited

Loading

leonardehrenfried Sep 13, 2024 •

edited

Loading