Repository: graphhopper/graphhopper (java)

Issue:
Better handling for missing ferry duration tags
If there is no duration tag on a ferry we currently use `speed=5km/h`, or even smaller if the distance is below 300m. But for longer distance ferries this seems very likely too slow. Also I think not tagging duration for such longer ferries is a mapping error and if this happens frequently we should report it to OSM.

Also see here: https://github.com/graphhopper/graphhopper/pull/2528/files#r819521430

Pull request:
Improve handling of short ferries without duration tag
In case there is a duration tag we need to use the distance of the entire way to calculate the speed (see #2528), but when it is missing we should actually use the distance of the edge instead, because there are some ferry ways that visit multiple places like this one: https://www.openstreetmap.org/way/996639224. There are also ferry ways that cross each other. In principle this could mean that a 'long' ferry is split into two 'shorter' parts for which we apply a slower speed (or will apply a slower speed when we fix #2532). But in reality this seems to only happen close to the harbour, so a long ferry really only might be split into a (still) long edge and a (very) short edge. In this case it would be fine to determine the speed for each edge separately, based on the distance of the edge not the entire way. Therefore, I now use the edge distance to determine the speed in case the duration tag is missing. So actually the same as before #2528, but different to what we did before #2457.

The other thing I changed here is I increase the distance limit under which we use the minimum speed for short ferries. Previously we used 300m, but now I use 500m, because since #2528 we use the real distance, not the beeline distance and this can be larger for curved ferry ways. One example is this ferry: https://www.openstreetmap.org/way/3549182 that otherwise would be (much) faster as the beeline distance was below 300m but the actual distance is above 300m. This would change the speed from `speedFactor/2` to `unknownSpeed` (could be a change from 2km/h to 6km/h, so three times faster, if `speedFactor` was 2 for example). The idea here is that we rather use the lowest possible speed for such short ferries so they are more likely to be avoided.


Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/util/FerrySpeedCalculator.java b/core/src/main/java/com/graphhopper/routing/util/FerrySpeedCalculator.java
index cec5ee99c..b37c7ffa6 100644
--- a/core/src/main/java/com/graphhopper/routing/util/FerrySpeedCalculator.java
+++ b/core/src/main/java/com/graphhopper/routing/util/FerrySpeedCalculator.java
@@ -27,12 +27,14 @@ public class FerrySpeedCalculator {
         } else {
             // we have no speed value to work with because there was no valid duration tag.
             // we have to take a guess based on the distance.
-            double wayDistance = way.getTag("way_distance", Double.NaN);
+            double wayDistance = way.getTag("edge_distance", Double.NaN);
             if (Double.isNaN(wayDistance))
-                // we don't know the distance of this way either. probably a broken way at the border of the map.
-                return unknownSpeed;
-            else if (wayDistance < 300)
-                // use the slowest possible speed for very short ferries
+                throw new IllegalStateException("No 'edge_distance' set for edge created for way: " + way.getId());
+            else if (wayDistance < 500)
+                // Use the slowest possible speed for very short ferries. Note that sometimes these aren't really ferries
+                // that take you from one harbour to another, but rather ways that only represent the beginning of a
+                // longer ferry connection and that are used by multiple different connections, like here: https://www.openstreetmap.org/way/107913687
+                // It should not matter much which speed we use in this case, so we have no special handling for these.
                 return minSpeed;
             else {
                 // todo: distinguish speed based on the distance of the ferry, see #2532


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/util/CarFlagEncoderTest.java b/core/src/test/java/com/graphhopper/routing/util/CarFlagEncoderTest.java
index c20577093..bdc6d48e0 100644
--- a/core/src/test/java/com/graphhopper/routing/util/CarFlagEncoderTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/CarFlagEncoderTest.java
@@ -489,7 +489,7 @@ public class CarFlagEncoderTest {
         way = new ReaderWay(1);
         way.setTag("route", "ferry");
         way.setTag("motorcar", "yes");
-        way.setTag("way_distance", 100.0);
+        way.setTag("edge_distance", 100.0);
         // accept
         assertTrue(encoder.getAccess(way).isFerry());
         encoder.handleWayTags(edgeFlags, way);
@@ -644,7 +644,7 @@ public class CarFlagEncoderTest {
     public void testIssue_1256() {
         ReaderWay way = new ReaderWay(1);
         way.setTag("route", "ferry");
-        way.setTag("way_distance", 257.0);
+        way.setTag("edge_distance", 257.0);
 
         // default is 5km/h minimum speed for car
         IntsRef edgeFlags = em.createEdgeFlags();
diff --git a/core/src/test/java/com/graphhopper/routing/util/FerrySpeedCalculatorTest.java b/core/src/test/java/com/graphhopper/routing/util/FerrySpeedCalculatorTest.java
index da6e7b7c5..14023c1d8 100644
--- a/core/src/test/java/com/graphhopper/routing/util/FerrySpeedCalculatorTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/FerrySpeedCalculatorTest.java
@@ -22,6 +22,7 @@ import com.graphhopper.reader.ReaderWay;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 class FerrySpeedCalculatorTest {
 
@@ -32,7 +33,7 @@ class FerrySpeedCalculatorTest {
         double unknownSpeed = 5;
         FerrySpeedCalculator c = new FerrySpeedCalculator(minSpeed, maxSpeed, unknownSpeed);
 
-        // speed_from_duration is set (way_distance is not even needed)
+        // speed_from_duration is set (edge_distance is not even needed)
         checkSpeed(c, 30.0, null, Math.round(30 / 1.4));
         checkSpeed(c, 45.0, null, Math.round(45 / 1.4));
         // above max (when including waiting time) (capped to max)
@@ -40,23 +41,22 @@ class FerrySpeedCalculatorTest {
         // below smallest storable non-zero value
         checkSpeed(c, 0.5, null, minSpeed);
 
-        // no speed_from_duration, but way_distance is present
+        // no speed_from_duration, but edge_distance is present
         // minimum speed for short ferries
         checkSpeed(c, null, 100.0, minSpeed);
         // unknown speed for longer ones
         checkSpeed(c, null, 1000.0, unknownSpeed);
 
-        // no speed, no distance -> unknown. this can also happen when the duration tag is valid, but no distance could
-        // be calculated (-> no speed)
-        checkSpeed(c, null, null, unknownSpeed);
+        // no speed, no distance -> error. this should never happen as we always set the edge distance.
+        assertThrows(IllegalStateException.class, () -> checkSpeed(c, null, null, unknownSpeed));
     }
 
-    private void checkSpeed(FerrySpeedCalculator calc, Double speedFromDuration, Double wayDistance, double expected) {
+    private void checkSpeed(FerrySpeedCalculator calc, Double speedFromDuration, Double edgeDistance, double expected) {
         ReaderWay way = new ReaderWay(0L);
         if (speedFromDuration != null)
             way.setTag("speed_from_duration", speedFromDuration);
-        if (wayDistance != null)
-            way.setTag("way_distance", wayDistance);
+        if (edgeDistance != null)
+            way.setTag("edge_distance", edgeDistance);
         assertEquals(expected, calc.getSpeed(way));
     }
 
diff --git a/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java b/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java
index 8d4f5476a..a8a505663 100644
--- a/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/FootFlagEncoderTest.java
@@ -250,7 +250,7 @@ public class FootFlagEncoderTest {
         ReaderWay way = new ReaderWay(1);
         way.setTag("route", "ferry");
         way.setTag("duration:seconds", 1800L);
-        way.setTag("way_distance", 30000.0);
+        way.setTag("edge_distance", 30000.0);
         way.setTag("speed_from_duration", 30 / 0.5);
         // the speed is truncated to maxspeed (=15)
         IntsRef edgeFlags = encodingManager.createEdgeFlags();