From 30cc9251a31bb1a7ee88e15a67b9f244c0d70b47 Mon Sep 17 00:00:00 2001 From: Ville Pihlava Date: Tue, 17 Dec 2024 09:47:50 +0200 Subject: [PATCH] Simplify implementation by removing changes to StreetNearbyStopFinder. --- .../module/DirectTransferGenerator.java | 141 ++++++++---------- .../nearbystops/StreetNearbyStopFinder.java | 28 +--- .../StreetNearbyStopFinderTest.java | 72 --------- 3 files changed, 61 insertions(+), 180 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java b/application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java index 90cab4019e3..6b7c53efc15 100644 --- a/application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java +++ b/application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java @@ -93,23 +93,13 @@ public void buildGraph() { timetableRepository.index(); /* The linker will use streets if they are available, or straight-line distance otherwise. */ - NearbyStopFinder nearbyStopFinder = createNearbyStopFinder( - defaultMaxTransferDuration, - Set.of() - ); + NearbyStopFinder nearbyStopFinder = createNearbyStopFinder(defaultMaxTransferDuration); HashMap defaultNearbyStopFinders = new HashMap<>(); /* These are used for calculating transfers only between carsAllowedStops. */ HashMap carsAllowedStopNearbyStopFinders = new HashMap<>(); List stops = graph.getVerticesOfType(TransitStopVertex.class); - Set carsAllowedStops = timetableRepository - .getStopLocationsUsedForCarsAllowedTrips() - .stream() - .map(StopLocation::getId) - .map(graph::getStopVertexForStopId) - // filter out null values if no TransitStopVertex is found for ID - .filter(TransitStopVertex.class::isInstance) - .collect(Collectors.toSet()); + Set carsAllowedStops = timetableRepository.getStopLocationsUsedForCarsAllowedTrips(); LOG.info("Creating transfers based on requests:"); transferRequests.forEach(transferProfile -> LOG.info(transferProfile.toString())); @@ -147,8 +137,7 @@ public void buildGraph() { flexTransferRequests, defaultNearbyStopFinders, carsAllowedStopNearbyStopFinders, - nearbyStopFinder, - carsAllowedStops + nearbyStopFinder ); stops @@ -169,14 +158,29 @@ public void buildGraph() { // Calculate default transfers. for (RouteRequest transferProfile : defaultTransferRequests) { StreetMode mode = transferProfile.journey().transfer().mode(); - findNearbyStops( - defaultNearbyStopFinders.get(mode), - ts0, - transferProfile, - stop, - distinctTransfers, - mode - ); + for (NearbyStop sd : defaultNearbyStopFinders + .get(mode) + .findNearbyStops(ts0, transferProfile, transferProfile.journey().transfer(), false)) { + // Skip the origin stop, loop transfers are not needed. + if (sd.stop == stop) { + continue; + } + if (sd.stop.transfersNotAllowed()) { + continue; + } + TransferKey transferKey = new TransferKey(stop, sd.stop, sd.edges); + PathTransfer pathTransfer = distinctTransfers.get(transferKey); + if (pathTransfer == null) { + // If the PathTransfer can't be found, it is created. + distinctTransfers.put( + transferKey, + new PathTransfer(stop, sd.stop, sd.distance, sd.edges, EnumSet.of(mode)) + ); + } else { + // If the PathTransfer is found, a new PathTransfer with the added mode is created. + distinctTransfers.put(transferKey, pathTransfer.withAddedMode(mode)); + } + } } // Calculate flex transfers if flex routing is enabled. for (RouteRequest transferProfile : flexTransferRequests) { @@ -210,17 +214,36 @@ public void buildGraph() { } } // Calculate transfers between stops that can use trips with cars if configured. - if (carsAllowedStops.contains(ts0)) { + if (carsAllowedStops.contains(stop)) { for (RouteRequest transferProfile : carsAllowedStopTransferRequests) { StreetMode mode = transferProfile.journey().transfer().mode(); - findNearbyStops( - carsAllowedStopNearbyStopFinders.get(mode), - ts0, - transferProfile, - stop, - distinctTransfers, - mode - ); + for (NearbyStop sd : carsAllowedStopNearbyStopFinders + .get(mode) + .findNearbyStops(ts0, transferProfile, transferProfile.journey().transfer(), false)) { + // Skip the origin stop, loop transfers are not needed. + if (sd.stop == stop) { + continue; + } + if (sd.stop.transfersNotAllowed()) { + continue; + } + // Only calculate transfers between carsAllowedStops. + if (!carsAllowedStops.contains(sd.stop)) { + continue; + } + TransferKey transferKey = new TransferKey(stop, sd.stop, sd.edges); + PathTransfer pathTransfer = distinctTransfers.get(transferKey); + if (pathTransfer == null) { + // If the PathTransfer can't be found, it is created. + distinctTransfers.put( + transferKey, + new PathTransfer(stop, sd.stop, sd.distance, sd.edges, EnumSet.of(mode)) + ); + } else { + // If the PathTransfer is found, a new PathTransfer with the added mode is created. + distinctTransfers.put(transferKey, pathTransfer.withAddedMode(mode)); + } + } } } @@ -270,10 +293,7 @@ public void buildGraph() { * whether the graph has a street network and if ConsiderPatternsForDirectTransfers feature is * enabled. */ - private NearbyStopFinder createNearbyStopFinder( - Duration radiusByDuration, - Set findOnlyVertices - ) { + private NearbyStopFinder createNearbyStopFinder(Duration radiusByDuration) { var transitService = new DefaultTransitService(timetableRepository); NearbyStopFinder finder; if (!graph.hasStreets) { @@ -283,7 +303,7 @@ private NearbyStopFinder createNearbyStopFinder( finder = new StraightLineNearbyStopFinder(transitService, radiusByDuration); } else { LOG.info("Creating direct transfer edges between stops using the street network from OSM..."); - finder = new StreetNearbyStopFinder(radiusByDuration, 0, null, Set.of(), findOnlyVertices); + finder = new StreetNearbyStopFinder(radiusByDuration, 0, null, Set.of()); } if (OTPFeature.ConsiderPatternsForDirectTransfers.isOn()) { @@ -299,8 +319,7 @@ private void parseTransferParameters( List flexTransferRequests, HashMap defaultNearbyStopFinders, HashMap carsAllowedStopNearbyStopFinders, - NearbyStopFinder nearbyStopFinder, - Set carsAllowedStops + NearbyStopFinder nearbyStopFinder ) { for (RouteRequest transferProfile : transferRequests) { StreetMode mode = transferProfile.journey().transfer().mode(); @@ -313,10 +332,7 @@ private void parseTransferParameters( // Set mode-specific maxTransferDuration, if it is set in the build config. Duration maxTransferDuration = transferParameters.maxTransferDuration(); if (maxTransferDuration != Duration.ZERO) { - defaultNearbyStopFinders.put( - mode, - createNearbyStopFinder(maxTransferDuration, Set.of()) - ); + defaultNearbyStopFinders.put(mode, createNearbyStopFinder(maxTransferDuration)); } else { defaultNearbyStopFinders.put(mode, nearbyStopFinder); } @@ -327,10 +343,7 @@ private void parseTransferParameters( carsAllowedStopTransferRequests.add(transferProfile); carsAllowedStopNearbyStopFinders.put( mode, - createNearbyStopFinder( - carsAllowedStopMaxTransferDuration, - Collections.unmodifiableSet(carsAllowedStops) - ) + createNearbyStopFinder(carsAllowedStopMaxTransferDuration) ); } } else { @@ -350,41 +363,5 @@ private void parseTransferParameters( } } - private void findNearbyStops( - NearbyStopFinder nearbyStopFinder, - TransitStopVertex ts0, - RouteRequest transferProfile, - RegularStop stop, - Map distinctTransfers, - StreetMode mode - ) { - for (NearbyStop sd : nearbyStopFinder.findNearbyStops( - ts0, - transferProfile, - transferProfile.journey().transfer(), - false - )) { - // Skip the origin stop, loop transfers are not needed. - if (sd.stop == stop) { - continue; - } - if (sd.stop.transfersNotAllowed()) { - continue; - } - TransferKey transferKey = new TransferKey(stop, sd.stop, sd.edges); - PathTransfer pathTransfer = distinctTransfers.get(transferKey); - if (pathTransfer == null) { - // If the PathTransfer can't be found, it is created. - distinctTransfers.put( - transferKey, - new PathTransfer(stop, sd.stop, sd.distance, sd.edges, EnumSet.of(mode)) - ); - } else { - // If the PathTransfer is found, a new PathTransfer with the added mode is created. - distinctTransfers.put(transferKey, pathTransfer.withAddedMode(mode)); - } - } - } - private record TransferKey(StopLocation source, StopLocation target, List edges) {} } diff --git a/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java b/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java index 327315a0be9..8277bd47e4c 100644 --- a/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java +++ b/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java @@ -38,7 +38,6 @@ public class StreetNearbyStopFinder implements NearbyStopFinder { private final int maxStopCount; private final DataOverlayContext dataOverlayContext; private final Set ignoreVertices; - private final Set findOnlyVertices; /** * Construct a NearbyStopFinder for the given graph and search radius. @@ -51,7 +50,7 @@ public StreetNearbyStopFinder( int maxStopCount, DataOverlayContext dataOverlayContext ) { - this(durationLimit, maxStopCount, dataOverlayContext, Set.of(), Set.of()); + this(durationLimit, maxStopCount, dataOverlayContext, Set.of()); } /** @@ -66,31 +65,11 @@ public StreetNearbyStopFinder( int maxStopCount, DataOverlayContext dataOverlayContext, Set ignoreVertices - ) { - this(durationLimit, maxStopCount, dataOverlayContext, ignoreVertices, Set.of()); - } - - /** - * Construct a NearbyStopFinder for the given graph and search radius. - * - * @param maxStopCount The maximum stops to return. 0 means no limit. Regardless of the maxStopCount - * we will always return all the directly connected stops. - * @param ignoreVertices A set of stop vertices to ignore and not return NearbyStops for. - * - * @param findOnlyVertices Only return NearbyStops that are in this set. If this is empty, no filtering is performed. - */ - public StreetNearbyStopFinder( - Duration durationLimit, - int maxStopCount, - DataOverlayContext dataOverlayContext, - Set ignoreVertices, - Set findOnlyVertices ) { this.dataOverlayContext = dataOverlayContext; this.durationLimit = durationLimit; this.maxStopCount = maxStopCount; this.ignoreVertices = ignoreVertices; - this.findOnlyVertices = findOnlyVertices; } /** @@ -170,10 +149,7 @@ public Collection findNearbyStops( continue; } if (targetVertex instanceof TransitStopVertex tsv && state.isFinal()) { - // If a set of findOnlyVertices is provided, only add stops that are in the set. - if (findOnlyVertices.isEmpty() || findOnlyVertices.contains(targetVertex)) { - stopsFound.add(NearbyStop.nearbyStopForState(state, tsv.getStop())); - } + stopsFound.add(NearbyStop.nearbyStopForState(state, tsv.getStop())); } if ( OTPFeature.FlexRouting.isOn() && diff --git a/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java b/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java index d970569d091..aae02451b33 100644 --- a/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java +++ b/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java @@ -163,78 +163,6 @@ void testIgnoreStopsWithMaxStops() { assertStopAtDistance(stopC, 200, sortedNearbyStops.get(0)); } - @Test - void testFindOnlyVerticesStops() { - var durationLimit = Duration.ofMinutes(10); - var maxStopCount = 0; - Set findOnlyStops = Set.of(stopB, stopC); - var finder = new StreetNearbyStopFinder( - durationLimit, - maxStopCount, - null, - Set.of(), - findOnlyStops - ); - - var sortedNearbyStops = sort( - finder.findNearbyStops(stopA, new RouteRequest(), new StreetRequest(), false) - ); - - assertThat(sortedNearbyStops).hasSize(3); - assertZeroDistanceStop(stopA, sortedNearbyStops.get(0)); - assertStopAtDistance(stopB, 100, sortedNearbyStops.get(1)); - assertStopAtDistance(stopC, 200, sortedNearbyStops.get(2)); - } - - @Test - void testFindOnlyVerticesStopsWithIgnore() { - var durationLimit = Duration.ofMinutes(10); - var maxStopCount = 0; - Set findOnlyStops = Set.of(stopB, stopC); - Set ignore = Set.of(stopB); - var finder = new StreetNearbyStopFinder( - durationLimit, - maxStopCount, - null, - ignore, - findOnlyStops - ); - - var sortedNearbyStops = sort( - finder.findNearbyStops(stopA, new RouteRequest(), new StreetRequest(), false) - ); - - assertThat(sortedNearbyStops).hasSize(2); - assertZeroDistanceStop(stopA, sortedNearbyStops.get(0)); - assertStopAtDistance(stopC, 200, sortedNearbyStops.get(1)); - } - - @Test - void testFindOnlyVerticesStopsWithDurationLimit() { - // If we only allow walk for 101 seconds and speed is 1 m/s we should only be able to reach - // one extra stop. - var durationLimit = Duration.ofSeconds(101); - var maxStopCount = 0; - Set findOnlyStops = Set.of(stopB, stopC); - var routeRequest = new RouteRequest() - .withPreferences(b -> b.withWalk(walkPreferences -> walkPreferences.withSpeed(1.0))); - - var finder = new StreetNearbyStopFinder( - durationLimit, - maxStopCount, - null, - Set.of(), - findOnlyStops - ); - var sortedNearbyStops = sort( - finder.findNearbyStops(stopA, routeRequest, new StreetRequest(), false) - ); - - assertThat(sortedNearbyStops).hasSize(2); - assertZeroDistanceStop(stopA, sortedNearbyStops.get(0)); - assertStopAtDistance(stopB, 100, sortedNearbyStops.get(1)); - } - static List sort(Collection stops) { return stops.stream().sorted(Comparator.comparing(x -> x.distance)).toList(); }