Reproducing & fixing reverse speed error

CONTRIBUTORS.md

@@ -1,11 +1,11 @@
 [Members](https://github.com/graphhopper?tab=members) and [Contributors](https://github.com/graphhopper/graphhopper/contributors)
 
- * ammagamma, improvements like #700
+ * ammagamma, several improvements like #700, #703
  * AnahitaS, docs for Android, Android, Tomcat
  * andreaswolf, flag encoder versioning and more
  * agouge, discussion and API refactoring
  * b3nn0, Android improvements
- * boldtrn, motorcycle and improvements like conditional tag parsing etc
+ * boldtrn, motorcycle and improvements like conditional tag parsing, round trips and a lot more
  * cgarreau, increase of routing success rate via subnetwork cleanup
  * ChristianSeitzer, motorcycle improvements
  * daisy1754, fixed usage of graphhopper.sh script

core/files/changelog.txt

@@ -1,4 +1,5 @@
 0.7
+    refactored FlagEncoder.handleFerryWay to getFerrySpeed to make it possible to fix #665
     removed setWeightLimit as too unspecific for arbitrary weights, use setMaxVisitedNodes instead
     missing renames for Path.setEdgeEntry -> setSPTEntry and AbstractAlgorithm.createEdgeEntry -> createSPTEntry
 

core/src/main/java/com/graphhopper/routing/util/AbstractFlagEncoder.java

@@ -475,7 +475,7 @@ public void applyWayTags( OSMWay way, EdgeIteratorState edge )
     /**
      * Special handling for ferry ways.
      */
-    protected long handleFerryTags( OSMWay way, double unknownSpeed, double shortTripsSpeed, double longTripsSpeed )
+    protected double getFerrySpeed( OSMWay way, double unknownSpeed, double shortTripsSpeed, double longTripsSpeed )
     {
         long duration = 0;
         try
@@ -511,8 +511,7 @@ protected long handleFerryTags( OSMWay way, double unknownSpeed, double shortTri
                             if (shortTripsSpeed > getMaxSpeed())
                                 shortTripsSpeed = getMaxSpeed();
                             longTripsSpeed = shortTripsSpeed;
-                        }
-                        else
+                        } else
                         {
                             // Now we set to the lowest possible still accessible speed. 
                             shortTripsSpeed = speedEncoder.factor / 2;
@@ -531,14 +530,14 @@ protected long handleFerryTags( OSMWay way, double unknownSpeed, double shortTri
         if (durationInHours == 0)
         {
             // unknown speed -> put penalty on ferry transport
-            return setSpeed(0, unknownSpeed);
+            return unknownSpeed;
         } else if (durationInHours > 1)
         {
             // lengthy ferries should be faster than short trip ferry
-            return setSpeed(0, longTripsSpeed);
+            return longTripsSpeed;
         } else
         {
-            return setSpeed(0, shortTripsSpeed);
+            return shortTripsSpeed;
         }
     }
 
@@ -655,11 +654,8 @@ else if (maxTurnCosts == 1)
         {
             if (costs != 0 || Double.isInfinite(costs))
                 throw new IllegalArgumentException("Restricted turn can only have infinite costs (or use 0)");
-        } else
-        {
-            if (costs >= maxTurnCosts)
-                throw new IllegalArgumentException("Cost is too high. Or specifiy restricted == true");
-        }
+        } else if (costs >= maxTurnCosts)
+            throw new IllegalArgumentException("Cost is too high. Or specifiy restricted == true");
 
         if (costs < 0)
             throw new IllegalArgumentException("Turn costs cannot be negative");
@@ -750,7 +746,7 @@ public double getDouble( long flags, int key )
     /**
      * @param way: needed to retrieve OSM tags
      * @param speed: speed guessed e.g. from the road type or other tags
-     * @return The assumed speed. 
+     * @return The assumed speed.
      */
     protected double applyMaxSpeed( OSMWay way, double speed )
     {

core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java

@@ -355,34 +355,35 @@ public long handleWayTags( OSMWay way, long allowed, long relationFlags )
         if (!isAccept(allowed))
             return 0;
 
-        long encoded = 0;
+        long flags = 0;
         double wayTypeSpeed = getSpeed(way);
         if (!isFerry(allowed))
         {
             wayTypeSpeed = applyMaxSpeed(way, wayTypeSpeed);
-            encoded = handleSpeed(way, wayTypeSpeed, encoded);
-            encoded = handleBikeRelated(way, encoded, relationFlags > UNCHANGED.getValue());
+            flags = handleSpeed(way, wayTypeSpeed, flags);
+            flags = handleBikeRelated(way, flags, relationFlags > UNCHANGED.getValue());
 
             boolean isRoundabout = way.hasTag("junction", "roundabout");
             if (isRoundabout)
             {
-                encoded = setBool(encoded, K_ROUNDABOUT, true);
+                flags = setBool(flags, K_ROUNDABOUT, true);
             }
 
         } else
         {
-            encoded = handleFerryTags(way,
+            double ferrySpeed = getFerrySpeed(way,
                     highwaySpeeds.get("living_street"),
                     highwaySpeeds.get("track"),
                     highwaySpeeds.get("primary"));
-            encoded |= directionBitMask;
+            flags = handleSpeed(way, ferrySpeed, flags);
+            flags |= directionBitMask;
         }
         int priorityFromRelation = 0;
         if (relationFlags != 0)
             priorityFromRelation = (int) relationCodeEncoder.getValue(relationFlags);
 
-        encoded = priorityWayEncoder.setValue(encoded, handlePriority(way, wayTypeSpeed, priorityFromRelation));
-        return encoded;
+        flags = priorityWayEncoder.setValue(flags, handlePriority(way, wayTypeSpeed, priorityFromRelation));
+        return flags;
     }
 
     int getSpeed( OSMWay way )

core/src/main/java/com/graphhopper/routing/util/CarFlagEncoder.java

@@ -243,7 +243,7 @@ public long handleWayTags( OSMWay way, long allowed, long relationFlags )
         if (!isAccept(allowed))
             return 0;
 
-        long encoded;
+        long flags = 0;
         if (!isFerry(allowed))
         {
             // get assumed speed from highway type
@@ -254,11 +254,11 @@ public long handleWayTags( OSMWay way, long allowed, long relationFlags )
             if (speed > 30 && way.hasTag("surface", badSurfaceSpeedMap))
                 speed = 30;
 
-            encoded = setSpeed(0, speed);
+            flags = setSpeed(flags, speed);
 
             boolean isRoundabout = way.hasTag("junction", "roundabout");
             if (isRoundabout)
-                encoded = setBool(encoded, K_ROUNDABOUT, true);
+                flags = setBool(flags, K_ROUNDABOUT, true);
 
             boolean isOneway = way.hasTag("oneway", oneways)
                     || way.hasTag("vehicle:backward")
@@ -272,19 +272,20 @@ public long handleWayTags( OSMWay way, long allowed, long relationFlags )
                         || way.hasTag("vehicle:forward", "no")
                         || way.hasTag("motor_vehicle:forward", "no");
                 if (isBackward)
-                    encoded |= backwardBit;
+                    flags |= backwardBit;
                 else
-                    encoded |= forwardBit;
+                    flags |= forwardBit;
             } else
-                encoded |= directionBitMask;
+                flags |= directionBitMask;
 
         } else
         {
-            encoded = handleFerryTags(way, defaultSpeedMap.get("living_street"), defaultSpeedMap.get("service"), defaultSpeedMap.get("residential"));
-            encoded |= directionBitMask;
+            double ferrySpeed = getFerrySpeed(way, defaultSpeedMap.get("living_street"), defaultSpeedMap.get("service"), defaultSpeedMap.get("residential"));
+            flags = setSpeed(flags, ferrySpeed);
+            flags |= directionBitMask;
         }
 
-        return encoded;
+        return flags;
     }
 
     public String getWayInfo( OSMWay way )

