GPX export: avoid adding rounding errors and make it easier to calculate the time via passing the nextlat,nextlon; fix to Directory.remove

Author: Peter

core/src/main/java/com/graphhopper/GraphHopper.java

@@ -479,6 +479,9 @@ public GraphHopper importOrLoad()
     {
         if (!load(ghLocation))
         {
+            if (!new File(osmFile).exists())
+                throw new IllegalStateException("Couldn't load from existing folder: " + ghLocation
+                        + " but also cannot import from OSM file as it does not exist: " + osmFile);
             printInfo();
             process(ghLocation, osmFile);
         } else

core/src/main/java/com/graphhopper/routing/Path.java

@@ -471,16 +471,16 @@ private void updatePointsAndInstruction( EdgeIteratorState edge, PointList pl )
                 // add points in opposite direction as adj node is previous
                 // skip base point => 'i > 0'
                 int len = pl.size() - 1;
-                long flags = edge.getFlags();
                 for (int i = len; i > 0; i--)
                 {
                     double lat = pl.getLatitude(i);
                     double lon = pl.getLongitude(i);
                     points.add(lat, lon);
                 }
-                double dist = edge.getDistance();
-                prevInstruction.setDistance(dist + prevInstruction.getDistance());
-                prevInstruction.setMillis(calcMillis(dist, flags) + prevInstruction.getMillis());
+                double newDist = edge.getDistance();
+                prevInstruction.setDistance(newDist + prevInstruction.getDistance());
+                long flags = edge.getFlags();
+                prevInstruction.setMillis(calcMillis(newDist, flags) + prevInstruction.getMillis());
             }
         });
 

core/src/main/java/com/graphhopper/storage/GHDirectory.java

@@ -156,7 +156,8 @@ void removeByName( String name )
         if (da == null)
             throw new IllegalStateException("Couldn't remove dataAccess object:" + name);
         da.close();
-        Helper.removeDir(new File(location + name));
+        if (da.getType().isStoring())
+            Helper.removeDir(new File(location + name));
     }
 
     @Override

core/src/main/java/com/graphhopper/util/Instruction.java

@@ -34,14 +34,13 @@ public class Instruction
     private final String name;
     private double distance;
     private long millis;
-    private final PointList points;
+    final PointList points;
     private final int pavementType;
     private final int waytype;
 
     /**
      * The points, distances and times have exactly the same count. The last point of this
-     * instruction is not duplicated here and should be in the next one. The first distance and time
-     * entries are measured between the first point and the second one etc.
+     * instruction is not duplicated here and should be in the next one.
      */
     public Instruction( int indication, String name, int waytype, int pavementType, PointList pl )
     {
@@ -94,7 +93,7 @@ public Instruction setMillis( long millis )
         this.millis = millis;
         return this;
     }
-    
+
     /**
      * Time in millis until no new instruction
      */
@@ -135,27 +134,26 @@ public long getMillis()
      * <p>
      * @return the time offset to add for the next instruction
      */
-    public long fillGPXList( List<GPXEntry> list, long time, double prevFactor, double prevLat, double prevLon )
+    long fillGPXList( List<GPXEntry> list, long time, double nextInstrLat, double nextInstrLon )
     {
+        checkOne();
         int len = points.size();
+        long prevTime = time;
+        double lat = points.getLatitude(0);
+        double lon = points.getLongitude(0);
         for (int i = 0; i < len; i++)
         {
-            double lat = points.getLatitude(i);
-            double lon = points.getLongitude(i);
-            if (!Double.isNaN(prevLat))
-            {
-                // Here we assume that the same speed is used until the next instruction.
-                // If we would calculate all the distances (and times) up front there
-                // would be a problem where the air-line distance is not the distance returned from the edge
-                // e.g. in the case if we include elevation data                
-                time += distanceCalc.calcDist(prevLat, prevLon, lat, lon) / prevFactor;
-            }
-            list.add(new GPXEntry(lat, lon, time));
-            prevFactor = distance / millis;
-            prevLat = lat;
-            prevLon = lon;
+            boolean last = i + 1 == len;
+            double nextLat = last ? nextInstrLat : points.getLatitude(i + 1);
+            double nextLon = last ? nextInstrLon : points.getLongitude(i + 1);
+
+            list.add(new GPXEntry(lat, lon, prevTime));
+            // TODO in the case of elevation data the air-line distance is probably not precise enough
+            prevTime = Math.round(prevTime + millis * distanceCalc.calcDist(nextLat, nextLon, lat, lon) / distance);
+            lat = nextLat;
+            lon = nextLon;
         }
-        return time;
+        return time + millis;
     }
 
     @Override
