Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GTFS-RT extension to add completely new routes #4667

Merged
merged 54 commits into from
Jan 18, 2023

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Dec 9, 2022

Summary

This adds a GTFS-RT extension which allows you to add completely new routes that are unknown before the realtime update arrives.

There is one catch though: the stops do have to be known in advance.

It also allows you to define feeds where the transfer generator doesn't check if a stop is used by a pattern. This is useful when you want to add stops that you can later add realtime-added routes to. Otherwise they would not be linked for transfers.

Unit tests

Added.

Documentation

Autogenerated.

@leonardehrenfried leonardehrenfried added Real-Time Update The issue/PR is related to RealTime updates X Bbnavi ~ Not in use any more ~ labels Dec 9, 2022
@leonardehrenfried leonardehrenfried requested a review from a team as a code owner December 9, 2022 12:25
@leonardehrenfried leonardehrenfried changed the title Add GTFS-RT extension to add completely new routes GTFS-RT extension to add completely new routes Dec 9, 2022
@t2gran t2gran added this to the 2.3 milestone Dec 13, 2022
@hannesj hannesj self-requested a review December 13, 2022 10:04
docs/BuildConfiguration.md Outdated Show resolved Hide resolved
src/main/proto/mfdz-realtime-extensions.proto Show resolved Hide resolved
.withTimezone("Europe/Paris")
.build();
Route route;
if (tripDescriptor.hasExtension(MfdzRealtimeExtensions.tripDescriptor)) {
FeedScopedId id = tripDescriptor.hasRouteId()
? new FeedScopedId(tripId.getFeedId(), tripDescriptor.getRouteId())
: tripId;

var builder = Route.of(id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to check for a route that you might have already created? For instance if this trip is sent several time through the gtfs-rt interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good suggestion and I just tried it out.

However, the problem is that these ADDED routes are not added to the transit index so it's actually quite hard to figure out if it already exits.

Nevertheless, since the route is added to trip pattern which is cached it is still the same instance when there are two consecutive updates. I've also added a test to check that this is the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this is enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm good question, it reveals an existing potential issue when your trip times matches a pattern of another added trip times on another route and isn't properly added with the right route reference as a result. For car pool, if two users provide a similar trop between two city, only one of the user route will be created with two trip times. If there's important information in the route to identify and contact the driver that might be an issue.

Similarly for transit, if two added trips, initially for two different routes/bus lines, share the same set of stops, they might both be displayed as on the same bus line. I feel the cache should be depending on the route id too.

Also the route not being added to the graph index could be problematic if you want to get the route details by route id later on, I feel this would be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually found a way to add the route the graph index. It was rather easy: bcc4096

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked at the on commit above and found two issues - this is why we need to refactor the TransitModel. Of cause you can make the index for route a concurrent map, but by injecting the implementaion/editor into the Snapshot you exposes the Snapshot for threading issues. This adds another stone to the task of refactoring the transit model, but I am not sure if we have another choice...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a suggestion I'm happy to implement it.

BTW, the Siri updater does the same thing:

transitModel.getTransitModelIndex().addRoutes(route);

// the route in this update doesn't already exist, but the update contains the information so it will be created
else if (
tripDescriptor.hasExtension(MfdzRealtimeExtensions.tripDescriptor) &&
!routeExists(tripId.getFeedId(), tripDescriptor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routeExists(tripId.getFeedId(), tripDescriptor) is already checked on the previous if, so no need to check it a second time. There is still some shared code between the two blocks, but we can refactor it at a later stage

@leonardehrenfried leonardehrenfried merged commit 37489ac into opentripplanner:dev-2.x Jan 18, 2023
@leonardehrenfried leonardehrenfried deleted the gtfs-rt-extension branch January 18, 2023 14:49
t2gran pushed a commit that referenced this pull request Jan 18, 2023
@mpaine-act
Copy link

Does GTFS-RT Trip Modifications supersede GTFS-ServiceChanges?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Real-Time Update The issue/PR is related to RealTime updates X Bbnavi ~ Not in use any more ~
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants