diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java index 86202687dbd..df733c0aa1b 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java @@ -1,6 +1,5 @@ package org.opentripplanner.routing.algorithm.raptoradapter.transit; -import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -18,17 +17,21 @@ public class Transfer { /** - * Since transfers costs are not computed through a full A* they can incur an absurdly high - * cost that overflows the integer cost inside raptor. + * Since transfers costs are not computed through a full A* with pruning they can incur an + * absurdly high cost that overflows the integer cost inside RAPTOR + * (https://github.com/opentripplanner/OpenTripPlanner/issues/5509). *

* An example would be a transfer using lots of stairs being used on a wheelchair when no * wheelchair-specific one has been generated. * (see https://docs.opentripplanner.org/en/dev-2.x/Accessibility/). *

- * For this reason there is this sanit limit that make sure that the transfer cost stays inside a - * bound that is still very high but far away from the integer overflow. + * For this reason there is this sanity limit that makes sure that the transfer cost stays below a + * limit is still very high (several days of transit-equivalent cost) but far away from the + * integer overflow. + * + * @see EdgeTraverser */ - private final double MAX_TRANSFER_COST = Duration.ofDays(3).toSeconds(); + private static final int MAX_TRANSFER_COST = Integer.MAX_VALUE / 100 / 30; private final int toStop; @@ -78,9 +81,7 @@ public Optional asRaptorTransfer(StreetSearchRequest request) { if (edges == null || edges.isEmpty()) { double durationSeconds = distanceMeters / walkPreferences.speed(); final double domainCost = durationSeconds * walkPreferences.reluctance(); - if (domainCost > MAX_TRANSFER_COST) { - return Optional.empty(); - } else { + if (domainCost < MAX_TRANSFER_COST) { return Optional.of( new DefaultRaptorTransfer( this.toStop, @@ -89,6 +90,8 @@ public Optional asRaptorTransfer(StreetSearchRequest request) { this ) ); + } else { + return Optional.empty(); } } @@ -99,15 +102,14 @@ public Optional asRaptorTransfer(StreetSearchRequest request) { return state .filter(s -> s.weight < MAX_TRANSFER_COST) - .map(s -> { - final int raptorCost = RaptorCostConverter.toRaptorCost(s.getWeight()); - return new DefaultRaptorTransfer( + .map(s -> + new DefaultRaptorTransfer( this.toStop, (int) s.getElapsedTimeSeconds(), - raptorCost, + RaptorCostConverter.toRaptorCost(s.getWeight()), this - ); - }); + ) + ); } @Override diff --git a/src/test/java/org/opentripplanner/_support/geometry/Coordinates.java b/src/test/java/org/opentripplanner/_support/geometry/Coordinates.java index bfa8e0da94e..f345b7095ed 100644 --- a/src/test/java/org/opentripplanner/_support/geometry/Coordinates.java +++ b/src/test/java/org/opentripplanner/_support/geometry/Coordinates.java @@ -5,6 +5,7 @@ public class Coordinates { public static final Coordinate BERLIN = new Coordinate(52.5212, 13.4105); + public static final Coordinate BERLIN_BRANDENBURG_GATE = new Coordinate(52.51627, 13.37770); public static final Coordinate HAMBURG = new Coordinate(53.5566, 10.0003); public static final Coordinate KONGSBERG_PLATFORM_1 = new Coordinate(59.67216, 9.65107); } diff --git a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/TransferTest.java b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/TransferTest.java new file mode 100644 index 00000000000..204cae8eac6 --- /dev/null +++ b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/TransferTest.java @@ -0,0 +1,73 @@ +package org.opentripplanner.routing.algorithm.raptoradapter.transit; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.opentripplanner.street.model._data.StreetModelForTest.intersectionVertex; + +import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.opentripplanner._support.geometry.Coordinates; +import org.opentripplanner.raptor.api.model.RaptorTransfer; +import org.opentripplanner.street.model._data.StreetModelForTest; +import org.opentripplanner.street.model.vertex.IntersectionVertex; +import org.opentripplanner.street.search.request.StreetSearchRequest; + +class TransferTest { + + private static final IntersectionVertex BERLIN_V = intersectionVertex(Coordinates.BERLIN); + private static final IntersectionVertex BRANDENBURG_GATE_V = intersectionVertex( + Coordinates.BERLIN_BRANDENBURG_GATE + ); + private static final IntersectionVertex KONGSBERG_V = intersectionVertex( + Coordinates.KONGSBERG_PLATFORM_1 + ); + + @Nested + class WithEdges { + + @Test + void limitMaxCost() { + // very long edge from Berlin to Kongsberg, Norway that has of course a huge cost to traverse + var edge = StreetModelForTest.streetEdge(BERLIN_V, KONGSBERG_V); + + var veryLongTransfer = new Transfer(0, List.of(edge)); + assertTrue(veryLongTransfer.getDistanceMeters() > 800_000); + // cost would be too high, so it should not be included in RAPTOR search + assertTrue(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).isEmpty()); + } + + @Test + void allowLowCost() { + var edge = StreetModelForTest.streetEdge(BERLIN_V, BRANDENBURG_GATE_V); + var transfer = new Transfer(0, List.of(edge)); + assertTrue(transfer.getDistanceMeters() < 4000); + final Optional raptorTransfer = transfer.asRaptorTransfer( + StreetSearchRequest.of().build() + ); + // cost is below max limit and should be included in RAPTOR + assertTrue(raptorTransfer.isPresent()); + } + } + + @Nested + class WithoutEdges { + + @Test + void limitMaxCost() { + var veryLongTransfer = new Transfer(0, 800_000); + // cost would be too high, so it should not be included in RAPTOR search + assertTrue(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).isEmpty()); + } + + @Test + void allowLowCost() { + var transfer = new Transfer(0, 200); + final Optional raptorTransfer = transfer.asRaptorTransfer( + StreetSearchRequest.of().build() + ); + // cost is below max limit and should be included in RAPTOR + assertTrue(raptorTransfer.isPresent()); + } + } +}