@@ -173,4 +171,10 @@ public String toString()
         sb.append(')');
         return sb.toString();
     }
+
+    void checkOne()
+    {
+        if (points.size() < 1)
+            throw new IllegalStateException("Instruction must contain at least one point " + toString());
+    }
 }

core/src/main/java/com/graphhopper/util/InstructionList.java

@@ -236,15 +236,19 @@ public List<GPXEntry> createGPXList()
 
         List<GPXEntry> gpxList = new ArrayList<GPXEntry>();
         long timeOffset = 0;
-        double prevLat = Double.NaN, prevLon = Double.NaN;
-        double prevFactor = 0;
-        for (Instruction i : this)
+        for (int i = 0; i < size() - 1; i++)
         {
-            timeOffset = i.fillGPXList(gpxList, timeOffset, prevFactor, prevLat, prevLon);
-            prevFactor = i.getDistance() / i.getMillis();
-            prevLat = i.getLastLat();
-            prevLon = i.getLastLon();
+            Instruction nextInstr = get(i + 1);
+            nextInstr.checkOne();            
+            // current instruction does not contain last point which is equals to first point of next instruction:
+            double nextLat = nextInstr.getFirstLat(), nextLon = nextInstr.getFirstLon();            
+            timeOffset = get(i).fillGPXList(gpxList, timeOffset, nextLat, nextLon);
         }
+        Instruction lastI = get(size() - 1);
+        if (lastI.points.size() != 1)
+            throw new IllegalStateException("Last instruction must have exactly one point but was " + lastI.points.size());
+        double lastLat = lastI.getFirstLat(), lastLon = lastI.getFirstLon();
+        gpxList.add(new GPXEntry(lastLat, lastLon, timeOffset));
         return gpxList;
     }
 

core/src/test/java/com/graphhopper/routing/GraphHopperIT.java

@@ -91,6 +91,13 @@ public void testMonacoWithInstructions()
             assertEquals(30, times.get(3) / 1000);
             assertEquals(87, times.get(4) / 1000);
             assertEquals(321, times.get(5) / 1000);
+
+            List<GPXEntry> list = rsp.getInstructions().createGPXList();
+            assertEquals(123, list.size());
+            final long lastEntryMillis = list.get(list.size() - 1).getMillis();
+            final long totalResponseMillis = rsp.getMillis();
+            assertEquals(totalResponseMillis, lastEntryMillis);
+
         } catch (Exception ex)
         {
             throw new RuntimeException("cannot handle osm file " + osmFile, ex);

core/src/test/java/com/graphhopper/util/InstructionListTest.java

@@ -24,7 +24,6 @@
 import com.graphhopper.routing.util.ShortestWeighting;
 import com.graphhopper.storage.Graph;
 import com.graphhopper.storage.GraphBuilder;
-import gnu.trove.list.array.TDoubleArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
@@ -221,12 +220,11 @@ public void testInstructionsWithTimeAndPlace()
         assertEquals(5, wayList.size());
 
         List<GPXEntry> gpxList = wayList.createGPXList();
-        // distances and times are not identical (only similar) as we only guessed the edge distance
         assertEquals(34000, p.getDistance(), 1e-1);
         assertEquals(34000, sum(wayList.createDistances()), 1e-1);
         assertEquals(5, gpxList.size());
         assertEquals(1604120, p.getMillis());
-        assertEquals(2097557, gpxList.get(gpxList.size() - 1).getMillis());
+        assertEquals(1604120, gpxList.get(gpxList.size() - 1).getMillis());
 
         assertEquals(Instruction.CONTINUE_ON_STREET, wayList.get(0).getIndication());
         assertEquals(15, wayList.get(0).getFirstLat(), 1e-3);
@@ -271,9 +269,9 @@ public void testCreateGPX()
 
         assertEquals(0, result.get(0).getMillis());
         assertEquals(10391, result.get(1).getMillis());
-        assertEquals(15602, result.get(2).getMillis());
-        assertEquals(25449, result.get(3).getMillis());
-        assertEquals(30559, result.get(4).getMillis());
+        assertEquals(15000, result.get(2).getMillis());
+        assertEquals(19000, result.get(3).getMillis());
+        assertEquals(22000, result.get(4).getMillis());
     }
 
     private long flagsForSpeed( EncodingManager encodingManager, int speedKmPerHour )