From f7fc002b0b51669ea532a5d245dd2dd50b39310e Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Mon, 22 Jul 2024 14:00:41 +0200 Subject: [PATCH 01/10] Introduce timetable builder --- .../RealtimeResolverTest.java | 7 +- .../ext/siri/AddedTripBuilder.java | 18 +- .../module/TimeZoneAdjusterModule.java | 13 +- .../gtfs/GenerateTripPatternsOperation.java | 59 ++-- .../org/opentripplanner/model/Timetable.java | 89 ++---- .../model/TimetableBuilder.java | 131 ++++++++ .../model/TimetableSnapshot.java | 61 ++-- .../model/impl/OtpTransitServiceBuilder.java | 35 +- .../netex/mapping/TripPatternMapper.java | 4 +- .../transit/model/network/TripPattern.java | 73 +---- .../model/network/TripPatternBuilder.java | 39 ++- .../apis/gtfs/GraphQLIntegrationTest.java | 6 +- .../gtfs/PatternByServiceDatesFilterTest.java | 13 +- .../GenerateTripPatternsOperationTest.java | 302 ++++++++++++++++++ .../interlining/InterlineProcessorTest.java | 7 +- .../model/TimetableSnapshotTest.java | 75 +++-- .../opentripplanner/model/TimetableTest.java | 10 +- ...pTransitServiceBuilderLimitPeriodTest.java | 40 ++- .../model/plan/TestItineraryBuilder.java | 4 +- .../ScheduledTransitLegReferenceTest.java | 19 +- .../mappers/TripPatternForDateMapperTest.java | 3 +- .../transit/request/TestRouteData.java | 2 +- .../stoptimes/StopTimesHelperTest.java | 19 +- .../updater/trip/RealtimeTestEnvironment.java | 2 +- .../RealtimeVehicleMatcherTest.java | 6 +- 25 files changed, 739 insertions(+), 298 deletions(-) create mode 100644 src/main/java/org/opentripplanner/model/TimetableBuilder.java create mode 100644 src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java diff --git a/src/ext-test/java/org/opentripplanner/ext/realtimeresolver/RealtimeResolverTest.java b/src/ext-test/java/org/opentripplanner/ext/realtimeresolver/RealtimeResolverTest.java index 21d5a7f1696..e5b842a474a 100644 --- a/src/ext-test/java/org/opentripplanner/ext/realtimeresolver/RealtimeResolverTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/realtimeresolver/RealtimeResolverTest.java @@ -129,9 +129,12 @@ void testPopulateLegsWithRealtimeKeepStaySeated() { private static TripPattern delay(TripPattern pattern1, int seconds) { var originalTimeTable = pattern1.getScheduledTimetable(); - var delayedTimetable = new Timetable(pattern1); var delayedTripTimes = delay(originalTimeTable.getTripTimes(0), seconds); - delayedTimetable.addTripTimes(delayedTripTimes); + var delayedTimetable = Timetable + .of() + .withTripPattern(pattern1) + .addTripTimes(delayedTripTimes) + .build(); return pattern1.copy().withScheduledTimeTable(delayedTimetable).build(); } diff --git a/src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java b/src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java index cdbaa8f3d4c..ed2378a839d 100644 --- a/src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java +++ b/src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java @@ -210,13 +210,6 @@ Result build() { // TODO: We always create a new TripPattern to be able to modify its scheduled timetable StopPattern stopPattern = new StopPattern(aimedStopTimes); - TripPattern pattern = TripPattern - .of(getTripPatternId.apply(trip)) - .withRoute(trip.getRoute()) - .withMode(trip.getMode()) - .withNetexSubmode(trip.getNetexSubMode()) - .withStopPattern(stopPattern) - .build(); RealTimeTripTimes tripTimes = TripTimesFactory.tripTimes( trip, @@ -229,7 +222,16 @@ Result build() { // therefore they must be valid tripTimes.validateNonIncreasingTimes(); tripTimes.setServiceCode(transitService.getServiceCodeForId(trip.getServiceId())); - pattern.add(tripTimes); + + TripPattern pattern = TripPattern + .of(getTripPatternId.apply(trip)) + .withRoute(trip.getRoute()) + .withMode(trip.getMode()) + .withNetexSubmode(trip.getNetexSubMode()) + .withStopPattern(stopPattern) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) + .build(); + RealTimeTripTimes updatedTripTimes = tripTimes.copyScheduledTimes(); // Loop through calls again and apply updates diff --git a/src/main/java/org/opentripplanner/graph_builder/module/TimeZoneAdjusterModule.java b/src/main/java/org/opentripplanner/graph_builder/module/TimeZoneAdjusterModule.java index eb3674bf76a..7168066afe3 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/TimeZoneAdjusterModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/TimeZoneAdjusterModule.java @@ -7,6 +7,7 @@ import java.util.HashMap; import java.util.Map; import org.opentripplanner.graph_builder.model.GraphBuilderModule; +import org.opentripplanner.transit.model.network.TripPattern; import org.opentripplanner.transit.service.TransitModel; /** @@ -45,9 +46,15 @@ public void buildGraph() { return; } - pattern - .getScheduledTimetable() - .updateAllTripTimes(it -> it.adjustTimesToGraphTimeZone(timeShift)); + TripPattern updatedPattern = pattern + .copy() + .withScheduledTimeTableBuilder(builder -> + builder.updateAllTripTimes(tt -> tt.adjustTimesToGraphTimeZone(timeShift)) + ) + .build(); + // replace the original pattern with the updated pattern in the transit model + transitModel.addTripPattern(updatedPattern.getId(), updatedPattern); }); + transitModel.index(); } } diff --git a/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java b/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java index bf26f9c6d7b..84b25340e6c 100644 --- a/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java +++ b/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java @@ -23,6 +23,7 @@ import org.opentripplanner.transit.model.network.Route; import org.opentripplanner.transit.model.network.StopPattern; import org.opentripplanner.transit.model.network.TripPattern; +import org.opentripplanner.transit.model.network.TripPatternBuilder; import org.opentripplanner.transit.model.timetable.Direction; import org.opentripplanner.transit.model.timetable.FrequencyEntry; import org.opentripplanner.transit.model.timetable.Trip; @@ -40,13 +41,13 @@ public class GenerateTripPatternsOperation { private final Map tripPatternIdCounters = new HashMap<>(); - private final OtpTransitServiceBuilder transitDaoBuilder; + private final OtpTransitServiceBuilder transitServiceBuilder; private final DataImportIssueStore issueStore; private final Deduplicator deduplicator; private final Set calendarServiceIds; private final GeometryProcessor geometryProcessor; - private final Multimap tripPatterns; + private final Multimap tripPatternBuilders = ArrayListMultimap.create(); private final ListMultimap frequenciesForTrip = ArrayListMultimap.create(); private int freqCount = 0; @@ -59,18 +60,17 @@ public GenerateTripPatternsOperation( Set calendarServiceIds, GeometryProcessor geometryProcessor ) { - this.transitDaoBuilder = builder; + this.transitServiceBuilder = builder; this.issueStore = issueStore; this.deduplicator = deduplicator; this.calendarServiceIds = calendarServiceIds; this.geometryProcessor = geometryProcessor; - this.tripPatterns = transitDaoBuilder.getTripPatterns(); } public void run() { collectFrequencyByTrip(); - final Collection trips = transitDaoBuilder.getTripsById().values(); + final Collection trips = transitServiceBuilder.getTripsById().values(); var progressLogger = ProgressTracker.track("build trip patterns", 50_000, trips.size()); LOG.info(progressLogger.startMessage()); @@ -85,6 +85,14 @@ public void run() { } } + tripPatternBuilders + .values() + .stream() + .map(TripPatternBuilder::build) + .forEach(tripPattern -> + transitServiceBuilder.getTripPatterns().put(tripPattern.getStopPattern(), tripPattern) + ); + LOG.info(progressLogger.completeMessage()); LOG.info( "Added {} frequency-based and {} single-trip timetable entries.", @@ -107,7 +115,7 @@ public boolean hasScheduledTrips() { * the same trip can be added at once to the same Timetable/TripPattern. */ private void collectFrequencyByTrip() { - for (Frequency freq : transitDaoBuilder.getFrequencies()) { + for (Frequency freq : transitServiceBuilder.getFrequencies()) { frequenciesForTrip.put(freq.getTrip(), freq); } } @@ -119,7 +127,7 @@ private void buildTripPatternForTrip(Trip trip) { return; // Invalid trip, skip it, it will break later } - List stopTimes = transitDaoBuilder.getStopTimesSortedByTrip().get(trip); + List stopTimes = transitServiceBuilder.getStopTimesSortedByTrip().get(trip); // If after filtering this trip does not contain at least 2 stoptimes, it does not serve any purpose. var staticTripWithFewerThan2Stops = @@ -134,8 +142,7 @@ private void buildTripPatternForTrip(Trip trip) { // Get the existing TripPattern for this filtered StopPattern, or create one. StopPattern stopPattern = new StopPattern(stopTimes); - Direction direction = trip.getDirection(); - TripPattern tripPattern = findOrCreateTripPattern(stopPattern, trip, direction); + TripPatternBuilder tripPatternBuilder = findOrCreateTripPattern(stopPattern, trip); // Create a TripTimes object for this list of stoptimes, which form one trip. TripTimes tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, deduplicator); @@ -144,44 +151,42 @@ private void buildTripPatternForTrip(Trip trip) { List frequencies = frequenciesForTrip.get(trip); if (!frequencies.isEmpty()) { for (Frequency freq : frequencies) { - tripPattern.add(new FrequencyEntry(freq, tripTimes)); + tripPatternBuilder.withScheduledTimeTableBuilder(builder -> + builder.addFrequencyEntry(new FrequencyEntry(freq, tripTimes)) + ); freqCount++; } } // This trip was not frequency-based. Add the TripTimes directly to the TripPattern's scheduled timetable. else { - tripPattern.add(tripTimes); + tripPatternBuilder.withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)); scheduledCount++; } } - private TripPattern findOrCreateTripPattern( - StopPattern stopPattern, - Trip trip, - Direction direction - ) { + private TripPatternBuilder findOrCreateTripPattern(StopPattern stopPattern, Trip trip) { Route route = trip.getRoute(); - for (TripPattern tripPattern : tripPatterns.get(stopPattern)) { + Direction direction = trip.getDirection(); + for (TripPatternBuilder tripPatternBuilder : tripPatternBuilders.get(stopPattern)) { if ( - tripPattern.getRoute().equals(route) && - tripPattern.getDirection().equals(direction) && - tripPattern.getMode().equals(trip.getMode()) && - tripPattern.getNetexSubmode().equals(trip.getNetexSubMode()) + tripPatternBuilder.getRoute().equals(route) && + tripPatternBuilder.getDirection().equals(direction) && + tripPatternBuilder.getMode().equals(trip.getMode()) && + tripPatternBuilder.getNetexSubmode().equals(trip.getNetexSubMode()) ) { - return tripPattern; + return tripPatternBuilder; } } FeedScopedId patternId = generateUniqueIdForTripPattern(route, direction); - TripPattern tripPattern = TripPattern + TripPatternBuilder tripPatternBuilder = TripPattern .of(patternId) .withRoute(route) .withStopPattern(stopPattern) .withMode(trip.getMode()) .withNetexSubmode(trip.getNetexSubMode()) - .withHopGeometries(geometryProcessor.createHopGeometries(trip)) - .build(); - tripPatterns.put(stopPattern, tripPattern); - return tripPattern; + .withHopGeometries(geometryProcessor.createHopGeometries(trip)); + tripPatternBuilders.put(stopPattern, tripPatternBuilder); + return tripPatternBuilder; } /** diff --git a/src/main/java/org/opentripplanner/model/Timetable.java b/src/main/java/org/opentripplanner/model/Timetable.java index 10c384e6211..5e2e02c3ffd 100644 --- a/src/main/java/org/opentripplanner/model/Timetable.java +++ b/src/main/java/org/opentripplanner/model/Timetable.java @@ -19,10 +19,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; -import java.util.function.UnaryOperator; -import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.opentripplanner.framework.time.ServiceDateUtils; import org.opentripplanner.transit.model.framework.DataValidationException; @@ -48,8 +45,7 @@ * one Timetable when stop time updates are being applied: one for the scheduled stop times, one for * each snapshot of updated stop times, another for a working buffer of updated stop times, etc. *

- * TODO OTP2 - Move this to package: org.opentripplanner.model after as Entur NeTEx PRs are merged. - * Also consider moving its dependencies into package org.opentripplanner.routing. The NEW + * TODO OTP2 consider moving dependencies into package org.opentripplanner.routing. The NEW * Timetable should not have any dependencies to [?] */ public class Timetable implements Serializable { @@ -58,28 +54,31 @@ public class Timetable implements Serializable { private final TripPattern pattern; - private final List tripTimes = new ArrayList<>(); + private final List tripTimes; - private final List frequencyEntries = new ArrayList<>(); + private final List frequencyEntries; @Nullable private final LocalDate serviceDate; - /** Construct an empty Timetable. */ - public Timetable(TripPattern pattern) { - this.pattern = pattern; - this.serviceDate = null; + Timetable(TimetableBuilder timetableBuilder) { + this.pattern = timetableBuilder.getPattern(); + this.serviceDate = timetableBuilder.getServiceDate(); + tripTimes = List.copyOf(timetableBuilder.getTripTimes()); + frequencyEntries = List.copyOf(timetableBuilder.getFrequencies()); } /** * Copy constructor: create an un-indexed Timetable with the same TripTimes as the specified * timetable. */ - Timetable(Timetable tt, @Nonnull LocalDate serviceDate) { - Objects.requireNonNull(serviceDate); - tripTimes.addAll(tt.tripTimes); - this.serviceDate = serviceDate; - this.pattern = tt.pattern; + public static TimetableBuilder of(Timetable tt) { + return new TimetableBuilder(tt); + } + + /** Construct an empty Timetable. */ + public static TimetableBuilder of() { + return new TimetableBuilder(); } /** @return the index of TripTimes for this trip ID in this particular Timetable */ @@ -134,17 +133,6 @@ public TripTimes getTripTimes(FeedScopedId tripId) { return null; } - /** - * Set new trip times for trip given a trip index - * - * @param tripIndex trip index of trip - * @param tt new trip times for trip - * @return old trip times of trip - */ - public TripTimes setTripTimes(int tripIndex, TripTimes tt) { - return tripTimes.set(tripIndex, tt); - } - /** * Apply the TripUpdate to the appropriate TripTimes from this Timetable. The existing TripTimes * must not be modified directly because they may be shared with the underlying @@ -386,44 +374,6 @@ public Result createUpdatedTripTimesFromGTFSRT( return Result.success(new TripTimesPatch(newTimes, skippedStopIndices)); } - /** - * Add a trip to this Timetable. The Timetable must be analyzed, compacted, and indexed any time - * trips are added, but this is not done automatically because it is time consuming and should - * only be done once after an entire batch of trips are added. Note that the trip is not added to - * the enclosing pattern here, but in the pattern's wrapper function. Here we don't know if it's a - * scheduled trip or a realtime-added trip. - */ - public void addTripTimes(TripTimes tt) { - tripTimes.add(tt); - } - - /** - * Apply the same update to all trip-times inculuding scheduled and frequency based - * trip times. - *

- * THIS IS NOT THREAD-SAFE - ONLY USE THIS METHOD DURING GRAPH-BUILD! - */ - public void updateAllTripTimes(UnaryOperator update) { - tripTimes.replaceAll(update); - frequencyEntries.replaceAll(it -> - new FrequencyEntry( - it.startTime, - it.endTime, - it.headway, - it.exactTimes, - update.apply(it.tripTimes) - ) - ); - } - - /** - * Add a frequency entry to this Timetable. See addTripTimes method. Maybe Frequency Entries - * should just be TripTimes for simplicity. - */ - public void addFrequencyEntry(FrequencyEntry freq) { - frequencyEntries.add(freq); - } - public boolean isValidFor(LocalDate serviceDate) { return this.serviceDate == null || this.serviceDate.equals(serviceDate); } @@ -493,4 +443,13 @@ public TripTimes getRepresentativeTripTimes() { return null; } } + + /** + * @return true if the timetable was created by a real-time update, false if this + * timetable is based on scheduled data. + * Only real-time timetables have a service date. + */ + public boolean isCreatedByRealTimeUpdater() { + return serviceDate != null; + } } diff --git a/src/main/java/org/opentripplanner/model/TimetableBuilder.java b/src/main/java/org/opentripplanner/model/TimetableBuilder.java new file mode 100644 index 00000000000..e41507fdc1b --- /dev/null +++ b/src/main/java/org/opentripplanner/model/TimetableBuilder.java @@ -0,0 +1,131 @@ +package org.opentripplanner.model; + +import java.time.LocalDate; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.function.UnaryOperator; +import org.opentripplanner.transit.model.network.TripPattern; +import org.opentripplanner.transit.model.timetable.Direction; +import org.opentripplanner.transit.model.timetable.FrequencyEntry; +import org.opentripplanner.transit.model.timetable.Trip; +import org.opentripplanner.transit.model.timetable.TripTimes; + +public class TimetableBuilder { + + private TripPattern pattern; + private LocalDate serviceDate; + private final List tripTimes = new ArrayList<>(); + private final List frequencies = new ArrayList<>(); + + TimetableBuilder() {} + + TimetableBuilder(Timetable tt) { + pattern = tt.getPattern(); + serviceDate = tt.getServiceDate(); + tripTimes.addAll(tt.getTripTimes()); + frequencies.addAll(tt.getFrequencyEntries()); + } + + public TimetableBuilder withTripPattern(TripPattern tripPattern) { + this.pattern = tripPattern; + return this; + } + + public TimetableBuilder withServiceDate(LocalDate serviceDate) { + this.serviceDate = serviceDate; + return this; + } + + public TimetableBuilder addTripTimes(TripTimes tripTimes) { + this.tripTimes.add(tripTimes); + return this; + } + + public TimetableBuilder addAllTripTimes(List tripTimes) { + this.tripTimes.addAll(tripTimes); + return this; + } + + public TimetableBuilder setTripTimes(int tripIndex, TripTimes tripTimes) { + this.tripTimes.set(tripIndex, tripTimes); + return this; + } + + public TimetableBuilder removeTripTimes(TripTimes tripTimesToRemove) { + tripTimes.remove(tripTimesToRemove); + return this; + } + + public TimetableBuilder removeAllTripTimes(Set tripTimesToBeRemoved) { + tripTimes.removeAll(tripTimesToBeRemoved); + return this; + } + + /** + * Apply the same update to all trip-times including scheduled and frequency based + * trip times. + *

+ */ + public TimetableBuilder updateAllTripTimes(UnaryOperator update) { + tripTimes.replaceAll(update); + frequencies.replaceAll(it -> + new FrequencyEntry( + it.startTime, + it.endTime, + it.headway, + it.exactTimes, + update.apply(it.tripTimes) + ) + ); + return this; + } + + public TimetableBuilder addFrequencyEntry(FrequencyEntry frequencyEntry) { + this.frequencies.add(frequencyEntry); + return this; + } + + public TripPattern getPattern() { + return pattern; + } + + public LocalDate getServiceDate() { + return serviceDate; + } + + public List getTripTimes() { + return tripTimes; + } + + public List getFrequencies() { + return frequencies; + } + + /** + * The direction for all the trips in this timetable. + */ + public Direction getDirection() { + return Optional + .ofNullable(getRepresentativeTripTimes()) + .map(TripTimes::getTrip) + .map(Trip::getDirection) + .orElse(Direction.UNKNOWN); + } + + private TripTimes getRepresentativeTripTimes() { + if (!tripTimes.isEmpty()) { + return tripTimes.getFirst(); + } else if (!frequencies.isEmpty()) { + return frequencies.getFirst().tripTimes; + } else { + // Pattern is created only for real-time updates + return null; + } + } + + public Timetable build() { + return new Timetable(this); + } +} diff --git a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index d076bf9f1f0..64f06362228 100644 --- a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -298,9 +298,7 @@ public Result update(RealTimeTripUpdate realTimeTrip } Timetable tt = resolve(pattern, serviceDate); - // we need to perform the copy of Timetable here rather than in Timetable.update() - // to avoid repeatedly copying in case several updates are applied to the same timetable - tt = copyTimetable(pattern, serviceDate, tt); + TimetableBuilder ttb = Timetable.of(tt).withServiceDate(serviceDate); // Assume all trips in a pattern are from the same feed, which should be the case. // Find trip index @@ -308,12 +306,15 @@ public Result update(RealTimeTripUpdate realTimeTrip int tripIndex = tt.getTripIndex(trip.getId()); if (tripIndex == -1) { // Trip not found, add it - tt.addTripTimes(updatedTripTimes); + ttb.addTripTimes(updatedTripTimes); } else { // Set updated trip times of trip - tt.setTripTimes(tripIndex, updatedTripTimes); + ttb.setTripTimes(tripIndex, updatedTripTimes); } + Timetable updated = ttb.build(); + swapTimetable(pattern, tt, updated); + if (pattern.isCreatedByRealtimeUpdater()) { // Remember this pattern for the added trip id and service date FeedScopedId tripId = trip.getId(); @@ -459,8 +460,11 @@ public boolean revertTripToScheduledTripPattern(FeedScopedId tripId, LocalDate s if (tripTimesToRemove != null) { for (Timetable originalTimetable : sortedTimetables) { if (originalTimetable.getTripTimes().contains(tripTimesToRemove)) { - Timetable updatedTimetable = copyTimetable(pattern, serviceDate, originalTimetable); - updatedTimetable.getTripTimes().remove(tripTimesToRemove); + Timetable updatedTimetable = Timetable + .of(originalTimetable) + .removeTripTimes(tripTimesToRemove) + .build(); + swapTimetable(pattern, originalTimetable, updatedTimetable); } } } @@ -579,36 +583,33 @@ private void addPatternToIndex(TripPattern tripPattern) { } /** - * Make a copy of the given timetable for a given pattern and service date. - * If the timetable was already copied-on write in this snapshot, the same instance will be - * returned. The SortedSet that holds the collection of Timetables for that pattern + * Replace the original Timetable by the updated one in the timetable index. + * The SortedSet that holds the collection of Timetables for that pattern * (sorted by service date) is shared between multiple snapshots and must be copied as well.
* Note on performance: if multiple Timetables are modified in a SortedSet, the SortedSet will be * copied multiple times. The impact on memory/garbage collection is assumed to be minimal * since the collection is small. * The SortedSet is made immutable to prevent change after snapshot publication. */ - private Timetable copyTimetable(TripPattern pattern, LocalDate serviceDate, Timetable tt) { - if (!dirtyTimetables.contains(tt)) { - Timetable old = tt; - tt = new Timetable(tt, serviceDate); - SortedSet sortedTimetables = timetables.get(pattern); - if (sortedTimetables == null) { - sortedTimetables = new TreeSet<>(new SortedTimetableComparator()); - } else { - SortedSet temp = new TreeSet<>(new SortedTimetableComparator()); - temp.addAll(sortedTimetables); - sortedTimetables = temp; - } - if (old.getServiceDate() != null) { - sortedTimetables.remove(old); - } - sortedTimetables.add(tt); - timetables.put(pattern, ImmutableSortedSet.copyOfSorted(sortedTimetables)); - dirtyTimetables.add(tt); - dirty = true; + private void swapTimetable(TripPattern pattern, Timetable original, Timetable updated) { + SortedSet sortedTimetables = timetables.get(pattern); + if (sortedTimetables == null) { + sortedTimetables = new TreeSet<>(new SortedTimetableComparator()); + } else { + SortedSet temp = new TreeSet<>(new SortedTimetableComparator()); + temp.addAll(sortedTimetables); + sortedTimetables = temp; + } + // This is a minor optimization: + // Since sortedTimetables contains only timetables created in real-time, no need to try to + // remove the original if it was not created by real-time. + if (original.isCreatedByRealTimeUpdater()) { + sortedTimetables.remove(original); } - return tt; + sortedTimetables.add(updated); + timetables.put(pattern, ImmutableSortedSet.copyOfSorted(sortedTimetables)); + dirtyTimetables.add(updated); + dirty = true; } protected static class SortedTimetableComparator implements Comparator { diff --git a/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java b/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java index 373b99f0bc6..aa82a66d0b9 100644 --- a/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java +++ b/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java @@ -8,6 +8,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; import org.opentripplanner.ext.flex.trip.FlexTrip; import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore; import org.opentripplanner.gtfs.mapping.StaySeatedNotAllowed; @@ -15,6 +16,7 @@ import org.opentripplanner.model.Frequency; import org.opentripplanner.model.OtpTransitService; import org.opentripplanner.model.ShapePoint; +import org.opentripplanner.model.Timetable; import org.opentripplanner.model.TripStopTimes; import org.opentripplanner.model.calendar.CalendarServiceData; import org.opentripplanner.model.calendar.ServiceCalendar; @@ -51,6 +53,7 @@ import org.opentripplanner.transit.model.site.Station; import org.opentripplanner.transit.model.timetable.Trip; import org.opentripplanner.transit.model.timetable.TripOnServiceDate; +import org.opentripplanner.transit.model.timetable.TripTimes; import org.opentripplanner.transit.service.StopModel; import org.opentripplanner.transit.service.StopModelBuilder; import org.slf4j.Logger; @@ -375,21 +378,39 @@ private void removeStopTimesForNoneExistingTrips() { private void fixOrRemovePatternsWhichReferenceNoneExistingTrips() { int orgSize = tripPatterns.size(); List> removePatterns = new ArrayList<>(); + List updatedPatterns = new ArrayList<>(); for (Map.Entry e : tripPatterns.entries()) { TripPattern ptn = e.getValue(); - ptn.removeTrips(t -> !tripsById.containsKey(t.getId())); - if (ptn.scheduledTripsAsStream().findAny().isEmpty()) { + Set tripTimesToBeRemoved = ptn + .getScheduledTimetable() + .getTripTimes() + .stream() + .filter(tripTimes -> !tripsById.containsKey(tripTimes.getTrip().getId())) + .collect(Collectors.toUnmodifiableSet()); + if (!tripTimesToBeRemoved.isEmpty()) { removePatterns.add(e); + Timetable updatedTimetable = Timetable + .of(ptn.getScheduledTimetable()) + .removeAllTripTimes(tripTimesToBeRemoved) + .build(); + TripPattern updatedPattern = ptn.copy().withScheduledTimeTable(updatedTimetable).build(); + if (!updatedTimetable.getTripTimes().isEmpty()) { + updatedPatterns.add(updatedPattern); + } else { + issueStore.add( + "RemovedEmptyTripPattern", + "Removed trip pattern %s as it contains no trips", + updatedPattern.getId() + ); + } } } for (Map.Entry it : removePatterns) { tripPatterns.remove(it.getKey(), it.getValue()); - issueStore.add( - "RemovedEmptyTripPattern", - "Removed trip pattern %s as it contains no trips", - it.getValue().getId() - ); + } + for (TripPattern tripPattern : updatedPatterns) { + tripPatterns.put(tripPattern.getStopPattern(), tripPattern); } logRemove("TripPattern", orgSize, tripPatterns.size(), "No trips for pattern exist."); } diff --git a/src/main/java/org/opentripplanner/netex/mapping/TripPatternMapper.java b/src/main/java/org/opentripplanner/netex/mapping/TripPatternMapper.java index 740224b4489..c7b85e72af8 100644 --- a/src/main/java/org/opentripplanner/netex/mapping/TripPatternMapper.java +++ b/src/main/java/org/opentripplanner/netex/mapping/TripPatternMapper.java @@ -255,8 +255,10 @@ Optional mapTripPattern(JourneyPattern_VersionStructure .withHopGeometries( serviceLinkMapper.getGeometriesByJourneyPattern(journeyPattern, stopPattern) ) + .withScheduledTimeTableBuilder(builder -> + builder.addAllTripTimes(createTripTimes(trips, tripStopTimes)) + ) .build(); - createTripTimes(trips, tripStopTimes).forEach(tripPattern::add); return Optional.of( new TripPatternMapperResult( diff --git a/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java b/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java index 57c71d06113..8e39d353913 100644 --- a/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java +++ b/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java @@ -8,7 +8,6 @@ import java.util.Collection; import java.util.List; import java.util.Objects; -import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nonnull; @@ -28,7 +27,6 @@ import org.opentripplanner.transit.model.site.Station; import org.opentripplanner.transit.model.site.StopLocation; import org.opentripplanner.transit.model.timetable.Direction; -import org.opentripplanner.transit.model.timetable.FrequencyEntry; import org.opentripplanner.transit.model.timetable.Trip; import org.opentripplanner.transit.model.timetable.TripTimes; import org.slf4j.Logger; @@ -122,20 +120,27 @@ public final class TripPattern private final RoutingTripPattern routingTripPattern; - public TripPattern(TripPatternBuilder builder) { + TripPattern(TripPatternBuilder builder) { super(builder.getId()); this.name = builder.getName(); this.route = builder.getRoute(); this.stopPattern = requireNonNull(builder.getStopPattern()); this.createdByRealtimeUpdater = builder.isCreatedByRealtimeUpdate(); - this.mode = requireNonNullElseGet(builder.getMode(), route::getMode); - this.netexSubMode = requireNonNullElseGet(builder.getNetexSubmode(), route::getNetexSubmode); + this.mode = requireNonNull(builder.getMode()); + this.netexSubMode = requireNonNull(builder.getNetexSubmode()); this.containsMultipleModes = builder.getContainsMultipleModes(); - this.scheduledTimetable = - builder.getScheduledTimetable() != null - ? builder.getScheduledTimetable() - : new Timetable(this); + if (builder.getScheduledTimetable() != null) { + if (builder.getScheduledTimetableBuilder() != null) { + throw new IllegalArgumentException( + "Cannot provide both scheduled timetable and scheduled timetable builder" + ); + } + this.scheduledTimetable = builder.getScheduledTimetable(); + } else { + this.scheduledTimetable = + builder.getScheduledTimetableBuilder().withTripPattern(this).build(); + } this.originalTripPattern = builder.getOriginalTripPattern(); @@ -331,56 +336,6 @@ public boolean isBoardAndAlightAt(int stopIndex, PickDrop value) { /* METHODS THAT DELEGATE TO THE SCHEDULED TIMETABLE */ - // TODO OTP2 this method modifies the state, it will be refactored in a subsequent step - /** - * Add the given tripTimes to this pattern's scheduled timetable, recording the corresponding trip - * as one of the scheduled trips on this pattern. - */ - public void add(TripTimes tt) { - // Only scheduled trips (added at graph build time, rather than directly to the timetable - // via updates) are in this list. - scheduledTimetable.addTripTimes(tt); - - // Check that all trips added to this pattern are on the initially declared route. - // Identity equality is valid on GTFS entity objects. - if (this.route != tt.getTrip().getRoute()) { - LOG.warn( - "The trip {} is on route {} but its stop pattern is on route {}.", - tt.getTrip(), - tt.getTrip().getRoute(), - route - ); - } - } - - // TODO OTP2 this method modifies the state, it will be refactored in a subsequent step - /** - * Add the given FrequencyEntry to this pattern's scheduled timetable, recording the corresponding - * trip as one of the scheduled trips on this pattern. - * TODO possible improvements: combine freq entries and TripTimes. Do not keep trips list in TripPattern - * since it is redundant. - */ - public void add(FrequencyEntry freq) { - scheduledTimetable.addFrequencyEntry(freq); - if (this.getRoute() != freq.tripTimes.getTrip().getRoute()) { - LOG.warn( - "The trip {} is on a different route than its stop pattern, which is on {}.", - freq.tripTimes.getTrip(), - route - ); - } - } - - // TODO OTP2 this method modifies the state, it will be refactored in a subsequent step - /** - * Remove all trips matching the given predicate. - * - * @param removeTrip it the predicate returns true - */ - public void removeTrips(Predicate removeTrip) { - scheduledTimetable.getTripTimes().removeIf(tt -> removeTrip.test(tt.getTrip())); - } - /** * Checks that this is TripPattern is based of the provided TripPattern and contains same stops * (but not necessarily with same pickup and dropoff values). diff --git a/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java b/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java index f34d206922b..91451d43cc2 100644 --- a/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java +++ b/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java @@ -1,13 +1,17 @@ package org.opentripplanner.transit.model.network; +import static java.util.Objects.requireNonNullElseGet; + import java.util.ArrayList; import java.util.List; +import java.util.function.UnaryOperator; import java.util.stream.IntStream; import org.locationtech.jts.geom.Coordinate; import org.locationtech.jts.geom.LineString; import org.opentripplanner.framework.geometry.CompactLineStringUtils; import org.opentripplanner.framework.geometry.GeometryUtils; import org.opentripplanner.model.Timetable; +import org.opentripplanner.model.TimetableBuilder; import org.opentripplanner.routing.algorithm.raptoradapter.api.SlackProvider; import org.opentripplanner.transit.model.basic.SubMode; import org.opentripplanner.transit.model.basic.TransitMode; @@ -24,6 +28,7 @@ public final class TripPatternBuilder private boolean containsMultipleModes; private StopPattern stopPattern; private Timetable scheduledTimetable; + private TimetableBuilder scheduledTimetableBuilder; private String name; private boolean createdByRealtimeUpdate; @@ -33,6 +38,7 @@ public final class TripPatternBuilder TripPatternBuilder(FeedScopedId id) { super(id); + this.scheduledTimetableBuilder = Timetable.of(); } TripPatternBuilder(TripPattern original) { @@ -86,10 +92,28 @@ public TripPatternBuilder withStopPattern(StopPattern stopPattern) { } public TripPatternBuilder withScheduledTimeTable(Timetable scheduledTimetable) { + if (scheduledTimetableBuilder != null) { + throw new IllegalStateException( + "Cannot set scheduled Timetable after scheduled Timetable builder is created" + ); + } this.scheduledTimetable = scheduledTimetable; return this; } + public TripPatternBuilder withScheduledTimeTableBuilder( + UnaryOperator producer + ) { + // create a builder for the scheduled timetable only if it needs to be modified. + // otherwise reuse the existing timetable + if (scheduledTimetableBuilder == null) { + scheduledTimetableBuilder = Timetable.of(scheduledTimetable); + scheduledTimetable = null; + } + producer.apply(scheduledTimetableBuilder); + return this; + } + public TripPatternBuilder withCreatedByRealtimeUpdater(boolean createdByRealtimeUpdate) { this.createdByRealtimeUpdate = createdByRealtimeUpdate; return this; @@ -115,6 +139,13 @@ public int transitReluctanceFactorIndex() { return route.getMode().ordinal(); } + public Object getDirection() { + if (scheduledTimetable != null) { + return scheduledTimetable.getDirection(); + } + return scheduledTimetableBuilder.getDirection(); + } + @Override protected TripPattern buildFromValues() { return new TripPattern(this); @@ -125,11 +156,11 @@ public Route getRoute() { } public TransitMode getMode() { - return mode; + return mode != null ? mode : route.getMode(); } public SubMode getNetexSubmode() { - return netexSubMode; + return netexSubMode != null ? netexSubMode : route.getNetexSubmode(); } public boolean getContainsMultipleModes() { @@ -144,6 +175,10 @@ public Timetable getScheduledTimetable() { return scheduledTimetable; } + public TimetableBuilder getScheduledTimetableBuilder() { + return scheduledTimetableBuilder; + } + public String getName() { return name; } diff --git a/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java b/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java index 79590ca2775..1f43b155a17 100644 --- a/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java +++ b/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java @@ -160,11 +160,13 @@ static void setup() { var model = stopModel.build(); var transitModel = new TransitModel(model, DEDUPLICATOR); - final TripPattern pattern = TEST_MODEL.pattern(BUS).build(); var trip = TransitModelForTest.trip("123").withHeadsign(I18NString.of("Trip Headsign")).build(); var stopTimes = TEST_MODEL.stopTimesEvery5Minutes(3, trip, T11_00); var tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, DEDUPLICATOR); - pattern.add(tripTimes); + final TripPattern pattern = TEST_MODEL + .pattern(BUS) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) + .build(); transitModel.addTripPattern(id("pattern-1"), pattern); diff --git a/src/test/java/org/opentripplanner/apis/gtfs/PatternByServiceDatesFilterTest.java b/src/test/java/org/opentripplanner/apis/gtfs/PatternByServiceDatesFilterTest.java index f01bac12006..ecea70e08d4 100644 --- a/src/test/java/org/opentripplanner/apis/gtfs/PatternByServiceDatesFilterTest.java +++ b/src/test/java/org/opentripplanner/apis/gtfs/PatternByServiceDatesFilterTest.java @@ -44,19 +44,18 @@ enum FilterExpectation { } private static TripPattern pattern() { - var pattern = TransitModelForTest - .tripPattern("1", ROUTE_1) - .withStopPattern(STOP_PATTERN) - .build(); - var tt = ScheduledTripTimes .of() .withTrip(TRIP) .withArrivalTimes("10:00 10:05") .withDepartureTimes("10:00 10:05") .build(); - pattern.add(tt); - return pattern; + + return TransitModelForTest + .tripPattern("1", ROUTE_1) + .withStopPattern(STOP_PATTERN) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tt)) + .build(); } static List invalidRangeCases() { diff --git a/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java b/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java new file mode 100644 index 00000000000..0451edbb40d --- /dev/null +++ b/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java @@ -0,0 +1,302 @@ +package org.opentripplanner.gtfs; + +import static org.opentripplanner.transit.model._data.TransitModelForTest.trip; + +import java.util.Collection; +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore; +import org.opentripplanner.graph_builder.issue.service.DefaultDataImportIssueStore; +import org.opentripplanner.graph_builder.issues.TripDegenerate; +import org.opentripplanner.graph_builder.issues.TripUndefinedService; +import org.opentripplanner.graph_builder.module.geometry.GeometryProcessor; +import org.opentripplanner.model.StopTime; +import org.opentripplanner.model.impl.OtpTransitServiceBuilder; +import org.opentripplanner.transit.model._data.TransitModelForTest; +import org.opentripplanner.transit.model.basic.TransitMode; +import org.opentripplanner.transit.model.framework.Deduplicator; +import org.opentripplanner.transit.model.framework.FeedScopedId; +import org.opentripplanner.transit.model.site.RegularStop; +import org.opentripplanner.transit.model.timetable.Direction; +import org.opentripplanner.transit.model.timetable.Trip; +import org.opentripplanner.transit.service.StopModel; + +class GenerateTripPatternsOperationTest { + + private static StopModel stopModel; + private static RegularStop stopA; + private static RegularStop stopB; + private static RegularStop stopC; + private static Trip trip1; + private static Trip trip2; + private static Trip trip3; + private static Trip trip4; + private static Trip trip5; + private static StopTime stopTimeA; + private static StopTime stopTimeB; + private static StopTime stopTimeC; + + private Deduplicator deduplicator; + private DataImportIssueStore issueStore; + private OtpTransitServiceBuilder transitServiceBuilder; + private GeometryProcessor geometryProcessor; + + @BeforeAll + static void setupClass() { + TransitModelForTest transitModelForTest = TransitModelForTest.of(); + stopA = transitModelForTest.stop("stopA").build(); + stopB = transitModelForTest.stop("stopB").build(); + stopC = transitModelForTest.stop("stopC").build(); + stopModel = + transitModelForTest + .stopModelBuilder() + .withRegularStop(stopA) + .withRegularStop(stopB) + .withRegularStop(stopC) + .build(); + + stopTimeA = new StopTime(); + stopTimeA.setStop(stopA); + stopTimeB = new StopTime(); + stopTimeB.setStop(stopB); + stopTimeC = new StopTime(); + stopTimeC.setStop(stopC); + + FeedScopedId serviceId1 = TransitModelForTest.id("SERVICE_ID_1"); + trip1 = + trip("TRIP_ID_1") + .withServiceId(serviceId1) + .withMode(TransitMode.RAIL) + .withNetexSubmode("SUBMODE_1") + .withDirection(Direction.INBOUND) + .build(); + + // same route, mode, submode and direction as trip1 + FeedScopedId serviceId2 = TransitModelForTest.id("SERVICE_ID_2"); + trip2 = + trip("TRIP_ID_2") + .withServiceId(serviceId2) + .withRoute(trip1.getRoute()) + .withMode(trip1.getMode()) + .withNetexSubmode(trip1.getNetexSubMode().name()) + .withDirection(trip1.getDirection()) + .build(); + + // same route, direction as trip1, different mode + FeedScopedId serviceId3 = TransitModelForTest.id("SERVICE_ID_3"); + trip3 = + trip("TRIP_ID_3") + .withServiceId(serviceId3) + .withRoute(trip1.getRoute()) + .withMode(TransitMode.BUS) + .withDirection(trip1.getDirection()) + .build(); + + // same route, mode, direction as trip1, different submode + FeedScopedId serviceId4 = TransitModelForTest.id("SERVICE_ID_4"); + trip4 = + trip("TRIP_ID_4") + .withServiceId(serviceId4) + .withRoute(trip1.getRoute()) + .withMode(trip1.getMode()) + .withNetexSubmode("SUMODE_2") + .withDirection(trip1.getDirection()) + .build(); + + // same route, mode as trip1, different direction + FeedScopedId serviceId5 = TransitModelForTest.id("SERVICE_ID_5"); + trip5 = + trip("TRIP_ID_5") + .withServiceId(serviceId5) + .withRoute(trip1.getRoute()) + .withMode(trip1.getMode()) + .withNetexSubmode(trip1.getNetexSubMode().name()) + .withDirection(Direction.OUTBOUND) + .build(); + } + + @BeforeEach + void setup() { + deduplicator = new Deduplicator(); + issueStore = new DefaultDataImportIssueStore(); + transitServiceBuilder = new OtpTransitServiceBuilder(stopModel, issueStore); + double maxStopToShapeSnapDistance = 100; + geometryProcessor = + new GeometryProcessor(transitServiceBuilder, maxStopToShapeSnapDistance, issueStore); + } + + @Test + void testGenerateTripPatternsNoTrip() { + Set calendarServiceIds = Set.of(); + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertTrue(transitServiceBuilder.getTripPatterns().isEmpty()); + Assertions.assertTrue(issueStore.listIssues().isEmpty()); + } + + @Test + void testGenerateTripPatternsTripWithUndefinedService() { + transitServiceBuilder.getTripsById().computeIfAbsent(trip1.getId(), feedScopedId -> trip1); + Set calendarServiceIds = Set.of(); + + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertTrue(transitServiceBuilder.getTripPatterns().isEmpty()); + Assertions.assertFalse(issueStore.listIssues().isEmpty()); + Assertions.assertInstanceOf(TripUndefinedService.class, issueStore.listIssues().getFirst()); + } + + @Test + void testGenerateTripPatternsDegeneratedTrip() { + transitServiceBuilder.getTripsById().computeIfAbsent(trip1.getId(), feedScopedId -> trip1); + Set calendarServiceIds = Set.of(trip1.getServiceId()); + + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertTrue(transitServiceBuilder.getTripPatterns().isEmpty()); + Assertions.assertFalse(issueStore.listIssues().isEmpty()); + Assertions.assertInstanceOf(TripDegenerate.class, issueStore.listIssues().getFirst()); + } + + @Test + void testGenerateTripPatterns() { + transitServiceBuilder.getTripsById().computeIfAbsent(trip1.getId(), feedScopedId -> trip1); + Collection stopTimes = List.of(stopTimeA, stopTimeB); + transitServiceBuilder.getStopTimesSortedByTrip().put(trip1, stopTimes); + Set calendarServiceIds = Set.of(trip1.getServiceId()); + + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertEquals(1, transitServiceBuilder.getTripPatterns().size()); + Assertions.assertTrue(issueStore.listIssues().isEmpty()); + } + + @Test + void testGenerateTripPatterns2TripsSameStops() { + transitServiceBuilder.getTripsById().computeIfAbsent(trip1.getId(), feedScopedId -> trip1); + transitServiceBuilder.getTripsById().computeIfAbsent(trip2.getId(), feedScopedId -> trip2); + Collection stopTimes = List.of(stopTimeA, stopTimeB); + + transitServiceBuilder.getStopTimesSortedByTrip().put(trip1, stopTimes); + transitServiceBuilder.getStopTimesSortedByTrip().put(trip2, stopTimes); + + Set calendarServiceIds = Set.of(trip1.getServiceId(), trip2.getServiceId()); + + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertEquals(1, transitServiceBuilder.getTripPatterns().size()); + Assertions.assertEquals( + 2, + transitServiceBuilder + .getTripPatterns() + .values() + .stream() + .findFirst() + .orElseThrow() + .getScheduledTimetable() + .getTripTimes() + .size() + ); + Assertions.assertTrue(issueStore.listIssues().isEmpty()); + } + + @Test + void testGenerateTripPatterns2TripsDifferentStops() { + transitServiceBuilder.getTripsById().computeIfAbsent(trip1.getId(), feedScopedId -> trip1); + transitServiceBuilder.getTripsById().computeIfAbsent(trip2.getId(), feedScopedId -> trip2); + Collection stopTimesTrip1 = List.of(stopTimeA, stopTimeB); + Collection stopTimesTrip2 = List.of(stopTimeA, stopTimeC); + + transitServiceBuilder.getStopTimesSortedByTrip().put(trip1, stopTimesTrip1); + transitServiceBuilder.getStopTimesSortedByTrip().put(trip2, stopTimesTrip2); + + Set calendarServiceIds = Set.of(trip1.getServiceId(), trip2.getServiceId()); + + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertEquals(2, transitServiceBuilder.getTripPatterns().size()); + Assertions.assertTrue(issueStore.listIssues().isEmpty()); + } + + static List testCases() { + return List.of( + Arguments.of(trip1, trip3), + Arguments.of(trip1, trip4), + Arguments.of(trip1, trip5) + ); + } + + @ParameterizedTest + @MethodSource("testCases") + void testGenerateDifferentTripPatterns(Trip t1, Trip t2) { + transitServiceBuilder.getTripsById().computeIfAbsent(t1.getId(), feedScopedId -> t1); + transitServiceBuilder.getTripsById().computeIfAbsent(t2.getId(), feedScopedId -> t2); + Collection stopTimes = List.of(stopTimeA, stopTimeB); + + transitServiceBuilder.getStopTimesSortedByTrip().put(t1, stopTimes); + transitServiceBuilder.getStopTimesSortedByTrip().put(t2, stopTimes); + + Set calendarServiceIds = Set.of(t1.getServiceId(), t2.getServiceId()); + + GenerateTripPatternsOperation generateTripPatternsOperation = new GenerateTripPatternsOperation( + transitServiceBuilder, + issueStore, + deduplicator, + calendarServiceIds, + geometryProcessor + ); + generateTripPatternsOperation.run(); + + Assertions.assertEquals(2, transitServiceBuilder.getTripPatterns().size()); + Assertions.assertTrue(issueStore.listIssues().isEmpty()); + } +} diff --git a/src/test/java/org/opentripplanner/gtfs/interlining/InterlineProcessorTest.java b/src/test/java/org/opentripplanner/gtfs/interlining/InterlineProcessorTest.java index a1aa4f9d753..e14a33cd276 100644 --- a/src/test/java/org/opentripplanner/gtfs/interlining/InterlineProcessorTest.java +++ b/src/test/java/org/opentripplanner/gtfs/interlining/InterlineProcessorTest.java @@ -163,13 +163,12 @@ private static TripPattern tripPattern(String tripId, String blockId, String ser ); var stopPattern = new StopPattern(stopTimes); - var tp = TripPattern + var tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, new Deduplicator()); + return TripPattern .of(TransitModelForTest.id(tripId)) .withRoute(trip.getRoute()) .withStopPattern(stopPattern) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) .build(); - var tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, new Deduplicator()); - tp.add(tripTimes); - return tp; } } diff --git a/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index 0a737630a5b..57bf11dd8f6 100644 --- a/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; @@ -52,9 +53,9 @@ public static void setUp() throws Exception { @Test public void testCompare() { - Timetable orig = new Timetable(null); - Timetable a = new Timetable(orig, LocalDate.now(timeZone).minusDays(1)); - Timetable b = new Timetable(orig, LocalDate.now(timeZone)); + Timetable orig = Timetable.of().build(); + Timetable a = Timetable.of(orig).withServiceDate(LocalDate.now(timeZone).minusDays(1)).build(); + Timetable b = Timetable.of(orig).withServiceDate(LocalDate.now(timeZone)).build(); assertTrue(new TimetableSnapshot.SortedTimetableComparator().compare(a, b) < 0); } @@ -106,52 +107,50 @@ public void testResolve() { @Test public void testUpdate() { - assertThrows( - ConcurrentModificationException.class, - () -> { - LocalDate today = LocalDate.now(timeZone); - LocalDate yesterday = today.minusDays(1); - TripPattern pattern = patternIndex.get(new FeedScopedId(feedId, "1.1")); + LocalDate today = LocalDate.now(timeZone); + LocalDate yesterday = today.minusDays(1); + TripPattern pattern = patternIndex.get(new FeedScopedId(feedId, "1.1")); - TimetableSnapshot resolver = new TimetableSnapshot(); - Timetable origNow = resolver.resolve(pattern, today); + TimetableSnapshot resolver = new TimetableSnapshot(); + Timetable origNow = resolver.resolve(pattern, today); - TripDescriptor.Builder tripDescriptorBuilder = TripDescriptor.newBuilder(); + TripDescriptor.Builder tripDescriptorBuilder = TripDescriptor.newBuilder(); - tripDescriptorBuilder.setTripId("1.1"); - tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.SCHEDULED); + tripDescriptorBuilder.setTripId("1.1"); + tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.SCHEDULED); - TripUpdate.Builder tripUpdateBuilder = TripUpdate.newBuilder(); + TripUpdate.Builder tripUpdateBuilder = TripUpdate.newBuilder(); - tripUpdateBuilder.setTrip(tripDescriptorBuilder); + tripUpdateBuilder.setTrip(tripDescriptorBuilder); - var stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(0); - stopTimeUpdateBuilder.setStopSequence(2); - stopTimeUpdateBuilder.setScheduleRelationship( - TripUpdate.StopTimeUpdate.ScheduleRelationship.SCHEDULED - ); - stopTimeUpdateBuilder.setDeparture( - TripUpdate.StopTimeEvent.newBuilder().setDelay(5).build() - ); + var stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(0); + stopTimeUpdateBuilder.setStopSequence(2); + stopTimeUpdateBuilder.setScheduleRelationship( + TripUpdate.StopTimeUpdate.ScheduleRelationship.SCHEDULED + ); + stopTimeUpdateBuilder.setDeparture(TripUpdate.StopTimeEvent.newBuilder().setDelay(5).build()); - TripUpdate tripUpdate = tripUpdateBuilder.build(); + TripUpdate tripUpdate = tripUpdateBuilder.build(); - // new timetable for today - updateResolver(resolver, pattern, tripUpdate, today); - Timetable updatedNow = resolver.resolve(pattern, today); - assertNotSame(origNow, updatedNow); + // new timetable for today + updateResolver(resolver, pattern, tripUpdate, today); + Timetable updatedNow = resolver.resolve(pattern, today); + assertNotSame(origNow, updatedNow); - // reuse timetable for today - updateResolver(resolver, pattern, tripUpdate, today); - assertEquals(updatedNow, resolver.resolve(pattern, today)); + // a new timetable instance is created for today + updateResolver(resolver, pattern, tripUpdate, today); + assertNotEquals(updatedNow, resolver.resolve(pattern, today)); - // create new timetable for tomorrow - updateResolver(resolver, pattern, tripUpdate, yesterday); - assertNotSame(origNow, resolver.resolve(pattern, yesterday)); - assertNotSame(updatedNow, resolver.resolve(pattern, yesterday)); + // create new timetable for tomorrow + updateResolver(resolver, pattern, tripUpdate, yesterday); + assertNotSame(origNow, resolver.resolve(pattern, yesterday)); + assertNotSame(updatedNow, resolver.resolve(pattern, yesterday)); - // exception if we try to modify a snapshot - TimetableSnapshot snapshot = resolver.commit(); + // exception if we try to modify a snapshot + TimetableSnapshot snapshot = resolver.commit(); + assertThrows( + ConcurrentModificationException.class, + () -> { updateResolver(snapshot, pattern, tripUpdate, yesterday); } ); diff --git a/src/test/java/org/opentripplanner/model/TimetableTest.java b/src/test/java/org/opentripplanner/model/TimetableTest.java index 9e6a7467dc5..75a3af3f3ca 100644 --- a/src/test/java/org/opentripplanner/model/TimetableTest.java +++ b/src/test/java/org/opentripplanner/model/TimetableTest.java @@ -190,7 +190,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable.setTripTimes(trip_1_1_index, updatedTripTimes); + timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); assertEquals(20 * 60 + 120, timetable.getTripTimes(trip_1_1_index).getArrivalTime(2)); }); @@ -217,7 +217,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable.setTripTimes(trip_1_1_index, updatedTripTimes); + timetable = timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); }); // update trip arrival time only @@ -246,7 +246,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable.setTripTimes(trip_1_1_index, updatedTripTimes); + timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); }); // update trip departure time only @@ -273,7 +273,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable.setTripTimes(trip_1_1_index, updatedTripTimes); + timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); }); // update trip using stop id @@ -299,7 +299,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable.setTripTimes(trip_1_1_index, updatedTripTimes); + timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); }); } diff --git a/src/test/java/org/opentripplanner/model/impl/OtpTransitServiceBuilderLimitPeriodTest.java b/src/test/java/org/opentripplanner/model/impl/OtpTransitServiceBuilderLimitPeriodTest.java index 5d1fef21271..27e6e56c645 100644 --- a/src/test/java/org/opentripplanner/model/impl/OtpTransitServiceBuilderLimitPeriodTest.java +++ b/src/test/java/org/opentripplanner/model/impl/OtpTransitServiceBuilderLimitPeriodTest.java @@ -1,6 +1,7 @@ package org.opentripplanner.model.impl; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.LocalDate; @@ -22,6 +23,7 @@ import org.opentripplanner.transit.model.network.Route; import org.opentripplanner.transit.model.network.StopPattern; import org.opentripplanner.transit.model.network.TripPattern; +import org.opentripplanner.transit.model.network.TripPatternBuilder; import org.opentripplanner.transit.model.site.RegularStop; import org.opentripplanner.transit.model.timetable.Direction; import org.opentripplanner.transit.model.timetable.Trip; @@ -141,16 +143,27 @@ public void testLimitPeriod() { assertTrue(patterns.contains(patternInT1), patterns.toString()); assertTrue(patterns.contains(patternInT2), patterns.toString()); - // Verify trips in pattern (one trip is removed from patternInT1) - assertEquals(1, patternInT1.scheduledTripsAsStream().count()); - assertEquals(tripCSIn, patternInT1.scheduledTripsAsStream().findFirst().orElseThrow()); - - // Verify trips in pattern is unchanged (one trip) + // Verify patternInT1 is replaced by a copy that contains one less trip + TripPattern copyOfTripPattern1 = subject + .getTripPatterns() + .values() + .stream() + .filter(p -> p.getId().equals(patternInT1.getId())) + .findFirst() + .orElseThrow(); + assertNotSame(patternInT1, copyOfTripPattern1); + assertEquals(1, copyOfTripPattern1.scheduledTripsAsStream().count()); + assertEquals(tripCSIn, copyOfTripPattern1.scheduledTripsAsStream().findFirst().orElseThrow()); + + // Verify trips in patternInT2 is unchanged (one trip) assertEquals(1, patternInT2.scheduledTripsAsStream().count()); - // Verify scheduledTimetable trips (one trip is removed from patternInT1) - assertEquals(1, patternInT1.getScheduledTimetable().getTripTimes().size()); - assertEquals(tripCSIn, patternInT1.getScheduledTimetable().getTripTimes().get(0).getTrip()); + // Verify scheduledTimetable trips (one trip is removed from the copy of patternInT1) + assertEquals(1, copyOfTripPattern1.getScheduledTimetable().getTripTimes().size()); + assertEquals( + tripCSIn, + copyOfTripPattern1.getScheduledTimetable().getTripTimes().get(0).getTrip() + ); // Verify scheduledTimetable trips in pattern is unchanged (one trip) assertEquals(1, patternInT2.getScheduledTimetable().getTripTimes().size()); @@ -186,16 +199,17 @@ private TripPattern createTripPattern(Collection trips) { FeedScopedId patternId = TransitModelForTest.id( trips.stream().map(t -> t.getId().getId()).collect(Collectors.joining(":")) ); - TripPattern p = TripPattern + TripPatternBuilder tpb = TripPattern .of(patternId) .withRoute(route) - .withStopPattern(STOP_PATTERN) - .build(); + .withStopPattern(STOP_PATTERN); for (Trip trip : trips) { - p.add(TripTimesFactory.tripTimes(trip, STOP_TIMES, DEDUPLICATOR)); + tpb.withScheduledTimeTableBuilder(builder -> + builder.addTripTimes(TripTimesFactory.tripTimes(trip, STOP_TIMES, DEDUPLICATOR)) + ); } - return p; + return tpb.build(); } private Trip createTrip(String id, FeedScopedId serviceId) { diff --git a/src/test/java/org/opentripplanner/model/plan/TestItineraryBuilder.java b/src/test/java/org/opentripplanner/model/plan/TestItineraryBuilder.java index edaafabd753..ec04d9f6d96 100644 --- a/src/test/java/org/opentripplanner/model/plan/TestItineraryBuilder.java +++ b/src/test/java/org/opentripplanner/model/plan/TestItineraryBuilder.java @@ -482,13 +482,13 @@ public TestItineraryBuilder transit( stopTimes.add(toStopTime); StopPattern stopPattern = new StopPattern(stopTimes); + final TripTimes tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, new Deduplicator()); TripPattern tripPattern = TripPattern .of(route.getId()) .withRoute(route) .withStopPattern(stopPattern) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) .build(); - final TripTimes tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, new Deduplicator()); - tripPattern.add(tripTimes); ScheduledTransitLeg leg; diff --git a/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java b/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java index 480639e5bda..315b12dfefc 100644 --- a/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java +++ b/src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java @@ -11,7 +11,6 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore; -import org.opentripplanner.model.Timetable; import org.opentripplanner.model.calendar.CalendarServiceData; import org.opentripplanner.model.plan.PlanTestConstants; import org.opentripplanner.model.plan.ScheduledTransitLeg; @@ -54,23 +53,23 @@ static void buildTransitService() { stop4 = TEST_MODEL.stop("STOP4", 0, 0).withParentStation(parentStation).build(); // build transit data + Trip trip = TransitModelForTest.trip("1").build(); + var tripTimes = TripTimesFactory.tripTimes( + trip, + TEST_MODEL.stopTimesEvery5Minutes(5, trip, PlanTestConstants.T11_00), + new Deduplicator() + ); + tripTimes.setServiceCode(SERVICE_CODE); TripPattern tripPattern = TransitModelForTest .tripPattern("1", TransitModelForTest.route(id("1")).build()) .withStopPattern(TransitModelForTest.stopPattern(stop1, stop2, stop3)) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) .build(); - Timetable timetable = tripPattern.getScheduledTimetable(); - Trip trip = TransitModelForTest.trip("1").build(); + tripId = trip.getId(); stopIdAtPosition0 = tripPattern.getStop(0).getId(); stopIdAtPosition1 = tripPattern.getStop(1).getId(); stopIdAtPosition2 = tripPattern.getStop(2).getId(); - var tripTimes = TripTimesFactory.tripTimes( - trip, - TEST_MODEL.stopTimesEvery5Minutes(5, trip, PlanTestConstants.T11_00), - new Deduplicator() - ); - tripTimes.setServiceCode(SERVICE_CODE); - timetable.addTripTimes(tripTimes); // build transit model StopModel stopModel = TEST_MODEL diff --git a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapperTest.java b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapperTest.java index 020dc6305a4..27f80062555 100644 --- a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapperTest.java +++ b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapperTest.java @@ -36,7 +36,6 @@ public class TripPatternForDateMapperTest { @BeforeAll public static void setUp() throws Exception { var pattern = TEST_MODEL.pattern(BUS).build(); - timetable = new Timetable(pattern); var trip = TransitModelForTest.trip("1").build(); var tripTimes = TripTimesFactory.tripTimes( trip, @@ -44,7 +43,7 @@ public static void setUp() throws Exception { new Deduplicator() ); tripTimes.setServiceCode(SERVICE_CODE); - timetable.addTripTimes(tripTimes); + timetable = Timetable.of().withTripPattern(pattern).addTripTimes(tripTimes).build(); } /** diff --git a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/request/TestRouteData.java b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/request/TestRouteData.java index f57f03705fd..8648ea20324 100644 --- a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/request/TestRouteData.java +++ b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/request/TestRouteData.java @@ -62,8 +62,8 @@ public TestRouteData(Route route, List stops, List times) { .of(TransitModelForTest.id("TP:" + route)) .withRoute(this.route) .withStopPattern(new StopPattern(stopTimesFistTrip)) + .withScheduledTimeTableBuilder(builder -> builder.addAllTripTimes(tripTimes)) .build(); - tripTimes.forEach(tripPattern::add); RoutingTripPattern routingTripPattern = tripPattern.getRoutingTripPattern(); diff --git a/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java b/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java index 1432c68fd49..d9377ba2c4f 100644 --- a/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java +++ b/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java @@ -33,14 +33,21 @@ public static void setUp() throws Exception { transitService = new DefaultTransitService(transitModel); feedId = transitModel.getFeedIds().iterator().next(); stopId = new FeedScopedId(feedId, "J"); - pattern = - transitService.getPatternForTrip( - transitService.getTripForId(new FeedScopedId(feedId, "5.1")) - ); - var tt = transitService.getTimetableForTripPattern(pattern, LocalDate.now()); + var originalPattern = transitService.getPatternForTrip( + transitService.getTripForId(new FeedScopedId(feedId, "5.1")) + ); + var tt = originalPattern.getScheduledTimetable(); var newTripTimes = tt.getTripTimes(0).copyScheduledTimes(); newTripTimes.cancelTrip(); - tt.setTripTimes(0, newTripTimes); + pattern = + originalPattern + .copy() + .withScheduledTimeTableBuilder(builder -> builder.setTripTimes(0, newTripTimes)) + .build(); + // replace the original pattern by the updated pattern in the transit model + transitModel.addTripPattern(pattern.getId(), pattern); + transitModel.index(); + transitService = new DefaultTransitService(transitModel); } /** diff --git a/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java b/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java index a40bd8bd797..fa89ab3e26e 100644 --- a/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java +++ b/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java @@ -355,8 +355,8 @@ private Trip createTrip(String id, Route route, List stops) { final TripPattern pattern = TransitModelForTest .tripPattern(id + "Pattern", route) .withStopPattern(TransitModelForTest.stopPattern(stops.stream().map(StopCall::stop).toList())) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) .build(); - pattern.add(tripTimes); transitModel.addTripPattern(pattern.getId(), pattern); diff --git a/src/test/java/org/opentripplanner/updater/vehicle_position/RealtimeVehicleMatcherTest.java b/src/test/java/org/opentripplanner/updater/vehicle_position/RealtimeVehicleMatcherTest.java index ea5d86cd0e1..4f10fb2d64b 100644 --- a/src/test/java/org/opentripplanner/updater/vehicle_position/RealtimeVehicleMatcherTest.java +++ b/src/test/java/org/opentripplanner/updater/vehicle_position/RealtimeVehicleMatcherTest.java @@ -383,10 +383,10 @@ private static TripPattern tripPattern(Trip trip, List stopTimes) { .of(trip.getId()) .withStopPattern(stopPattern) .withRoute(ROUTE) + .withScheduledTimeTableBuilder(builder -> + builder.addTripTimes(TripTimesFactory.tripTimes(trip, stopTimes, new Deduplicator())) + ) .build(); - pattern - .getScheduledTimetable() - .addTripTimes(TripTimesFactory.tripTimes(trip, stopTimes, new Deduplicator())); return pattern; } From 307a745692e20311758758b227e268d89354f837 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Thu, 22 Aug 2024 19:43:18 +0200 Subject: [PATCH 02/10] refactor: Encapsulate ordering trip-times in Timetable All trip-times in a Timetable should be ordered by the first departure-time for the first stop in the trip pattern. This was not enforced in the Timetable <-> TimetableBuilder, but relied on the "outside" to do it. This PR enforces this and provides two methods for adding trip-times to a Timetable (add & addOrUpdate). --- .../org/opentripplanner/model/Timetable.java | 4 +- .../model/TimetableBuilder.java | 54 +++++++++++++------ .../model/TimetableSnapshot.java | 12 +---- .../opentripplanner/model/TimetableTest.java | 10 ++-- .../stoptimes/StopTimesHelperTest.java | 2 +- .../speed_test/SpeedIntegrationTest.java | 6 +-- 6 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/opentripplanner/model/Timetable.java b/src/main/java/org/opentripplanner/model/Timetable.java index 5e2e02c3ffd..7cb747b2c90 100644 --- a/src/main/java/org/opentripplanner/model/Timetable.java +++ b/src/main/java/org/opentripplanner/model/Timetable.java @@ -64,8 +64,8 @@ public class Timetable implements Serializable { Timetable(TimetableBuilder timetableBuilder) { this.pattern = timetableBuilder.getPattern(); this.serviceDate = timetableBuilder.getServiceDate(); - tripTimes = List.copyOf(timetableBuilder.getTripTimes()); - frequencyEntries = List.copyOf(timetableBuilder.getFrequencies()); + this.tripTimes = timetableBuilder.createImmutableOrderedListOfTripTimes(); + this.frequencyEntries = List.copyOf(timetableBuilder.getFrequencies()); } /** diff --git a/src/main/java/org/opentripplanner/model/TimetableBuilder.java b/src/main/java/org/opentripplanner/model/TimetableBuilder.java index e41507fdc1b..bd21856eecb 100644 --- a/src/main/java/org/opentripplanner/model/TimetableBuilder.java +++ b/src/main/java/org/opentripplanner/model/TimetableBuilder.java @@ -2,10 +2,13 @@ import java.time.LocalDate; import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; -import java.util.Set; import java.util.function.UnaryOperator; +import org.opentripplanner.transit.model.framework.FeedScopedId; import org.opentripplanner.transit.model.network.TripPattern; import org.opentripplanner.transit.model.timetable.Direction; import org.opentripplanner.transit.model.timetable.FrequencyEntry; @@ -16,7 +19,7 @@ public class TimetableBuilder { private TripPattern pattern; private LocalDate serviceDate; - private final List tripTimes = new ArrayList<>(); + private final Map tripTimes = new HashMap<>(); private final List frequencies = new ArrayList<>(); TimetableBuilder() {} @@ -24,8 +27,8 @@ public class TimetableBuilder { TimetableBuilder(Timetable tt) { pattern = tt.getPattern(); serviceDate = tt.getServiceDate(); - tripTimes.addAll(tt.getTripTimes()); frequencies.addAll(tt.getFrequencyEntries()); + addAllTripTimes(tt.getTripTimes()); } public TimetableBuilder withTripPattern(TripPattern tripPattern) { @@ -38,28 +41,47 @@ public TimetableBuilder withServiceDate(LocalDate serviceDate) { return this; } + /** + * Add a new trip-times to the timetable. If the associated trip already exists, an exception is + * thrown. This is considered a programming error. Use {@link #addOrUpdateTripTimes(TripTimes)} + * if you want to replace an existing trip. + */ public TimetableBuilder addTripTimes(TripTimes tripTimes) { - this.tripTimes.add(tripTimes); - return this; + var trip = tripTimes.getTrip(); + if (this.tripTimes.containsKey(trip.getId())) { + throw new IllegalStateException( + "Error! TripTimes for the same trip is added twice. Trip: " + trip + ); + } + return addOrUpdateTripTimes(tripTimes); } - public TimetableBuilder addAllTripTimes(List tripTimes) { - this.tripTimes.addAll(tripTimes); + /** + * Add or update the trip-times. If the trip has an associated trip-times, then the trip-times + * are replaced. If not, the trip-times it is added. Consider using + * {@link #addTripTimes(TripTimes)}. + */ + public TimetableBuilder addOrUpdateTripTimes(TripTimes tripTimes) { + this.tripTimes.put(tripTimes.getTrip().getId(), tripTimes); return this; } - public TimetableBuilder setTripTimes(int tripIndex, TripTimes tripTimes) { - this.tripTimes.set(tripIndex, tripTimes); + public TimetableBuilder addAllTripTimes(List tripTimes) { + for (TripTimes it : tripTimes) { + addTripTimes(it); + } return this; } public TimetableBuilder removeTripTimes(TripTimes tripTimesToRemove) { - tripTimes.remove(tripTimesToRemove); + tripTimes.remove(tripTimesToRemove.getTrip().getId()); return this; } - public TimetableBuilder removeAllTripTimes(Set tripTimesToBeRemoved) { - tripTimes.removeAll(tripTimesToBeRemoved); + public TimetableBuilder removeAllTripTimes(Collection tripTimesToBeRemoved) { + for (TripTimes it : tripTimesToBeRemoved) { + tripTimes.remove(it.getTrip().getId()); + } return this; } @@ -69,7 +91,7 @@ public TimetableBuilder removeAllTripTimes(Set tripTimesToBeRemoved) *

*/ public TimetableBuilder updateAllTripTimes(UnaryOperator update) { - tripTimes.replaceAll(update); + tripTimes.replaceAll((t, tt) -> update.apply(tt)); frequencies.replaceAll(it -> new FrequencyEntry( it.startTime, @@ -95,8 +117,8 @@ public LocalDate getServiceDate() { return serviceDate; } - public List getTripTimes() { - return tripTimes; + List createImmutableOrderedListOfTripTimes() { + return tripTimes.values().stream().sorted().toList(); } public List getFrequencies() { @@ -116,7 +138,7 @@ public Direction getDirection() { private TripTimes getRepresentativeTripTimes() { if (!tripTimes.isEmpty()) { - return tripTimes.getFirst(); + return tripTimes.values().stream().findAny().get(); } else if (!frequencies.isEmpty()) { return frequencies.getFirst().tripTimes; } else { diff --git a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 64f06362228..05011db5a48 100644 --- a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -301,20 +301,12 @@ public Result update(RealTimeTripUpdate realTimeTrip TimetableBuilder ttb = Timetable.of(tt).withServiceDate(serviceDate); // Assume all trips in a pattern are from the same feed, which should be the case. - // Find trip index - Trip trip = updatedTripTimes.getTrip(); - int tripIndex = tt.getTripIndex(trip.getId()); - if (tripIndex == -1) { - // Trip not found, add it - ttb.addTripTimes(updatedTripTimes); - } else { - // Set updated trip times of trip - ttb.setTripTimes(tripIndex, updatedTripTimes); - } + ttb.addOrUpdateTripTimes(updatedTripTimes); Timetable updated = ttb.build(); swapTimetable(pattern, tt, updated); + Trip trip = updatedTripTimes.getTrip(); if (pattern.isCreatedByRealtimeUpdater()) { // Remember this pattern for the added trip id and service date FeedScopedId tripId = trip.getId(); diff --git a/src/test/java/org/opentripplanner/model/TimetableTest.java b/src/test/java/org/opentripplanner/model/TimetableTest.java index 75a3af3f3ca..4b9748dd0d7 100644 --- a/src/test/java/org/opentripplanner/model/TimetableTest.java +++ b/src/test/java/org/opentripplanner/model/TimetableTest.java @@ -190,7 +190,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); + timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); assertEquals(20 * 60 + 120, timetable.getTripTimes(trip_1_1_index).getArrivalTime(2)); }); @@ -217,7 +217,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); + timetable = timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); }); // update trip arrival time only @@ -246,7 +246,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); + timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); }); // update trip departure time only @@ -273,7 +273,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); + timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); }); // update trip using stop id @@ -299,7 +299,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).setTripTimes(trip_1_1_index, updatedTripTimes).build(); + timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); }); } diff --git a/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java b/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java index d9377ba2c4f..61fffd7a905 100644 --- a/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java +++ b/src/test/java/org/opentripplanner/routing/stoptimes/StopTimesHelperTest.java @@ -42,7 +42,7 @@ public static void setUp() throws Exception { pattern = originalPattern .copy() - .withScheduledTimeTableBuilder(builder -> builder.setTripTimes(0, newTripTimes)) + .withScheduledTimeTableBuilder(builder -> builder.addOrUpdateTripTimes(newTripTimes)) .build(); // replace the original pattern by the updated pattern in the transit model transitModel.addTripPattern(pattern.getId(), pattern); diff --git a/src/test/java/org/opentripplanner/transit/speed_test/SpeedIntegrationTest.java b/src/test/java/org/opentripplanner/transit/speed_test/SpeedIntegrationTest.java index e5006d949b6..61a23b10ff6 100644 --- a/src/test/java/org/opentripplanner/transit/speed_test/SpeedIntegrationTest.java +++ b/src/test/java/org/opentripplanner/transit/speed_test/SpeedIntegrationTest.java @@ -24,14 +24,14 @@ import org.opentripplanner.transit.speed_test.options.SpeedTestConfig; /** - * This test run the SpeedTest on the Portland dataset. It tests all SpeedTest + * This test runs the SpeedTest on the Portland dataset. It tests all SpeedTest * profiles. This is also a good integration-test for the OTP routing query. */ public class SpeedIntegrationTest { /** - * We need to use a relative path here, because the test will update the results files in case - * the results differ. This make it easy to maintain the test. + * We need to use a relative path here, because the test will update the result files in case + * the results differ. This makes it easy to maintain the test. */ private static final File BASE_DIR = Path.of("src", "test", "resources", "speedtest").toFile(); From 2b6c7f0e49d05d72997f7ea78cbb112d6bb3f7d5 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Thu, 22 Aug 2024 19:47:41 +0200 Subject: [PATCH 03/10] refactor: Replace 'Timetable.of(tt)' with 'tt.copyOf()' --- .../java/org/opentripplanner/model/Timetable.java | 15 +++++++-------- .../opentripplanner/model/TimetableSnapshot.java | 6 +++--- .../model/impl/OtpTransitServiceBuilder.java | 5 +++-- .../transit/model/network/TripPatternBuilder.java | 2 +- .../model/TimetableSnapshotTest.java | 4 ++-- .../org/opentripplanner/model/TimetableTest.java | 10 +++++----- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/opentripplanner/model/Timetable.java b/src/main/java/org/opentripplanner/model/Timetable.java index 7cb747b2c90..97ec6eddb97 100644 --- a/src/main/java/org/opentripplanner/model/Timetable.java +++ b/src/main/java/org/opentripplanner/model/Timetable.java @@ -68,19 +68,18 @@ public class Timetable implements Serializable { this.frequencyEntries = List.copyOf(timetableBuilder.getFrequencies()); } - /** - * Copy constructor: create an un-indexed Timetable with the same TripTimes as the specified - * timetable. - */ - public static TimetableBuilder of(Timetable tt) { - return new TimetableBuilder(tt); - } - /** Construct an empty Timetable. */ public static TimetableBuilder of() { return new TimetableBuilder(); } + /** + * Copy timetable into a builder witch can be used to modify the timetable. + */ + public TimetableBuilder copyOf() { + return new TimetableBuilder(this); + } + /** @return the index of TripTimes for this trip ID in this particular Timetable */ public int getTripIndex(FeedScopedId tripId) { int ret = 0; diff --git a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 05011db5a48..8cd0707d655 100644 --- a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -298,7 +298,7 @@ public Result update(RealTimeTripUpdate realTimeTrip } Timetable tt = resolve(pattern, serviceDate); - TimetableBuilder ttb = Timetable.of(tt).withServiceDate(serviceDate); + TimetableBuilder ttb = tt.copyOf().withServiceDate(serviceDate); // Assume all trips in a pattern are from the same feed, which should be the case. ttb.addOrUpdateTripTimes(updatedTripTimes); @@ -452,8 +452,8 @@ public boolean revertTripToScheduledTripPattern(FeedScopedId tripId, LocalDate s if (tripTimesToRemove != null) { for (Timetable originalTimetable : sortedTimetables) { if (originalTimetable.getTripTimes().contains(tripTimesToRemove)) { - Timetable updatedTimetable = Timetable - .of(originalTimetable) + Timetable updatedTimetable = originalTimetable + .copyOf() .removeTripTimes(tripTimesToRemove) .build(); swapTimetable(pattern, originalTimetable, updatedTimetable); diff --git a/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java b/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java index aa82a66d0b9..d62d0331a34 100644 --- a/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java +++ b/src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java @@ -390,8 +390,9 @@ private void fixOrRemovePatternsWhichReferenceNoneExistingTrips() { .collect(Collectors.toUnmodifiableSet()); if (!tripTimesToBeRemoved.isEmpty()) { removePatterns.add(e); - Timetable updatedTimetable = Timetable - .of(ptn.getScheduledTimetable()) + Timetable updatedTimetable = ptn + .getScheduledTimetable() + .copyOf() .removeAllTripTimes(tripTimesToBeRemoved) .build(); TripPattern updatedPattern = ptn.copy().withScheduledTimeTable(updatedTimetable).build(); diff --git a/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java b/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java index 91451d43cc2..88a391c1e33 100644 --- a/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java +++ b/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java @@ -107,7 +107,7 @@ public TripPatternBuilder withScheduledTimeTableBuilder( // create a builder for the scheduled timetable only if it needs to be modified. // otherwise reuse the existing timetable if (scheduledTimetableBuilder == null) { - scheduledTimetableBuilder = Timetable.of(scheduledTimetable); + scheduledTimetableBuilder = scheduledTimetable.copyOf(); scheduledTimetable = null; } producer.apply(scheduledTimetableBuilder); diff --git a/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index 57bf11dd8f6..ffdfc027b76 100644 --- a/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -54,8 +54,8 @@ public static void setUp() throws Exception { @Test public void testCompare() { Timetable orig = Timetable.of().build(); - Timetable a = Timetable.of(orig).withServiceDate(LocalDate.now(timeZone).minusDays(1)).build(); - Timetable b = Timetable.of(orig).withServiceDate(LocalDate.now(timeZone)).build(); + Timetable a = orig.copyOf().withServiceDate(LocalDate.now(timeZone).minusDays(1)).build(); + Timetable b = orig.copyOf().withServiceDate(LocalDate.now(timeZone)).build(); assertTrue(new TimetableSnapshot.SortedTimetableComparator().compare(a, b) < 0); } diff --git a/src/test/java/org/opentripplanner/model/TimetableTest.java b/src/test/java/org/opentripplanner/model/TimetableTest.java index 4b9748dd0d7..9135abd96ee 100644 --- a/src/test/java/org/opentripplanner/model/TimetableTest.java +++ b/src/test/java/org/opentripplanner/model/TimetableTest.java @@ -190,7 +190,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); + timetable = timetable.copyOf().addOrUpdateTripTimes(updatedTripTimes).build(); assertEquals(20 * 60 + 120, timetable.getTripTimes(trip_1_1_index).getArrivalTime(2)); }); @@ -217,7 +217,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); + timetable = timetable.copyOf().addOrUpdateTripTimes(updatedTripTimes).build(); }); // update trip arrival time only @@ -246,7 +246,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); + timetable = timetable.copyOf().addOrUpdateTripTimes(updatedTripTimes).build(); }); // update trip departure time only @@ -273,7 +273,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); + timetable = timetable.copyOf().addOrUpdateTripTimes(updatedTripTimes).build(); }); // update trip using stop id @@ -299,7 +299,7 @@ public void update() { result.ifSuccess(p -> { var updatedTripTimes = p.getTripTimes(); assertNotNull(updatedTripTimes); - timetable = Timetable.of(timetable).addOrUpdateTripTimes(updatedTripTimes).build(); + timetable = timetable.copyOf().addOrUpdateTripTimes(updatedTripTimes).build(); }); } From cc692c6f56c81df9a37ff5b518f8c32e78260a06 Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Mon, 26 Aug 2024 11:26:56 +0200 Subject: [PATCH 04/10] Fix unit test (the timetable is now sorted) --- .../netex/NetexEpipBundleSmokeTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/java/org/opentripplanner/netex/NetexEpipBundleSmokeTest.java b/src/test/java/org/opentripplanner/netex/NetexEpipBundleSmokeTest.java index b89e6883951..72e750c055f 100644 --- a/src/test/java/org/opentripplanner/netex/NetexEpipBundleSmokeTest.java +++ b/src/test/java/org/opentripplanner/netex/NetexEpipBundleSmokeTest.java @@ -84,7 +84,7 @@ private static FeedScopedId fId(String id) { private void assertAgencies(Collection agencies) { assertEquals(3, agencies.size()); - Agency a = list(agencies).get(0); + Agency a = list(agencies).getFirst(); assertEquals("DE::Authority:41::", a.getId().getId()); assertEquals("HOCHBAHN, Bus", a.getName()); assertNull(a.getUrl()); @@ -146,7 +146,7 @@ private void assertTripPatterns(Collection patterns) { p.getStops().toString() ); List trips = p.scheduledTripsAsStream().toList(); - assertEquals("Trip{HH:DE::ServiceJourney:36439031_0:: X86}", trips.get(0).toString()); + assertEquals("Trip{HH:DE::ServiceJourney:36439062_0:: X86}", trips.getFirst().toString()); assertEquals(55, trips.size()); assertEquals(4, patterns.size()); } @@ -176,12 +176,12 @@ private void assertServiceIds(Collection trips, Collection s private void assetServiceCalendar(CalendarServiceData cal) { ArrayList sIds = new ArrayList<>(cal.getServiceIds()); assertEquals(2, sIds.size()); - FeedScopedId serviceId1 = sIds.get(0); + FeedScopedId serviceId1 = sIds.getFirst(); List dates = cal.getServiceDatesForServiceId(serviceId1); - assertEquals("2023-02-02", dates.get(0).toString()); - assertEquals("2023-12-08", dates.get(dates.size() - 1).toString()); + assertEquals("2023-02-02", dates.getFirst().toString()); + assertEquals("2023-12-08", dates.getLast().toString()); assertEquals(214, dates.size()); } } From 15833df9dfa493e2dcb6b2e3f268978950459964 Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Mon, 26 Aug 2024 11:27:23 +0200 Subject: [PATCH 05/10] Remove redundant timetable sorting in TransitLayerMapper --- .../transit/mappers/TransitLayerMapper.java | 14 -------------- .../mappers/TripPatternForDateMapper.java | 19 +------------------ 2 files changed, 1 insertion(+), 32 deletions(-) diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerMapper.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerMapper.java index d375b4c546c..13f8facef3d 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerMapper.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerMapper.java @@ -7,15 +7,12 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; import javax.annotation.Nullable; import org.opentripplanner.framework.application.OTPFeature; -import org.opentripplanner.model.Timetable; import org.opentripplanner.routing.algorithm.raptoradapter.transit.Transfer; import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitLayer; import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitTuningParameters; @@ -26,7 +23,6 @@ import org.opentripplanner.routing.algorithm.raptoradapter.transit.request.RaptorRequestTransferCache; import org.opentripplanner.transit.model.network.TripPattern; import org.opentripplanner.transit.model.site.StopTransferPriority; -import org.opentripplanner.transit.model.timetable.TripTimes; import org.opentripplanner.transit.service.DefaultTransitService; import org.opentripplanner.transit.service.StopModel; import org.opentripplanner.transit.service.TransitModel; @@ -64,16 +60,6 @@ public static TransitLayer map( return new TransitLayerMapper(transitModel).map(tuningParameters); } - // TODO We could save time by either pre-sorting these, or by using a sorting algorithm that is - // optimized for sorting nearly-sorted lists. - static List getSortedTripTimes(Timetable timetable) { - return timetable - .getTripTimes() - .stream() - .sorted(Comparator.comparing(TripTimes::sortIndex)) - .collect(Collectors.toList()); - } - private TransitLayer map(TransitTuningParameters tuningParameters) { HashMap> tripPatternsByStopByDate; List> transferByStopIndex; diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapper.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapper.java index cd00b9356dc..256a268e2c7 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapper.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapper.java @@ -3,12 +3,9 @@ import gnu.trove.set.TIntSet; import java.time.LocalDate; import java.util.ArrayList; -import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import java.util.stream.Collectors; import javax.annotation.Nullable; import org.opentripplanner.model.Timetable; @@ -33,8 +30,6 @@ public class TripPatternForDateMapper { private static final Logger LOG = LoggerFactory.getLogger(TripPatternForDateMapper.class); - private final ConcurrentMap> sortedTripTimesForTimetable = new ConcurrentHashMap<>(); - private final Map serviceCodesRunningForDate; /** @@ -69,19 +64,7 @@ public TripPatternForDate map(Timetable timetable, LocalDate serviceDate) { List times = new ArrayList<>(); - // The TripTimes are not sorted by departure time in the source timetable because - // OTP1 performs a simple/ linear search. Raptor results depend on trips being - // sorted. We reuse the same timetables many times on different days, so cache the - // sorted versions to avoid repeated compute-intensive sorting. Anecdotally this - // reduces mapping time by more than half, but it is still rather slow. NL Mapping - // takes 32 seconds sorting every timetable, 9 seconds with cached sorting, and 6 - // seconds with no timetable sorting at all. - List sortedTripTimes = sortedTripTimesForTimetable.computeIfAbsent( - timetable, - TransitLayerMapper::getSortedTripTimes - ); - - for (TripTimes tripTimes : sortedTripTimes) { + for (TripTimes tripTimes : timetable.getTripTimes()) { if (!serviceCodesRunning.contains(tripTimes.getServiceCode())) { continue; } From 4b6a019ec5f835b420fe13b34f1eae481a72945f Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Tue, 27 Aug 2024 15:34:27 +0200 Subject: [PATCH 06/10] Code clean-up --- src/main/java/org/opentripplanner/model/TimetableBuilder.java | 2 +- .../transit/model/network/TripPatternBuilder.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opentripplanner/model/TimetableBuilder.java b/src/main/java/org/opentripplanner/model/TimetableBuilder.java index bd21856eecb..d2856a52a98 100644 --- a/src/main/java/org/opentripplanner/model/TimetableBuilder.java +++ b/src/main/java/org/opentripplanner/model/TimetableBuilder.java @@ -138,7 +138,7 @@ public Direction getDirection() { private TripTimes getRepresentativeTripTimes() { if (!tripTimes.isEmpty()) { - return tripTimes.values().stream().findAny().get(); + return tripTimes.values().stream().findFirst().get(); } else if (!frequencies.isEmpty()) { return frequencies.getFirst().tripTimes; } else { diff --git a/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java b/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java index 88a391c1e33..ec2451ea66d 100644 --- a/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java +++ b/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java @@ -17,6 +17,7 @@ import org.opentripplanner.transit.model.basic.TransitMode; import org.opentripplanner.transit.model.framework.AbstractEntityBuilder; import org.opentripplanner.transit.model.framework.FeedScopedId; +import org.opentripplanner.transit.model.timetable.Direction; @SuppressWarnings("UnusedReturnValue") public final class TripPatternBuilder @@ -139,7 +140,7 @@ public int transitReluctanceFactorIndex() { return route.getMode().ordinal(); } - public Object getDirection() { + public Direction getDirection() { if (scheduledTimetable != null) { return scheduledTimetable.getDirection(); } From 16c8264c0647506c5980175f5309ee15b14cddc2 Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Mon, 16 Sep 2024 10:40:30 +0200 Subject: [PATCH 07/10] Ensure stable TripPattern creation order --- .../gtfs/GenerateTripPatternsOperation.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java b/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java index 84b25340e6c..08ad300fa9a 100644 --- a/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java +++ b/src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java @@ -3,6 +3,7 @@ import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ListMultimap; import com.google.common.collect.Multimap; +import com.google.common.collect.MultimapBuilder; import java.util.Collection; import java.util.HashMap; import java.util.List; @@ -47,7 +48,12 @@ public class GenerateTripPatternsOperation { private final Set calendarServiceIds; private final GeometryProcessor geometryProcessor; - private final Multimap tripPatternBuilders = ArrayListMultimap.create(); + // TODO the linked hashset configuration ensures that TripPatterns are created in the same order + // as Trips are imported, as a workaround for issue #6067 + private final Multimap tripPatternBuilders = MultimapBuilder + .linkedHashKeys() + .linkedHashSetValues() + .build(); private final ListMultimap frequenciesForTrip = ArrayListMultimap.create(); private int freqCount = 0; From c3553d5c87146847a83011245e472aa48cb8554f Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Mon, 16 Sep 2024 15:04:23 +0200 Subject: [PATCH 08/10] Code clean-up --- .../org/opentripplanner/model/Timetable.java | 60 ++++++++++++++----- .../model/TimetableBuilder.java | 19 +----- .../transit/model/network/TripPattern.java | 7 ++- 3 files changed, 52 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/opentripplanner/model/Timetable.java b/src/main/java/org/opentripplanner/model/Timetable.java index 97ec6eddb97..6740e2cc99d 100644 --- a/src/main/java/org/opentripplanner/model/Timetable.java +++ b/src/main/java/org/opentripplanner/model/Timetable.java @@ -16,6 +16,7 @@ import java.time.LocalDate; import java.time.ZoneId; import java.util.ArrayList; +import java.util.Collection; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -422,25 +423,19 @@ public LocalDate getServiceDate() { } /** - * The direction for all the trips in this pattern. + * Return the direction for all the trips in this timetable. + * By construction, all trips in a timetable have the same direction. */ public Direction getDirection() { - return Optional - .ofNullable(getRepresentativeTripTimes()) - .map(TripTimes::getTrip) - .map(Trip::getDirection) - .orElse(Direction.UNKNOWN); + return getDirection(tripTimes, frequencyEntries); } + /** + * Return an arbitrary TripTimes in this Timetable. + * Return a scheduled trip times if it exists, otherwise return a frequency-based trip times. + */ public TripTimes getRepresentativeTripTimes() { - if (!getTripTimes().isEmpty()) { - return getTripTimes(0); - } else if (!getFrequencyEntries().isEmpty()) { - return getFrequencyEntries().get(0).tripTimes; - } else { - // Pattern is created only for real-time updates - return null; - } + return getRepresentativeTripTimes(tripTimes, frequencyEntries); } /** @@ -451,4 +446,41 @@ public TripTimes getRepresentativeTripTimes() { public boolean isCreatedByRealTimeUpdater() { return serviceDate != null; } + + /** + * The direction for the given collections of trip times. + * The method assumes that all trip times have the same directions and picks up one arbitrarily. + * @param scheduledTripTimes all the scheduled-based trip times in a timetable. + * @param frequencies all the frequency-based trip times in a timetable. + */ + static Direction getDirection( + Collection scheduledTripTimes, + Collection frequencies + ) { + return Optional + .ofNullable(getRepresentativeTripTimes(scheduledTripTimes, frequencies)) + .map(TripTimes::getTrip) + .map(Trip::getDirection) + .orElse(Direction.UNKNOWN); + } + + /** + * Return an arbitrary TripTimes. + * @param scheduledTripTimes all the scheduled-based trip times in a timetable. + * @param frequencies all the frequency-based trip times in a timetable. + * + */ + private static TripTimes getRepresentativeTripTimes( + Collection scheduledTripTimes, + Collection frequencies + ) { + if (!scheduledTripTimes.isEmpty()) { + return scheduledTripTimes.iterator().next(); + } else if (!frequencies.isEmpty()) { + return frequencies.iterator().next().tripTimes; + } else { + // Pattern is created only for real-time updates + return null; + } + } } diff --git a/src/main/java/org/opentripplanner/model/TimetableBuilder.java b/src/main/java/org/opentripplanner/model/TimetableBuilder.java index d2856a52a98..1c4e30dbdc1 100644 --- a/src/main/java/org/opentripplanner/model/TimetableBuilder.java +++ b/src/main/java/org/opentripplanner/model/TimetableBuilder.java @@ -6,13 +6,11 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.function.UnaryOperator; import org.opentripplanner.transit.model.framework.FeedScopedId; import org.opentripplanner.transit.model.network.TripPattern; import org.opentripplanner.transit.model.timetable.Direction; import org.opentripplanner.transit.model.timetable.FrequencyEntry; -import org.opentripplanner.transit.model.timetable.Trip; import org.opentripplanner.transit.model.timetable.TripTimes; public class TimetableBuilder { @@ -129,22 +127,7 @@ public List getFrequencies() { * The direction for all the trips in this timetable. */ public Direction getDirection() { - return Optional - .ofNullable(getRepresentativeTripTimes()) - .map(TripTimes::getTrip) - .map(Trip::getDirection) - .orElse(Direction.UNKNOWN); - } - - private TripTimes getRepresentativeTripTimes() { - if (!tripTimes.isEmpty()) { - return tripTimes.values().stream().findFirst().get(); - } else if (!frequencies.isEmpty()) { - return frequencies.getFirst().tripTimes; - } else { - // Pattern is created only for real-time updates - return null; - } + return Timetable.getDirection(tripTimes.values(), frequencies); } public Timetable build() { diff --git a/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java b/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java index 8e39d353913..e7d7af45b6d 100644 --- a/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java +++ b/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java @@ -1,7 +1,6 @@ package org.opentripplanner.transit.model.network; import static java.util.Objects.requireNonNull; -import static java.util.Objects.requireNonNullElseGet; import static org.opentripplanner.framework.lang.ObjectUtils.requireNotInitialized; import java.util.ArrayList; @@ -349,7 +348,11 @@ public boolean isModifiedFromTripPatternWithEqualStops(TripPattern other) { } /** - * The direction for all the trips in this pattern. + * Return the direction for all the trips in this pattern. + * By construction, all trips in a pattern have the same direction: + * - trips derived from NeTEx data belong to a ServiceJourney that belongs to a JourneyPattern + * that belongs to a NeTEx Route that specifies a single direction. + * - trips derived from GTFS data are grouped by direction in a trip pattern, during graph build. */ public Direction getDirection() { return scheduledTimetable.getDirection(); From fb4eef585bf7e35d2c12326cf0dd7991ff82639f Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Fri, 20 Sep 2024 11:54:52 +0200 Subject: [PATCH 09/10] Apply review suggestions --- .../model/TimetableBuilder.java | 26 +++++++++---------- .../GenerateTripPatternsOperationTest.java | 3 +++ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/opentripplanner/model/TimetableBuilder.java b/src/main/java/org/opentripplanner/model/TimetableBuilder.java index 1c4e30dbdc1..74e89d0d973 100644 --- a/src/main/java/org/opentripplanner/model/TimetableBuilder.java +++ b/src/main/java/org/opentripplanner/model/TimetableBuilder.java @@ -107,30 +107,30 @@ public TimetableBuilder addFrequencyEntry(FrequencyEntry frequencyEntry) { return this; } - public TripPattern getPattern() { - return pattern; + /** + * The direction for all the trips in this timetable. + */ + public Direction getDirection() { + return Timetable.getDirection(tripTimes.values(), frequencies); } - public LocalDate getServiceDate() { - return serviceDate; + public Timetable build() { + return new Timetable(this); } List createImmutableOrderedListOfTripTimes() { return tripTimes.values().stream().sorted().toList(); } - public List getFrequencies() { - return frequencies; + TripPattern getPattern() { + return pattern; } - /** - * The direction for all the trips in this timetable. - */ - public Direction getDirection() { - return Timetable.getDirection(tripTimes.values(), frequencies); + LocalDate getServiceDate() { + return serviceDate; } - public Timetable build() { - return new Timetable(this); + List getFrequencies() { + return frequencies; } } diff --git a/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java b/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java index 0451edbb40d..74819deabc2 100644 --- a/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java +++ b/src/test/java/org/opentripplanner/gtfs/GenerateTripPatternsOperationTest.java @@ -269,8 +269,11 @@ void testGenerateTripPatterns2TripsDifferentStops() { static List testCases() { return List.of( + // trips with different modes Arguments.of(trip1, trip3), + // trips with different sub-modes Arguments.of(trip1, trip4), + // trips with different directions Arguments.of(trip1, trip5) ); } From e15c3d0cf376a252202f9e6a581a4461e688bd93 Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Wed, 25 Sep 2024 10:09:42 +0200 Subject: [PATCH 10/10] Fix merge conflicts --- .../transit/model/_data/PatternTestModel.java | 13 ++++++------- .../trip/RealtimeTestEnvironmentBuilder.java | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/test/java/org/opentripplanner/transit/model/_data/PatternTestModel.java b/src/test/java/org/opentripplanner/transit/model/_data/PatternTestModel.java index c3afd0ddf61..0b149ae4ee4 100644 --- a/src/test/java/org/opentripplanner/transit/model/_data/PatternTestModel.java +++ b/src/test/java/org/opentripplanner/transit/model/_data/PatternTestModel.java @@ -29,18 +29,17 @@ public class PatternTestModel { * Creates a trip pattern that has a stop pattern, trip times and a trip with a service id. */ public static TripPattern pattern() { - var pattern = TransitModelForTest - .tripPattern("1", ROUTE_1) - .withStopPattern(STOP_PATTERN) - .build(); - var tt = ScheduledTripTimes .of() .withTrip(TRIP) .withArrivalTimes("10:00 10:05") .withDepartureTimes("10:00 10:05") .build(); - pattern.add(tt); - return pattern; + + return TransitModelForTest + .tripPattern("1", ROUTE_1) + .withStopPattern(STOP_PATTERN) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tt)) + .build(); } } diff --git a/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironmentBuilder.java b/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironmentBuilder.java index 7a79b27923e..88f4bf41012 100644 --- a/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironmentBuilder.java +++ b/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironmentBuilder.java @@ -88,8 +88,8 @@ private Trip createTrip(TripInput tripInput) { tripInput.stops().stream().map(TripInput.StopCall::stop).toList() ) ) + .withScheduledTimeTableBuilder(builder -> builder.addTripTimes(tripTimes)) .build(); - pattern.add(tripTimes); transitModel.addTripPattern(pattern.getId(), pattern);