Merge pull request #6484 from HSLdevcom/fix-rounding-error-in-astar

Fix rounding error in street routing

application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java

@@ -7,6 +7,7 @@
 import java.time.Instant;
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
+import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -54,7 +55,7 @@ public class RouteRequest implements Cloneable, Serializable {
 
   private List<ViaLocation> via = Collections.emptyList();
 
-  private Instant dateTime = Instant.now();
+  private Instant dateTime = Instant.now().truncatedTo(ChronoUnit.SECONDS);
 
   @Nullable
   private Duration maxSearchWindow;
@@ -152,8 +153,14 @@ public Instant dateTime() {
     return dateTime;
   }
 
+  /**
+   * The dateTime will be set to a whole number of seconds. We don't do sub-second accuracy,
+   * and if we set the millisecond part to a non-zero value, rounding will not be guaranteed
+   * to be the same for departAt and arriveBy queries.
+   * @param dateTime Either a departAt time or an arriveBy time, one second's accuracy
+   */
   public void setDateTime(Instant dateTime) {
-    this.dateTime = dateTime;
+    this.dateTime = dateTime.truncatedTo(ChronoUnit.SECONDS);
   }
 
   public void setDateTime(String date, String time, ZoneId tz) {

application/src/main/java/org/opentripplanner/routing/impl/GraphPathFinder.java

@@ -1,7 +1,6 @@
 package org.opentripplanner.routing.impl;
 
 import java.time.Instant;
-import java.time.temporal.ChronoUnit;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
@@ -139,7 +138,7 @@ public List<GraphPath<State, Edge, Vertex>> graphPathFinderEntryPoint(
     Set<Vertex> to
   ) {
     OTPRequestTimeoutException.checkForTimeout();
-    Instant reqTime = request.dateTime().truncatedTo(ChronoUnit.SECONDS);
+    Instant reqTime = request.dateTime();
 
     List<GraphPath<State, Edge, Vertex>> paths = getPaths(request, from, to);
 
@@ -151,13 +150,21 @@ public List<GraphPath<State, Edge, Vertex>> graphPathFinderEntryPoint(
         GraphPath<State, Edge, Vertex> graphPath = gpi.next();
         // TODO check, is it possible that arriveBy and time are modifed in-place by the search?
         if (request.arriveBy()) {
-          if (graphPath.states.getLast().getTime().isAfter(reqTime)) {
-            LOG.error("A graph path arrives after the requested time. This implies a bug.");
+          if (graphPath.states.getLast().getTimeAccurate().isAfter(reqTime)) {
+            LOG.error(
+              "A graph path arrives {} after the requested time {}. This implies a bug.",
+              graphPath.states.getLast().getTimeAccurate(),
+              reqTime
+            );
             gpi.remove();
           }
         } else {
-          if (graphPath.states.getFirst().getTime().isBefore(reqTime)) {
-            LOG.error("A graph path leaves before the requested time. This implies a bug.");
+          if (graphPath.states.getFirst().getTimeAccurate().isBefore(reqTime)) {
+            LOG.error(
+              "A graph path leaves {} before the requested time {}. This implies a bug.",
+              graphPath.states.getFirst().getTimeAccurate(),
+              reqTime
+            );
             gpi.remove();
           }
         }

application/src/main/java/org/opentripplanner/street/search/request/StreetSearchRequest.java

@@ -1,6 +1,7 @@
 package org.opentripplanner.street.search.request;
 
 import java.time.Instant;
+import java.time.temporal.ChronoUnit;
 import javax.annotation.Nullable;
 import org.locationtech.jts.geom.Coordinate;
 import org.locationtech.jts.geom.Envelope;
@@ -52,7 +53,7 @@ public class StreetSearchRequest implements AStarRequest {
    * Constructor only used for creating a default instance.
    */
   private StreetSearchRequest() {
-    this.startTime = Instant.now();
+    this.startTime = Instant.now().truncatedTo(ChronoUnit.SECONDS);
     this.preferences = new RoutingPreferences();
     this.mode = StreetMode.WALK;
     this.arriveBy = false;
@@ -64,7 +65,7 @@ private StreetSearchRequest() {
   }
 
   StreetSearchRequest(StreetSearchRequestBuilder builder) {
-    this.startTime = builder.startTime;
+    this.startTime = builder.startTime.truncatedTo(ChronoUnit.SECONDS);
     this.preferences = builder.preferences;
     this.mode = builder.mode;
     this.arriveBy = builder.arriveBy;

application/src/main/java/org/opentripplanner/street/search/state/State.java

@@ -526,6 +526,9 @@ private double getWalkDistanceDelta() {
   }
 
   private State reversedClone() {
+    // these must be getTime(), not getTimeAccurate(), so that the reversed path (which does not
+    // have arriveBy true anymore) has times which round correctly, as the rounding rules
+    // depend on arriveBy
     StreetSearchRequest reversedRequest = request
       .copyOfReversed(getTime())
       .withPreferences(p -> {

application/src/test/java/org/opentripplanner/street/integration/WalkRoutingTest.java

@@ -1,9 +1,14 @@
 package org.opentripplanner.street.integration;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
 import java.time.Instant;
+import java.time.temporal.ChronoUnit;
 import java.util.List;
-import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.opentripplanner.ConstantsForTests;
 import org.opentripplanner.TestOtpModel;
 import org.opentripplanner.astar.model.GraphPath;
@@ -45,18 +50,39 @@ class WalkRoutingTest {
   void shouldRouteAroundRoundabout() {
     var start = new GenericLocation(59.94646, 10.77511);
     var end = new GenericLocation(59.94641, 10.77522);
-    Assertions.assertDoesNotThrow(() -> route(roundabout, start, end));
+    assertDoesNotThrow(() -> route(roundabout, start, end, dateTime, false));
+  }
+
+  @ParameterizedTest
+  @ValueSource(ints = { 0, 200, 400, 499, 500, 501, 600, 700, 800, 900, 999 })
+  void pathReversalWorks(int offset) {
+    var start = new GenericLocation(59.94646, 10.77511);
+    var end = new GenericLocation(59.94641, 10.77522);
+    var base = dateTime.truncatedTo(ChronoUnit.SECONDS);
+    var time = base.plusMillis(offset);
+    var results = route(roundabout, start, end, time, true);
+    assertEquals(1, results.size());
+    var states = results.get(0).states;
+    var diff = ChronoUnit.MILLIS.between(
+      states.getFirst().getTimeAccurate(),
+      states.getLast().getTimeAccurate()
+    );
+    // should be same for every parametrized offset, otherwise irrelevant
+    assertEquals(13926, diff);
   }
 
   private static List<GraphPath<State, Edge, Vertex>> route(
     Graph graph,
     GenericLocation from,
-    GenericLocation to
+    GenericLocation to,
+    Instant instant,
+    boolean arriveBy
   ) {
     RouteRequest request = new RouteRequest();
-    request.setDateTime(dateTime);
+    request.setDateTime(instant);
     request.setFrom(from);
     request.setTo(to);
+    request.setArriveBy(arriveBy);
     request.journey().direct().setMode(StreetMode.WALK);
 
     try (