AverageSpeedDetails: avoid exception for short edges (#1871)

* AverageSpeedDetails: avoid exception for short edges, fixes #1848 * add comment for this limitation Co-authored-by: Andi <easbar.mail@posteo.net>
graphhopper · Jan 28, 2020 · 9791b3f · 9791b3f
1 parent 4582ee0
commit 9791b3f
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 9 deletions.
diff --git a/core/src/main/java/com/graphhopper/storage/BaseGraph.java b/core/src/main/java/com/graphhopper/storage/BaseGraph.java
@@ -50,8 +50,9 @@
  * loadExisting, (4) usage, (5) flush, (6) close
  */
 class BaseGraph implements Graph {
-    // currently distances are stored as 4 byte integers. using a conversion factor of 1000 the minimum distance
-    // that is not considered zero is 0.0005m (=0.5mm) and the maximum distance per edge is about 2.147.483m=2147km
+    // Currently distances are stored as 4 byte integers. using a conversion factor of 1000 the minimum distance
+    // that is not considered zero is 0.0005m (=0.5mm) and the maximum distance per edge is about 2.147.483m=2147km.
+    // See OSMReader.addEdge and #1871.
     private static final double INT_DIST_FACTOR = 1000d;
     static double MAX_DIST = Integer.MAX_VALUE / INT_DIST_FACTOR;
 

diff --git a/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java b/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java
@@ -10,7 +10,7 @@ public class AverageSpeedDetails extends AbstractPathDetailsBuilder {
 
     private final Weighting weighting;
     private final double precision;
-    private double decimalValue = -1;
+    private Double decimalValue;
     // will include the turn time penalty
     private int prevEdgeId = -1;
 
@@ -35,12 +35,21 @@ protected Object getCurrentValue() {
 
     @Override
     public boolean isEdgeDifferentToLastEdge(EdgeIteratorState edge) {
-        double tmpVal = edge.getDistance() / GHUtility.calcMillisWithTurnMillis(weighting, edge, false, prevEdgeId) * 3600;
-        if (Double.isInfinite(tmpVal))
-            throw new IllegalStateException("average_speed was infinite for " + edge.fetchWayGeometry(3));
+        // for very short edges we might not be able calculate a proper value for speed. dividing by calcMillis can
+        // even lead to speed=Infinity -> just ignore these cases here, see #1848
+        final double distance = edge.getDistance();
+        if (distance < 0.1) {
+            if (decimalValue != null)
+                return false;
+
+            // in case this is the first edge we have to return some value
+            decimalValue = null;
+            return true;
+        }
 
+        double tmpVal = distance / GHUtility.calcMillisWithTurnMillis(weighting, edge, false, prevEdgeId) * 3600;
         prevEdgeId = edge.getEdge();
-        if (Math.abs(tmpVal - decimalValue) >= precision) {
+        if (decimalValue == null || Math.abs(tmpVal - decimalValue) >= precision) {
             this.decimalValue = Math.round(tmpVal / precision) * precision;
             return true;
         }

diff --git a/core/src/test/java/com/graphhopper/routing/PathTest.java b/core/src/test/java/com/graphhopper/routing/PathTest.java
@@ -316,6 +316,28 @@ public void testCalcAverageSpeedDetails() {
         assertEquals(4, averageSpeedDetails.get(3).getLast());
     }
 
+    @Test
+    public void testCalcAverageSpeedDetailsWithShortDistances_issue1848() {
+        ShortestWeighting weighting = new ShortestWeighting(encoder);
+        Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 6);
+        assertTrue(p.isFound());
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(AVERAGE_SPEED), new PathDetailsBuilderFactory(), 0);
+        assertTrue(details.size() == 1);
+        List<PathDetail> averageSpeedDetails = details.get(AVERAGE_SPEED);
+        assertEquals(4, averageSpeedDetails.size());
+
+        // reverse path includes 'null' value as first
+        p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(6, 1);
+        assertTrue(p.isFound());
+        details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(AVERAGE_SPEED), new PathDetailsBuilderFactory(), 0);
+        assertTrue(details.size() == 1);
+        averageSpeedDetails = details.get(AVERAGE_SPEED);
+        assertEquals(5, averageSpeedDetails.size());
+        assertNull(averageSpeedDetails.get(0).getValue());
+    }
+
     @Test
     public void testCalcStreetNameDetails() {
         ShortestWeighting weighting = new ShortestWeighting(encoder);
@@ -924,6 +946,7 @@ private Graph generatePathDetailsGraph() {
         na.setNode(3, 52.514, 13.350);
         na.setNode(4, 52.515, 13.349);
         na.setNode(5, 52.516, 13.3452);
+        na.setNode(6, 52.516, 13.344);
 
         ReaderWay w = new ReaderWay(1);
         w.setTag("highway", "tertiary");
@@ -946,6 +969,9 @@ private Graph generatePathDetailsGraph() {
         tmpEdge = g.edge(3, 4, 10, true).setName("3-4");
         tmpEdge.setFlags(carManager.handleWayTags(w, map, relFlags));
 
+        tmpEdge = g.edge(5, 6, 0.01, true).setName("3-4");
+        tmpEdge.setFlags(carManager.handleWayTags(w, map, relFlags));
+
         return g;
     }
 

diff --git a/reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java b/reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java
@@ -698,11 +698,11 @@ EdgeIteratorState addEdge(int fromIndex, int toIndex, PointList pointList, IntsR
                     pillarNodes.add(lat, lon);
             }
         }
-        if (towerNodeDistance < 0.0001) {
+        if (towerNodeDistance < 0.001) {
             // As investigation shows often two paths should have crossed via one identical point 
             // but end up in two very close points.
             zeroCounter++;
-            towerNodeDistance = 0.0001;
+            towerNodeDistance = 0.001;
         }
 
         double maxDistance = (Integer.MAX_VALUE - 1) / 1000d;