Fix max-stop limit in StreetNearbyStopFinder

application/src/main/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategy.java

@@ -0,0 +1,36 @@
+package org.opentripplanner.astar.strategy;
+
+import java.util.function.Predicate;
+import org.opentripplanner.astar.spi.AStarState;
+import org.opentripplanner.astar.spi.SearchTerminationStrategy;
+
+/**
+ * This termination strategy is used to terminate an a-star search after a number of states matching
+ * some criteria has been found. For example it can be used to limit a search to a maximum number of
+ * stops.
+ */
+public class MaxCountTerminationStrategy<State extends AStarState<State, ?, ?>>
+  implements SearchTerminationStrategy<State> {
+
+  private final int maxCount;
+  private final Predicate<State> shouldIncreaseCount;
+  private int count;
+
+  /**
+   * @param maxCount Terminate the search after this many matching states have been reached.
+   * @param shouldIncreaseCount A predicate to check if a state should increase the count or not.
+   */
+  public MaxCountTerminationStrategy(int maxCount, Predicate<State> shouldIncreaseCount) {
+    this.maxCount = maxCount;
+    this.shouldIncreaseCount = shouldIncreaseCount;
+    this.count = 0;
+  }
+
+  @Override
+  public boolean shouldSearchTerminate(State current) {
+    if (shouldIncreaseCount.test(current)) {
+      count++;
+    }
+    return count >= maxCount;
+  }
+}

application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java

@@ -11,10 +11,8 @@
 import java.util.List;
 import java.util.Set;
 import org.opentripplanner.astar.model.ShortestPathTree;
-import org.opentripplanner.astar.spi.SkipEdgeStrategy;
-import org.opentripplanner.astar.strategy.ComposingSkipEdgeStrategy;
 import org.opentripplanner.astar.strategy.DurationSkipEdgeStrategy;
-import org.opentripplanner.astar.strategy.MaxCountSkipEdgeStrategy;
+import org.opentripplanner.astar.strategy.MaxCountTerminationStrategy;
 import org.opentripplanner.ext.dataoverlay.routing.DataOverlayContext;
 import org.opentripplanner.framework.application.OTPFeature;
 import org.opentripplanner.framework.application.OTPRequestTimeoutException;
@@ -30,8 +28,6 @@
 import org.opentripplanner.street.model.vertex.Vertex;
 import org.opentripplanner.street.search.StreetSearchBuilder;
 import org.opentripplanner.street.search.TraverseMode;
-import org.opentripplanner.street.search.request.StreetSearchRequest;
-import org.opentripplanner.street.search.request.StreetSearchRequestMapper;
 import org.opentripplanner.street.search.state.State;
 import org.opentripplanner.street.search.strategy.DominanceFunctions;
 import org.opentripplanner.transit.model.site.AreaStop;