core/src/main/java/com/graphhopper/routing/util/FootFlagEncoder.java

@@ -287,38 +287,39 @@ public long handleWayTags( OSMWay way, long allowed, long relationFlags )
         if (!isAccept(allowed))
             return 0;
 
-        long encoded = 0;
+        long flags = 0;
         if (!isFerry(allowed))
         {
             String sacScale = way.getTag("sac_scale");
             if (sacScale != null)
             {
                 if ("hiking".equals(sacScale))
-                    encoded = speedEncoder.setDoubleValue(encoded, MEAN_SPEED);
+                    flags = speedEncoder.setDoubleValue(flags, MEAN_SPEED);
                 else
-                    encoded = speedEncoder.setDoubleValue(encoded, SLOW_SPEED);
+                    flags = speedEncoder.setDoubleValue(flags, SLOW_SPEED);
             } else
             {
-                encoded = speedEncoder.setDoubleValue(encoded, MEAN_SPEED);
+                flags = speedEncoder.setDoubleValue(flags, MEAN_SPEED);
             }
-            encoded |= directionBitMask;
+            flags |= directionBitMask;
 
             boolean isRoundabout = way.hasTag("junction", "roundabout");
             if (isRoundabout)
-                encoded = setBool(encoded, K_ROUNDABOUT, true);
+                flags = setBool(flags, K_ROUNDABOUT, true);
 
         } else
         {
-            encoded = encoded | handleFerryTags(way, SLOW_SPEED, MEAN_SPEED, FERRY_SPEED);
-            encoded |= directionBitMask;
+            double ferrySpeed = getFerrySpeed(way, SLOW_SPEED, MEAN_SPEED, FERRY_SPEED);
+            flags = setSpeed(flags, ferrySpeed);
+            flags |= directionBitMask;
         }
 
         int priorityFromRelation = 0;
         if (relationFlags != 0)
             priorityFromRelation = (int) relationCodeEncoder.getValue(relationFlags);
 
