Merge pull request #6157 from ibi-group/consolidated-stops-fixup

Improve logic for transfers in consolidated stops

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<FareProductUse> fpu = List.of(
     new FareProductUse("c1a04702-1fb6-32d4-ba02-483bf68111ed", fp)
   );
+  private static final List<ConsolidatedStopGroup> 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);
+  }
 }

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.

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;

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<StopLocation> primaryStop(FeedScopedId id);
 
-  boolean isPartOfConsolidatedStop(StopLocation sl);
+  boolean isPartOfConsolidatedStop(@Nullable StopLocation sl);
 }

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

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())
       );