diff --git a/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java b/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java index 7f389bc41ec..eb37b274c8d 100644 --- a/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java +++ b/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java @@ -12,10 +12,14 @@ import org.opentripplanner.ext.stopconsolidation.internal.DefaultStopConsolidationRepository; import org.opentripplanner.ext.stopconsolidation.internal.DefaultStopConsolidationService; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup; +import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopLeg; import org.opentripplanner.model.fare.FareProduct; import org.opentripplanner.model.fare.FareProductUse; +import org.opentripplanner.model.plan.Leg; import org.opentripplanner.model.plan.Place; import org.opentripplanner.model.plan.PlanTestConstants; +import org.opentripplanner.model.plan.ScheduledTransitLeg; +import org.opentripplanner.model.plan.StreetLeg; import org.opentripplanner.model.plan.TestItineraryBuilder; import org.opentripplanner.transit.model.basic.Money; @@ -32,21 +36,18 @@ class DecorateConsolidatedStopNamesTest { private static final List fpu = List.of( new FareProductUse("c1a04702-1fb6-32d4-ba02-483bf68111ed", fp) ); + private static final List GROUPS = List.of( + new ConsolidatedStopGroup(STOP_C.getId(), List.of(STOP_D.getId())) + ); + private static final Place PLACE_C = Place.forStop(STOP_C); @Test void changeNames() { - var transitModel = TestStopConsolidationModel.buildTransitModel(); - - var groups = List.of(new ConsolidatedStopGroup(STOP_C.getId(), List.of(STOP_D.getId()))); - var repo = new DefaultStopConsolidationRepository(); - repo.addGroups(groups); - - var service = new DefaultStopConsolidationService(repo, transitModel); - var filter = new DecorateConsolidatedStopNames(service); + var filter = defaultFilter(); var itinerary = TestItineraryBuilder - .newItinerary(Place.forStop(STOP_C)) - .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, Place.forStop(STOP_C)) + .newItinerary(PLACE_C) + .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, PLACE_C) .bus(1, T11_05, T11_12, PlanTestConstants.E) .bus(1, T11_05, T11_12, PlanTestConstants.F) .build(); @@ -62,4 +63,51 @@ void changeNames() { // Check that the fares were carried over assertEquals(fpu, updatedLeg.fareProducts()); } + + @Test + void removeTransferAtConsolidatedStop() { + final var filter = defaultFilter(); + + var itinerary = TestItineraryBuilder + .newItinerary(PLACE_C) + .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, PLACE_C) + .walk(1, PLACE_C) + .bus(1, T11_05, T11_12, PlanTestConstants.F) + .build(); + + filter.decorate(itinerary); + + var legs = itinerary.getLegs().stream().map(Leg::getClass).toList(); + assertEquals(List.of(ConsolidatedStopLeg.class, ScheduledTransitLeg.class), legs); + } + + @Test + void keepRegularTransfer() { + final var filter = defaultFilter(); + + var itinerary = TestItineraryBuilder + .newItinerary(PLACE_C) + .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, PLACE_C) + .walk(1, PlanTestConstants.E) + .bus(1, T11_05, T11_12, PlanTestConstants.F) + .build(); + + filter.decorate(itinerary); + + var legs = itinerary.getLegs().stream().map(Leg::getClass).toList(); + assertEquals( + List.of(ConsolidatedStopLeg.class, StreetLeg.class, ScheduledTransitLeg.class), + legs + ); + } + + private static DecorateConsolidatedStopNames defaultFilter() { + var transitModel = TestStopConsolidationModel.buildTransitModel(); + + var repo = new DefaultStopConsolidationRepository(); + repo.addGroups(GROUPS); + + var service = new DefaultStopConsolidationService(repo, transitModel); + return new DecorateConsolidatedStopNames(service); + } } diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java index 1806b3e9e32..6def5c85fa1 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java @@ -1,8 +1,10 @@ package org.opentripplanner.ext.stopconsolidation; +import java.util.ArrayList; import java.util.Objects; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopLeg; import org.opentripplanner.model.plan.Itinerary; +import org.opentripplanner.model.plan.Leg; import org.opentripplanner.model.plan.ScheduledTransitLeg; import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryDecorator; @@ -13,6 +15,7 @@ */ public class DecorateConsolidatedStopNames implements ItineraryDecorator { + private static final int MAX_INTRA_STOP_WALK_DISTANCE_METERS = 15; private final StopConsolidationService service; public DecorateConsolidatedStopNames(StopConsolidationService service) { @@ -22,6 +25,7 @@ public DecorateConsolidatedStopNames(StopConsolidationService service) { @Override public void decorate(Itinerary itinerary) { replaceConsolidatedStops(itinerary); + removeShortWalkLegs(itinerary); } /** @@ -51,6 +55,43 @@ private void replaceConsolidatedStops(Itinerary i) { }); } + /** + * Removes walk legs from and to a consolidated stop if they are deemed "short". This means that + * they are from a different element of the consolidated stop. + */ + private void removeShortWalkLegs(Itinerary itinerary) { + var legs = new ArrayList<>(itinerary.getLegs()); + var first = legs.getFirst(); + if ( + service.isPartOfConsolidatedStop(first.getTo().stop) && + isShortWalkLeg(first) + ) { + legs.removeFirst(); + } + var last = legs.getLast(); + if ( + service.isPartOfConsolidatedStop(last.getFrom().stop) && + isShortWalkLeg(last) + ) { + legs.removeLast(); + } + + var transfersRemoved = legs.stream().filter(l -> !isTransferWithinConsolidatedStop(l)).toList(); + + itinerary.setLegs(transfersRemoved); + } + + private boolean isTransferWithinConsolidatedStop(Leg l) { + return isShortWalkLeg(l) && + service.isPartOfConsolidatedStop(l.getFrom().stop) && + service.isPartOfConsolidatedStop(l.getTo().stop); + } + + private static boolean isShortWalkLeg(Leg leg) { + return leg.isWalkingLeg() && + leg.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS; + } + /** * Figures out if the from/to stops are part of a consolidated stop group and therefore * some stops need to be replaced. diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java index 7f5cf431a4c..ca7254aa2fa 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java @@ -25,7 +25,7 @@ */ public class StopConsolidationModule implements GraphBuilderModule { - private static final Logger LOG = LoggerFactory.getLogger(TripPattern.class); + private static final Logger LOG = LoggerFactory.getLogger(StopConsolidationModule.class); private final StopConsolidationRepository repository; private final TransitModel transitModel; diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java index 68efe8744cc..0457212e66a 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java @@ -2,6 +2,7 @@ import java.util.List; import java.util.Optional; +import javax.annotation.Nullable; import org.opentripplanner.ext.stopconsolidation.model.StopReplacement; import org.opentripplanner.transit.model.framework.FeedScopedId; import org.opentripplanner.transit.model.organization.Agency; @@ -44,5 +45,5 @@ public interface StopConsolidationService { */ Optional primaryStop(FeedScopedId id); - boolean isPartOfConsolidatedStop(StopLocation sl); + boolean isPartOfConsolidatedStop(@Nullable StopLocation sl); } diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java index 53a0f8ed827..80d3d700866 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java @@ -5,6 +5,7 @@ import java.util.Optional; import java.util.stream.Stream; import org.opentripplanner.ext.stopconsolidation.StopConsolidationRepository; +import javax.annotation.Nullable; import org.opentripplanner.ext.stopconsolidation.StopConsolidationService; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup; import org.opentripplanner.ext.stopconsolidation.model.StopReplacement; @@ -67,8 +68,12 @@ public boolean isSecondaryStop(StopLocation stop) { } @Override - public boolean isPartOfConsolidatedStop(StopLocation sl) { - return isSecondaryStop(sl) || isPrimaryStop(sl); + public boolean isPartOfConsolidatedStop(@Nullable StopLocation sl) { + if (sl == null) { + return false; + } else { + return isSecondaryStop(sl) || isPrimaryStop(sl); + } } @Override diff --git a/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java b/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java index 01cba0dd47f..695ac5a15f5 100644 --- a/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java +++ b/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java @@ -114,7 +114,9 @@ public static ItineraryListFilterChain createFilterChain( builder.withEmissions(new DecorateWithEmission(context.emissionsService())); } - if (context.stopConsolidationService() != null) { + if ( + context.stopConsolidationService() != null && context.stopConsolidationService().isActive() + ) { builder.withConsolidatedStopNamesDecorator( new DecorateConsolidatedStopNames(context.stopConsolidationService()) );