-        encoded = priorityWayEncoder.setValue(encoded, handlePriority(way, priorityFromRelation));
-        return encoded;
+        flags = priorityWayEncoder.setValue(flags, handlePriority(way, priorityFromRelation));
+        return flags;
     }
 
     @Override

core/src/main/java/com/graphhopper/routing/util/MotorcycleFlagEncoder.java

@@ -208,7 +208,7 @@ public long handleWayTags( OSMWay way, long allowed, long priorityFromRelation )
         if (!isAccept(allowed))
             return 0;
 
-        long encoded = 0;
+        long flags = 0;
         if (!isFerry(allowed))
         {
             // get assumed speed from highway type
@@ -225,39 +225,41 @@ public long handleWayTags( OSMWay way, long allowed, long priorityFromRelation )
 
             boolean isRoundabout = way.hasTag("junction", "roundabout");
             if (isRoundabout)
-                encoded = setBool(0, K_ROUNDABOUT, true);
+                flags = setBool(0, K_ROUNDABOUT, true);
 
             if (way.hasTag("oneway", oneways) || isRoundabout)
             {
                 if (way.hasTag("oneway", "-1"))
                 {
-                    encoded = setReverseSpeed(encoded, speed);
-                    encoded |= backwardBit;
+                    flags = setReverseSpeed(flags, speed);
+                    flags |= backwardBit;
                 } else
                 {
-                    encoded = setSpeed(encoded, speed);
-                    encoded |= forwardBit;
+                    flags = setSpeed(flags, speed);
+                    flags |= forwardBit;
                 }
             } else
             {
-                encoded = setSpeed(encoded, speed);
-                encoded = setReverseSpeed(encoded, speed);
-                encoded |= directionBitMask;
+                flags = setSpeed(flags, speed);
+                flags = setReverseSpeed(flags, speed);
+                flags |= directionBitMask;
             }
 
         } else
         {
-            encoded = handleFerryTags(way, defaultSpeedMap.get("living_street"), defaultSpeedMap.get("service"), defaultSpeedMap.get("residential"));
-            encoded |= directionBitMask;
+            double ferrySpeed = getFerrySpeed(way, defaultSpeedMap.get("living_street"), defaultSpeedMap.get("service"), defaultSpeedMap.get("residential"));
+            flags = setSpeed(flags, ferrySpeed);
+            flags = setReverseSpeed(flags, ferrySpeed);
+            flags |= directionBitMask;
         }
 
         // relations are not yet stored -> see BikeCommonFlagEncoder.defineRelationBits how to do this
-        encoded = priorityWayEncoder.setValue(encoded, handlePriority(way, priorityFromRelation));
+        flags = priorityWayEncoder.setValue(flags, handlePriority(way, priorityFromRelation));
 
         // Set the curvature to the Maximum
-        encoded = curvatureEncoder.setValue(encoded, 10);
+        flags = curvatureEncoder.setValue(flags, 10);
 
-        return encoded;
+        return flags;
     }
 
     @Override

core/src/test/java/com/graphhopper/reader/OSMReaderTest.java

@@ -36,8 +36,11 @@
 import org.junit.Test;
 
 import com.graphhopper.GraphHopper;