@@ -46,8 +42,8 @@ public class StreetNearbyStopFinder implements NearbyStopFinder {
   /**
    * 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 maxStopCount The maximum stops to return. 0 means no limit. Regardless of the
+   *                     maxStopCount we will always return all the directly connected stops.
    */
   public StreetNearbyStopFinder(
     Duration durationLimit,
@@ -117,23 +113,30 @@ public Collection<NearbyStop> findNearbyStops(
     // Return only the origin vertices if there are no valid street modes
     if (
       streetRequest.mode() == StreetMode.NOT_SET ||
-      (maxStopCount != 0 && stopsFound.size() >= maxStopCount)
+      (maxStopCount > 0 && stopsFound.size() >= maxStopCount)
     ) {
       return stopsFound;
     }
     stopsFound = new ArrayList<>(stopsFound);
 
-    ShortestPathTree<State, Edge, Vertex> spt = StreetSearchBuilder
+    var streetSearch = StreetSearchBuilder
       .of()
-      .setSkipEdgeStrategy(getSkipEdgeStrategy())
+      .setSkipEdgeStrategy(new DurationSkipEdgeStrategy<>(durationLimit))
       .setDominanceFunction(new DominanceFunctions.MinimumWeight())
       .setRequest(request)
       .setArriveBy(reverseDirection)
       .setStreetRequest(streetRequest)
       .setFrom(reverseDirection ? null : originVertices)
       .setTo(reverseDirection ? originVertices : null)
-      .setDataOverlayContext(dataOverlayContext)
-      .getShortestPathTree();
+      .setDataOverlayContext(dataOverlayContext);
+
+    if (maxStopCount > 0) {
+      streetSearch.setTerminationStrategy(
+        new MaxCountTerminationStrategy<>(maxStopCount, this::hasReachedStop)
+      );
+    }
+
+    ShortestPathTree<State, Edge, Vertex> spt = streetSearch.getShortestPathTree();
 
     // Only used if OTPFeature.FlexRouting.isOn()
     Multimap<AreaStop, State> locationsMap = ArrayListMultimap.create();
@@ -186,16 +189,6 @@ public Collection<NearbyStop> findNearbyStops(
     return stopsFound;
   }
 
-  private SkipEdgeStrategy<State, Edge> getSkipEdgeStrategy() {
-    var durationSkipEdgeStrategy = new DurationSkipEdgeStrategy(durationLimit);
-
-    if (maxStopCount > 0) {
-      var strategy = new MaxCountSkipEdgeStrategy<>(maxStopCount, this::hasReachedStop);
-      return new ComposingSkipEdgeStrategy<>(strategy, durationSkipEdgeStrategy);
-    }
-    return durationSkipEdgeStrategy;
-  }
-
   private boolean canBoardFlex(State state, boolean reverse) {
     Collection<Edge> edges = reverse
       ? state.getVertex().getIncoming()
@@ -212,13 +205,13 @@ private boolean canBoardFlex(State state, boolean reverse) {
    * <p>
    * This is important because there can be cases where states that cannot actually board the vehicle
    * can dominate those that can thereby leading to zero found stops when this predicate is used with
-   * the {@link MaxCountSkipEdgeStrategy}.
+   * the {@link MaxCountTerminationStrategy}.
    * <p>
    * An example of this would be an egress/reverse search with a very high walk reluctance where the
    * states that speculatively rent a vehicle move the walk states down the A* priority queue until
    * the required number of stops are reached to abort the search, leading to zero egress results.
    */
-  public boolean hasReachedStop(State state) {
+  private boolean hasReachedStop(State state) {
     var vertex = state.getVertex();
     return (
       vertex instanceof TransitStopVertex && state.isFinal() && !ignoreVertices.contains(vertex)

application/src/test/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategyTest.java

@@ -0,0 +1,29 @@
+package org.opentripplanner.astar.strategy;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+class MaxCountTerminationStrategyTest {
+
+  @Test
+  void countStates() {
+    var countAllStatesStrategy = new MaxCountTerminationStrategy<>(3, state -> true);
+
+    assertFalse(countAllStatesStrategy.shouldSearchTerminate(null));
+    assertFalse(countAllStatesStrategy.shouldSearchTerminate(null));
+    assertTrue(countAllStatesStrategy.shouldSearchTerminate(null));
+    assertTrue(countAllStatesStrategy.shouldSearchTerminate(null));
+  }
+
+  @Test
+  void countNoStates() {
+    var countNoStatesStrategy = new MaxCountTerminationStrategy<>(1, state -> false);
+
+    assertFalse(countNoStatesStrategy.shouldSearchTerminate(null));
+    assertFalse(countNoStatesStrategy.shouldSearchTerminate(null));
+    assertFalse(countNoStatesStrategy.shouldSearchTerminate(null));
+    assertFalse(countNoStatesStrategy.shouldSearchTerminate(null));
+  }
+}

application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderMultipleLinksTest.java

@@ -0,0 +1,71 @@
+package org.opentripplanner.graph_builder.module.nearbystops;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinderTest.assertStopAtDistance;
+import static org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinderTest.assertZeroDistanceStop;
+import static org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinderTest.sort;
+
+import java.time.Duration;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.opentripplanner.framework.geometry.WgsCoordinate;
+import org.opentripplanner.routing.algorithm.GraphRoutingTest;
+import org.opentripplanner.routing.api.request.RouteRequest;
+import org.opentripplanner.routing.api.request.request.StreetRequest;
+import org.opentripplanner.street.model.vertex.TransitStopVertex;
+
+class StreetNearbyStopFinderMultipleLinksTest extends GraphRoutingTest {
+
+  private static final WgsCoordinate origin = new WgsCoordinate(0.0, 0.0);
+  private TransitStopVertex stopA;
+  private TransitStopVertex stopB;
+  private TransitStopVertex stopC;
+
+  @BeforeEach
+  protected void setUp() throws Exception {
+    modelOf(
+      new Builder() {
+        @Override
+        public void build() {
+          var A = intersection("A", origin);
+          var B = intersection("B", origin.moveEastMeters(100));
+          var C = intersection("C", origin.moveEastMeters(200));
+
+          biStreet(A, B, 100);
+          biStreet(B, C, 100);
+
+          stopA = stop("StopA", A.toWgsCoordinate());
+          stopB = stop("StopB", B.toWgsCoordinate());
+          stopC = stop("StopC", C.toWgsCoordinate());
+
+          biLink(A, stopA);
+
+          // B has many links
+          biLink(B, stopB);
+          biLink(B, stopB);
+          biLink(B, stopB);
+          biLink(B, stopB);
+
+          biLink(C, stopC);
+        }
+      }
+    );
+  }
+
+  @Test
+  void testMaxStopCountRegression() {
+    // Max-stop-count should work correctly even though there are multiple links B <-> stopB
+    var durationLimit = Duration.ofMinutes(10);
+    var maxStopCount = 3;
+    var finder = new StreetNearbyStopFinder(durationLimit, maxStopCount, null);
+
+    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));
+  }
+}

application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java

@@ -99,7 +99,6 @@ void testMultipleStops() {
   }
 
   @Test
-  @Disabled("Currently disabled because of a bug in stop counting")
   void testMaxStopCount() {
     var durationLimit = Duration.ofMinutes(10);
     var maxStopCount = 2;
@@ -150,7 +149,6 @@ void testIgnoreStops() {
   }
 
   @Test
-  @Disabled("Currently disabled because of a bug in stop counting")
   void testIgnoreStopsWithMaxStops() {
     var durationLimit = Duration.ofMinutes(10);
     var maxStopCount = 1;
@@ -165,14 +163,14 @@ void testIgnoreStopsWithMaxStops() {
     assertStopAtDistance(stopC, 200, sortedNearbyStops.get(0));
   }
 
-  private List<NearbyStop> sort(Collection<NearbyStop> stops) {
+  static List<NearbyStop> sort(Collection<NearbyStop> stops) {
     return stops.stream().sorted(Comparator.comparing(x -> x.distance)).toList();
   }
 
   /**
    * Verify that the nearby stop is zero distance and corresponds to the expected vertex
    */
-  private void assertZeroDistanceStop(TransitStopVertex expected, NearbyStop nearbyStop) {
+  static void assertZeroDistanceStop(TransitStopVertex expected, NearbyStop nearbyStop) {
     assertEquals(expected.getStop(), nearbyStop.stop);
     assertEquals(0, nearbyStop.distance);
     assertEquals(0, nearbyStop.edges.size());
@@ -183,7 +181,7 @@ private void assertZeroDistanceStop(TransitStopVertex expected, NearbyStop nearb
   /**
    * Verify that the nearby stop is at a specific distance and corresponds to the expected vertex
    */
-  private void assertStopAtDistance(
+  static void assertStopAtDistance(
     TransitStopVertex expected,
     double expectedDistance,
     NearbyStop nearbyStop