Repository: graphhopper/graphhopper (java)

Issue:
Speed for AverageSpeedDetails should depend on Weighting
Currently the average speed stored in the edge is used, but when flexible mode is used the speed is calculated on the fly.

So we should somehow get rid of `encoder.getAccessEnc()` (weighting-dependent too!) and `encoder.getAverageSpeed()`.

For AverageSpeedDetails we must use Weighting.calcMillis and the edge distance to calculate the more precise weighting-dependent speed. (change FlagEncoder to Weighting in PathDetailsBuilderFactory)

~~In InstructionsOutgoingEdges we should get rid of the speed dependency. With #1548 this should be doable using the EncodedValues `RoadEnvironment` and `RoadClass`.~~ The refactoring of InstructionsOutgoingEdges should be done in #990.

Pull request description:
remove FlagEncoder from PathDetailsBuilderFactory
Fixes #1551

Patch:
diff --git a/core/src/main/java/com/graphhopper/util/PathMerger.java b/core/src/main/java/com/graphhopper/util/PathMerger.java
index 4b1651b48..5fab95154 100644
--- a/core/src/main/java/com/graphhopper/util/PathMerger.java
+++ b/core/src/main/java/com/graphhopper/util/PathMerger.java
@@ -137,7 +137,7 @@ public class PathMerger {
                 }
 
                 fullPoints.add(tmpPoints);
-                altRsp.addPathDetails(PathDetailsFromEdges.calcDetails(path, weighting, requestedPathDetails, pathBuilderFactory, origPoints));
+                altRsp.addPathDetails(PathDetailsFromEdges.calcDetails(path, encodingManager, weighting, requestedPathDetails, pathBuilderFactory, origPoints));
                 origPoints = fullPoints.size();
             }
 
diff --git a/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java b/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java
new file mode 100644
index 000000000..0f64e13b4
--- /dev/null
+++ b/core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java
@@ -0,0 +1,40 @@
+package com.graphhopper.util.details;
+
+import com.graphhopper.routing.weighting.Weighting;
+import com.graphhopper.util.EdgeIteratorState;
+import com.graphhopper.util.Helper;
+
+import static com.graphhopper.util.Parameters.Details.AVERAGE_SPEED;
+
+public class AverageSpeedDetails extends AbstractPathDetailsBuilder {
+
+    private final Weighting weighting;
+    private double decimalValue = -1;
+    // will include the turn time penalty
+    private int prevEdgeId = -1;
+
+    public AverageSpeedDetails(Weighting weighting) {
+        super(AVERAGE_SPEED);
+        this.weighting = weighting;
+    }
+
+    @Override
+    protected Object getCurrentValue() {
+        if (Double.isInfinite(decimalValue))
+            throw new IllegalStateException("average_speed must not be infinite");
+
+        return decimalValue;
+    }
+
+    @Override
+    public boolean isEdgeDifferentToLastEdge(EdgeIteratorState edge) {
+        double tmpVal = edge.getDistance() / weighting.calcMillis(edge, false, prevEdgeId) * 3600;
+        prevEdgeId = edge.getEdge();
+        // avoid creating too many path details => round the speed to 0.01 precision and include it only if:
+        if (Math.abs(tmpVal - decimalValue) >= 0.1) {
+            this.decimalValue = Helper.round2(tmpVal);
+            return true;
+        }
+        return false;
+    }
+}
diff --git a/core/src/main/java/com/graphhopper/util/details/PathDetailsBuilderFactory.java b/core/src/main/java/com/graphhopper/util/details/PathDetailsBuilderFactory.java
index a6aff6d77..84e71b5fd 100644
--- a/core/src/main/java/com/graphhopper/util/details/PathDetailsBuilderFactory.java
+++ b/core/src/main/java/com/graphhopper/util/details/PathDetailsBuilderFactory.java
@@ -19,7 +19,7 @@ package com.graphhopper.util.details;
 
 import com.graphhopper.coll.MapEntry;
 import com.graphhopper.routing.profiles.*;
-import com.graphhopper.routing.util.FlagEncoder;
+import com.graphhopper.routing.util.EncodingManager;
 import com.graphhopper.routing.weighting.Weighting;
 
 import java.util.ArrayList;