+import com.graphhopper.GHRequest;
+import com.graphhopper.GHResponse;
 import com.graphhopper.reader.dem.ElevationProvider;
 import com.graphhopper.reader.dem.SRTMProvider;
+import com.graphhopper.routing.DijkstraBidirectionRef;
 import com.graphhopper.routing.util.*;
 import com.graphhopper.storage.*;
 import com.graphhopper.storage.index.QueryResult;
@@ -64,6 +67,7 @@ public class OSMReaderTest
     // test-osm6.pbf was created by running "osmconvert test-osm6.xml --timestamp=2014-01-02T00:10:14Z -o=test-osm6.pbf"
     // The osmconvert tool can be found here: http://wiki.openstreetmap.org/wiki/Osmconvert
     private final String file6 = "test-osm6.pbf";
+    private final String file7 = "test-osm7.xml";
     private final String fileNegIds = "test-osm-negative-ids.xml";
     private final String fileBarriers = "test-barriers.xml";
     private final String fileTurnRestrictions = "test-restrictions.xml";
@@ -854,4 +858,20 @@ public void testCrossBoundary_issue667()
         assertEquals(-179.6, qr.getSnappedPoint().lon, 0.1);
         assertEquals(56, qr.getClosestEdge().getDistance() / 1000, 1);
     }
+
+    public void testRoutingRequestFails_issue665()
+    {
+        GraphHopper hopper = new GraphHopper();
+        hopper.setOSMFile("src/test/resources/com/graphhopper/reader/" + file7);
+        hopper.setEncodingManager(new EncodingManager("car,motorcycle"));
+        hopper.setCHEnable(false);
+        hopper.setGraphHopperLocation(dir);
+        hopper.importOrLoad();
+        GHRequest req = new GHRequest(48.97725592769741, 8.256896138191223, 48.978875552977684, 8.25486302375793).
+                setWeighting("curvature").
+                setVehicle("motorcycle");
+
+        GHResponse ghRsp = hopper.route(req);
+        assertFalse(ghRsp.getErrors().toString(), ghRsp.hasErrors());
+    }
 }

core/src/test/java/com/graphhopper/routing/util/Bike2WeightFlagEncoderTest.java

@@ -20,9 +20,7 @@
 
 import com.graphhopper.reader.OSMWay;
 import com.graphhopper.storage.*;
-import com.graphhopper.util.EdgeIteratorState;
-import com.graphhopper.util.GHUtility;
-import com.graphhopper.util.Helper;
+import com.graphhopper.util.*;
 import org.junit.Test;
 
 import static org.junit.Assert.*;
@@ -94,4 +92,43 @@ public void testSetSpeed0_issue367()
         assertFalse(encoder.isForward(flags));
         assertTrue(encoder.isBackward(flags));
     }
+
+    @Test
+    public void testRoutingFailsWithInvalidGraph_issue665()
+    {
+        GraphHopperStorage graph = new GraphHopperStorage(
+                new RAMDirectory(), em, true, new GraphExtension.NoOpExtension());
+        graph.create(100);
+
+        OSMWay way = new OSMWay(0);
+        way.setTag("route", "ferry");
+
+        long includeWay = em.acceptWay(way);
+        long relationFlags = 0;
+        long wayFlags = em.handleWayTags(way, includeWay, relationFlags);
+        graph.edge(0, 1).setDistance(247).setFlags(wayFlags);
+
+        assertTrue(isGraphValid(graph, encoder));
+    }
+
+    private boolean isGraphValid( Graph graph, FlagEncoder encoder )
+    {
+        EdgeExplorer explorer = graph.createEdgeExplorer();
+
+        // iterator at node 0 considers the edge 0-1 to be undirected
+        EdgeIterator iter0 = explorer.setBaseNode(0);
+        iter0.next();
+        boolean iter0flag
+                = iter0.getBaseNode() == 0 && iter0.getAdjNode() == 1
+                && iter0.isForward(encoder) && iter0.isBackward(encoder);
+
+        // iterator at node 1 considers the edge 1-0 to be directed
+        EdgeIterator iter1 = explorer.setBaseNode(1);
+        iter1.next();
+        boolean iter1flag
+                = iter1.getBaseNode() == 1 && iter1.getAdjNode() == 0
+                && iter1.isForward(encoder) && iter1.isBackward(encoder);
+
+        return iter0flag && iter1flag;
+    }
 }