@@ -37,11 +37,11 @@ import static com.graphhopper.util.Parameters.Details.*;
  */
 public class PathDetailsBuilderFactory {
 
-    public List<PathDetailsBuilder> createPathDetailsBuilders(List<String> requestedPathDetails, FlagEncoder encoder, Weighting weighting) {
+    public List<PathDetailsBuilder> createPathDetailsBuilders(List<String> requestedPathDetails, EncodingManager em, Weighting weighting) {
         List<PathDetailsBuilder> builders = new ArrayList<>();
 
         if (requestedPathDetails.contains(AVERAGE_SPEED))
-            builders.add(new DecimalDetails(AVERAGE_SPEED, encoder.getAverageSpeedEnc()));
+            builders.add(new AverageSpeedDetails(weighting));
 
         if (requestedPathDetails.contains(STREET_NAME))
             builders.add(new StreetNameDetails());
@@ -59,14 +59,14 @@ public class PathDetailsBuilderFactory {
             builders.add(new DistanceDetails());
 
         for (String checkSuffix : requestedPathDetails) {
-            if (checkSuffix.contains(getKey("", "priority")) && encoder.hasEncodedValue(checkSuffix))
-                builders.add(new DecimalDetails(checkSuffix, encoder.getDecimalEncodedValue(checkSuffix)));
+            if (checkSuffix.contains(getKey("", "priority")) && em.hasEncodedValue(checkSuffix))
+                builders.add(new DecimalDetails(checkSuffix, em.getDecimalEncodedValue(checkSuffix)));
         }
 
         for (String key : Arrays.asList(MaxSpeed.KEY, MaxWidth.KEY, MaxHeight.KEY, MaxWeight.KEY,
                 MaxAxleLoad.KEY, MaxLength.KEY)) {
-            if (requestedPathDetails.contains(key) && encoder.hasEncodedValue(key))
-                builders.add(new DecimalDetails(key, encoder.getDecimalEncodedValue(key)));
+            if (requestedPathDetails.contains(key) && em.hasEncodedValue(key))
+                builders.add(new DecimalDetails(key, em.getDecimalEncodedValue(key)));
         }
 
         for (Map.Entry entry : Arrays.asList(new MapEntry<>(RoadClass.KEY, RoadClass.class),
@@ -76,8 +76,8 @@ public class PathDetailsBuilderFactory {
                 new MapEntry<>(HazmatTunnel.KEY, HazmatTunnel.class), new MapEntry<>(HazmatWater.KEY, HazmatWater.class),
                 new MapEntry<>(Country.KEY, Country.class))) {
             String key = (String) entry.getKey();
-            if (requestedPathDetails.contains(key) && encoder.hasEncodedValue(key))
-                builders.add(new EnumDetails(key, encoder.getEnumEncodedValue(key, (Class<Enum>) entry.getValue())));
+            if (requestedPathDetails.contains(key) && em.hasEncodedValue(key))
+                builders.add(new EnumDetails(key, em.getEnumEncodedValue(key, (Class<Enum>) entry.getValue())));
         }
 
         if (requestedPathDetails.size() != builders.size()) {
diff --git a/core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java b/core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java
index 5cce01d92..0cc0ca688 100644
--- a/core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java
+++ b/core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java
@@ -18,6 +18,7 @@
 package com.graphhopper.util.details;
 
 import com.graphhopper.routing.Path;
+import com.graphhopper.routing.util.EncodingManager;
 import com.graphhopper.routing.weighting.Weighting;
 import com.graphhopper.util.EdgeIteratorState;
 
@@ -54,10 +55,11 @@ public class PathDetailsFromEdges implements Path.EdgeVisitor {
      * @param pathBuilderFactory Generates the relevant PathBuilders
      * @return List of PathDetails for this Path
      */
-    public static Map<String, List<PathDetail>> calcDetails(Path path, Weighting weighting, List<String> requestedPathDetails, PathDetailsBuilderFactory pathBuilderFactory, int previousIndex) {
+    public static Map<String, List<PathDetail>> calcDetails(Path path, EncodingManager encodingManager, Weighting weighting,
+                                                            List<String> requestedPathDetails, PathDetailsBuilderFactory pathBuilderFactory, int previousIndex) {
         if (!path.isFound() || requestedPathDetails.isEmpty())
             return Collections.emptyMap();
-        List<PathDetailsBuilder> pathBuilders = pathBuilderFactory.createPathDetailsBuilders(requestedPathDetails, weighting.getFlagEncoder(), weighting);
+        List<PathDetailsBuilder> pathBuilders = pathBuilderFactory.createPathDetailsBuilders(requestedPathDetails, encodingManager, weighting);
         if (pathBuilders.isEmpty())
             return Collections.emptyMap();
 
diff --git a/core/src/main/java/com/graphhopper/util/details/TimeDetails.java b/core/src/main/java/com/graphhopper/util/details/TimeDetails.java
index 3ebb21f51..f15da3755 100644
--- a/core/src/main/java/com/graphhopper/util/details/TimeDetails.java
+++ b/core/src/main/java/com/graphhopper/util/details/TimeDetails.java
@@ -30,8 +30,8 @@ import static com.graphhopper.util.Parameters.Details.TIME;
 public class TimeDetails extends AbstractPathDetailsBuilder {
 
     private final Weighting weighting;
-
-    private int edgeId = -1;
+    private int prevEdgeId = -1;
+    // will include the turn time penalty
     private long time = 0;
 
     public TimeDetails(Weighting weighting) {
@@ -41,9 +41,9 @@ public class TimeDetails extends AbstractPathDetailsBuilder {
 
     @Override
     public boolean isEdgeDifferentToLastEdge(EdgeIteratorState edge) {
-        if (edge.getEdge() != edgeId) {
-            edgeId = edge.getEdge();
-            time = weighting.calcMillis(edge, false, edgeId);
+        if (edge.getEdge() != prevEdgeId) {
+            time = weighting.calcMillis(edge, false, prevEdgeId);
+            prevEdgeId = edge.getEdge();
             return true;
         }
         return false;


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/PathTest.java b/core/src/test/java/com/graphhopper/routing/PathTest.java
index 022600d5d..c787cebfc 100644
--- a/core/src/test/java/com/graphhopper/routing/PathTest.java
+++ b/core/src/test/java/com/graphhopper/routing/PathTest.java
@@ -306,7 +306,8 @@ public class PathTest {
         Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 5);
         assertTrue(p.isFound());
 
-        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, weighting, Arrays.asList(new String[]{AVERAGE_SPEED}), new PathDetailsBuilderFactory(), 0);
+        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);
@@ -329,7 +330,8 @@ public class PathTest {
         Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 5);
         assertTrue(p.isFound());
 
-        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, weighting, Arrays.asList(new String[]{STREET_NAME}), new PathDetailsBuilderFactory(), 0);
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(STREET_NAME), new PathDetailsBuilderFactory(), 0);
         assertTrue(details.size() == 1);
 
         List<PathDetail> streetNameDetails = details.get(STREET_NAME);
@@ -354,7 +356,8 @@ public class PathTest {
         Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 5);
         assertTrue(p.isFound());
 
-        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, weighting, Arrays.asList(new String[]{EDGE_ID}), new PathDetailsBuilderFactory(), 0);
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(EDGE_ID), new PathDetailsBuilderFactory(), 0);
         assertTrue(details.size() == 1);
 
         List<PathDetail> edgeIdDetails = details.get(EDGE_ID);
@@ -378,7 +381,8 @@ public class PathTest {
         Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 5);
         assertTrue(p.isFound());
 
-        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, weighting, Arrays.asList(new String[]{TIME}), new PathDetailsBuilderFactory(), 0);
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(TIME), new PathDetailsBuilderFactory(), 0);
         assertTrue(details.size() == 1);
 
         List<PathDetail> timeDetails = details.get(TIME);
@@ -401,7 +405,8 @@ public class PathTest {
         Path p = new Dijkstra(pathDetailGraph, weighting, TraversalMode.NODE_BASED).calcPath(1, 5);
         assertTrue(p.isFound());
 
-        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, weighting, Arrays.asList(new String[]{DISTANCE}), new PathDetailsBuilderFactory(), 0);
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(DISTANCE), new PathDetailsBuilderFactory(), 0);
         assertTrue(details.size() == 1);
 
         List<PathDetail> distanceDetails = details.get(DISTANCE);
diff --git a/core/src/test/java/com/graphhopper/util/PathSimplificationTest.java b/core/src/test/java/com/graphhopper/util/PathSimplificationTest.java
index be79d1ae3..1cf591ab8 100644
--- a/core/src/test/java/com/graphhopper/util/PathSimplificationTest.java
+++ b/core/src/test/java/com/graphhopper/util/PathSimplificationTest.java
@@ -31,7 +31,6 @@ import com.graphhopper.routing.weighting.ShortestWeighting;
 import com.graphhopper.storage.Graph;
 import com.graphhopper.storage.GraphBuilder;
 import com.graphhopper.storage.NodeAccess;
-import com.graphhopper.util.Parameters.Details;
 import com.graphhopper.util.details.PathDetail;
 import com.graphhopper.util.details.PathDetailsBuilderFactory;
 import com.graphhopper.util.details.PathDetailsFromEdges;
@@ -40,6 +39,7 @@ import org.junit.Test;
 
 import java.util.*;
 
+import static com.graphhopper.util.Parameters.Details.AVERAGE_SPEED;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
@@ -140,7 +140,8 @@ public class PathSimplificationTest {
         ShortestWeighting weighting = new ShortestWeighting(carEncoder);
         Path p = new Dijkstra(g, weighting, tMode).calcPath(0, 10);
         InstructionList wayList = InstructionsFromEdges.calcInstructions(p, g, weighting, carManager.getBooleanEncodedValue(Roundabout.KEY), usTR);
-        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, weighting, Arrays.asList(Details.AVERAGE_SPEED), new PathDetailsBuilderFactory(), 0);
+        Map<String, List<PathDetail>> details = PathDetailsFromEdges.calcDetails(p, carManager, weighting,
+                Arrays.asList(AVERAGE_SPEED), new PathDetailsBuilderFactory(), 0);
 
         PathWrapper pathWrapper = new PathWrapper();
         pathWrapper.setInstructions(wayList);
diff --git a/web/src/test/java/com/graphhopper/http/resources/RouteResourceTest.java b/web/src/test/java/com/graphhopper/http/resources/RouteResourceTest.java
index 6008f8a02..9316dc6d6 100644
--- a/web/src/test/java/com/graphhopper/http/resources/RouteResourceTest.java
+++ b/web/src/test/java/com/graphhopper/http/resources/RouteResourceTest.java
@@ -221,10 +221,10 @@ public class RouteResourceTest {
         assertTrue(pathDetails.containsKey("edge_id"));
         assertTrue(pathDetails.containsKey("time"));
         List<PathDetail> averageSpeedList = pathDetails.get("average_speed");
-        assertEquals(9, averageSpeedList.size());
-        assertEquals(30.0, averageSpeedList.get(0).getValue());
+        assertEquals(13, averageSpeedList.size());
+        assertEquals(30.00, averageSpeedList.get(0).getValue());
         assertEquals(14, averageSpeedList.get(0).getLength());
-        assertEquals(60.0, averageSpeedList.get(1).getValue());
+        assertEquals(60.06, averageSpeedList.get(1).getValue());
         assertEquals(5, averageSpeedList.get(1).getLength());
 
         List<PathDetail> edgeIdDetails = pathDetails.get("edge_id");
@@ -277,9 +277,9 @@ public class RouteResourceTest {
         JsonNode details = path.get("details");
         assertTrue(details.has("average_speed"));
         JsonNode averageSpeed = details.get("average_speed");
-        assertEquals(30.0, averageSpeed.get(0).get(2).asDouble(), .01);
+        assertEquals(30.00, averageSpeed.get(0).get(2).asDouble(), .01);
         assertEquals(14, averageSpeed.get(0).get(1).asInt());
-        assertEquals(60.0, averageSpeed.get(1).get(2).asDouble(), .01);
+        assertEquals(60.06, averageSpeed.get(1).get(2).asDouble(), .01);
         assertEquals(19, averageSpeed.get(1).get(1).asInt());
         assertTrue(details.has("edge_id"));
         JsonNode edgeIds = details.get("edge_id");