Repository: graphhopper/graphhopper
Pull request #2203 (2020-12-06 10:15:00)

Change of non-shared EncodedValue
With #2198 we have stricter naming but we still allow `.` although this is not allowed in a Java variable.

This is a bit ugly as we can currently use the non-shared EncodedValues like `car.access` or `car.average_speed` or `bike.access` in the CustomWeighting. They are "non-shared" because the FlagEncoder creates and populates them - this will be moved into a separate TagParser in the future, but for now we have to detect it as a special case because these EncodedValues should be excluded when storing+loading all encoded values. This will disappear when FlagEncoders dissolve. 

This change is basically a change to make master forward compatible with the janino_scripting branch, i.e. also allow non-shared EncodedValues in the new CustomWeighting with janino scripting. As variables with `.` introduce a problem as e.g. `car.access` would need special handling. Either we introduce a class+object per flagEncoder (ala `class Car { BooleanEncodedValue access; DecimalEncodedValue average_speed; }` etc) so that the expression becomes a valid Java expression or we replace the dot with a character like `$` before creating the script, so that this is a valid Java variable and does not need special handling. Both solutions make the scripting creation more complex and error-prone, which I would like to avoid especially as we already plan to deprecate this `x.y` notation (when it is moved to TagParser).

Why `$` and not something else? I didn't find a better character that is suited as a separator character and is also allowed in a Java variable.


Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/ev/TrackType.java b/core/src/main/java/com/graphhopper/routing/ev/TrackType.java
index 6421609b2..1fb61cff2 100644
--- a/core/src/main/java/com/graphhopper/routing/ev/TrackType.java
+++ b/core/src/main/java/com/graphhopper/routing/ev/TrackType.java
@@ -21,14 +21,14 @@ import com.graphhopper.util.Helper;
 
 /**
  * This enum defines the track type of an edge which describes how well-maintained a certain track is.
- * If not tagged the value will be MISSING (the default and easiest grad) and all edges that do not fit get OTHER as
- * value (the worst grade).
+ * If there is no value tagged or if the value does not fit the given values, the TrackType will be MISSING.
+ * grade1 is a very well-maintained road, grade5 is a poorly maintained road.
  *
  * @see <a href="https://wiki.openstreetmap.org/wiki/Tracktype">Tracktype Wiki</a>
  */
 public enum TrackType {
     MISSING("missing"), GRADE1("grade1"), GRADE2("grade2"),
-    GRADE3("grade3"), GRADE4("grade4"), GRADE5("grade5"), OTHER("other");
+    GRADE3("grade3"), GRADE4("grade4"), GRADE5("grade5");
 
     public static final String KEY = "track_type";
 
@@ -49,7 +49,7 @@ public enum TrackType {
         try {
             return TrackType.valueOf(Helper.toUpperCase(name));
         } catch (IllegalArgumentException ex) {
-            return OTHER;
+            return MISSING;
         }
     }
 }
diff --git a/core/src/main/java/com/graphhopper/routing/util/EncodingManager.java b/core/src/main/java/com/graphhopper/routing/util/EncodingManager.java
index c9a8c8b94..2fc64d0ee 100644
--- a/core/src/main/java/com/graphhopper/routing/util/EncodingManager.java
+++ b/core/src/main/java/com/graphhopper/routing/util/EncodingManager.java
@@ -794,10 +794,10 @@ public class EncodingManager implements EncodedValueLookup {
         return (T) ev;
     }
 
-    private static final String SPECIAL_SEPARATOR = ".";
+    private static final String SPECIAL_SEPARATOR = "$";
 
     public static boolean isSharedEncodedValues(EncodedValue ev) {
-        return !ev.getName().contains(SPECIAL_SEPARATOR);
+        return isValidEncodedValue(ev.getName()) && !ev.getName().contains(SPECIAL_SEPARATOR);
     }
 
     /**
@@ -837,14 +837,19 @@ public class EncodingManager implements EncodedValueLookup {
         // first character must be a lower case letter
         if (name.isEmpty() || !isLowerLetter(name.charAt(0)) || KEYWORDS.contains(name)) return false;
 
-        int dotCount = 0;
+        int dollarCount = 0, underscoreCount = 0;
         for (int i = 1; i < name.length(); i++) {
             char c = name.charAt(i);
-            if (c == '.') {
-                if (dotCount > 0) return false;
-                dotCount++;
-            } else if (!isLowerLetter(c) && !isNumber(c) && c != '_' && c != '$') {
+            if (c == '$') {
+                if (dollarCount > 0) return false;
+                dollarCount++;
+            } else if (c == '_') {
+                if (underscoreCount > 0) return false;
+                underscoreCount++;
+            } else if (!isLowerLetter(c) && !isNumber(c)) {
                 return false;
+            } else {
+                underscoreCount = 0;
             }
         }
         return true;
diff --git a/core/src/main/java/com/graphhopper/util/GHUtility.java b/core/src/main/java/com/graphhopper/util/GHUtility.java
index a55e54a1e..b94812015 100644
--- a/core/src/main/java/com/graphhopper/util/GHUtility.java
+++ b/core/src/main/java/com/graphhopper/util/GHUtility.java
@@ -256,11 +256,12 @@ public class GHUtility {
         }
     }
 
-    public static List<Snap> createRandomSnaps(BBox bbox, LocationIndex locationIndex, Random rnd, int numVirtualNodes, boolean acceptTower, EdgeFilter filter) {
-        int count = 0;
-        List<Snap> snaps = new ArrayList<>(numVirtualNodes);
-        while (snaps.size() < numVirtualNodes) {
-            if (count > numVirtualNodes * 100) {
+    public static List<Snap> createRandomSnaps(BBox bbox, LocationIndex locationIndex, Random rnd, int numPoints, boolean acceptTower, EdgeFilter filter) {
+        int maxTries = numPoints * 100;
+        int tries = 0;
+        List<Snap> snaps = new ArrayList<>(numPoints);
+        while (snaps.size() < numPoints) {
+            if (tries > maxTries) {
                 throw new IllegalArgumentException("Could not create enough virtual edges");
             }
             Snap snap = getRandomSnap(locationIndex, rnd, bbox, filter);
@@ -269,7 +270,7 @@ public class GHUtility {
                 accepted = accepted && !snap.getSnappedPosition().equals(Snap.Position.TOWER);
             if (accepted)
                 snaps.add(snap);
-            count++;
+            tries++;
         }
         return snaps;
     }
diff --git a/core/src/main/java/com/graphhopper/util/details/BooleanDetails.java b/core/src/main/java/com/graphhopper/util/details/BooleanDetails.java
index 9ffb122d4..218bebe56 100644
--- a/core/src/main/java/com/graphhopper/util/details/BooleanDetails.java
+++ b/core/src/main/java/com/graphhopper/util/details/BooleanDetails.java
@@ -1,3 +1,20 @@
+/*
+ *  Licensed to GraphHopper GmbH under one or more contributor
+ *  license agreements. See the NOTICE file distributed with this work for
+ *  additional information regarding copyright ownership.
+ *
+ *  GraphHopper GmbH licenses this file to you under the Apache License,
+ *  Version 2.0 (the "License"); you may not use this file except in
+ *  compliance with the License. You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
 package com.graphhopper.util.details;
 
 import com.graphhopper.routing.ev.BooleanEncodedValue;
diff --git a/docs/isochrone/java.md b/docs/isochrone/java.md
index 7eceec2c4..6a39949e4 100644
--- a/docs/isochrone/java.md
+++ b/docs/isochrone/java.md
@@ -3,13 +3,11 @@
 To use the following examples you need to specify the dependency in
 your [Maven config](/README.md#maven) correctly.
 
-To create an isochrone in Java code:
+You'll first need to build off an existing Graphhopper instance for [routing](../core/routing.md).
 
-You'll first need to build off an existing Graphhopper instance for [routing](/../core/routing.md).
+See this [example code](../../example/src/main/java/com/graphhopper/example/IsochroneExample.java)
+how to build and traverse a shortest path tree, which means enumerating, in order, all nodes that 
+can be reached within a given time limit.
 
-Next, compute the isochrone itself.
-
-See the [example code](../../example/src/main/java/com/graphhopper/example/IsochroneExample.java).
-
-See [IsochroneResource.java](https://github.com/graphhopper/graphhopper/blob/master/web-bundle/src/main/java/com/graphhopper/resources/IsochroneResource.java)
-to find out how to create an iso line polygon ("isochrone") from using the shortest path tree.
\ No newline at end of file
+See [IsochroneResource.java](../../web-bundle/src/main/java/com/graphhopper/resources/IsochroneResource.java)
+to see how we use the shortest path tree to construct an isochrone (or other isoline, depending on the weighting).
\ No newline at end of file


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/util/EncodingManagerTest.java b/core/src/test/java/com/graphhopper/routing/util/EncodingManagerTest.java
index 718c405a5..6470caef6 100644
--- a/core/src/test/java/com/graphhopper/routing/util/EncodingManagerTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/EncodingManagerTest.java
@@ -280,11 +280,12 @@ public class EncodingManagerTest {
 
     @Test
     public void validEV() {
-        for (String str : Arrays.asList("blup_test", "test", "test12", "tes$0", "blup.test", "blup_te.st_")) {
+        for (String str : Arrays.asList("blup_test", "test", "test12", "tes$0", "car_test_test", "small_car$average_speed")) {
             assertTrue(str, EncodingManager.isValidEncodedValue(str));
         }
 
-        for (String str : Arrays.asList("Test", "12test", "test|3", "test{34", "test,21", "täst", "blup.two.three", "blup..test")) {
+        for (String str : Arrays.asList("Test", "12test", "test|3", "car__test", "blup_te.st_", "car___test", "car$$access",
+                "test{34", "truck__average_speed", "blup.test", "test,21", "täst", "blup.two.three", "blup..test")) {
             assertFalse(str, EncodingManager.isValidEncodedValue(str));
         }
 
diff --git a/core/src/test/java/com/graphhopper/routing/util/parsers/OSMTrackTypeParserTest.java b/core/src/test/java/com/graphhopper/routing/util/parsers/OSMTrackTypeParserTest.java
index afc1cb434..58e7611b8 100644
--- a/core/src/test/java/com/graphhopper/routing/util/parsers/OSMTrackTypeParserTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/parsers/OSMTrackTypeParserTest.java
@@ -60,7 +60,7 @@ public class OSMTrackTypeParserTest {
         IntsRef intsRef = em.createEdgeFlags();
         readerWay.setTag("tracktype", "unknownstuff");
         parser.handleWayTags(intsRef, readerWay, false, relFlags);
-        assertEquals(TrackType.OTHER, ttEnc.getEnum(false, intsRef));
+        assertEquals(TrackType.MISSING, ttEnc.getEnum(false, intsRef));
     }
 
     @Test
diff --git a/isochrone/src/test/java/com/graphhopper/isochrone/algorithm/ShortestPathTreeTest.java b/isochrone/src/test/java/com/graphhopper/isochrone/algorithm/ShortestPathTreeTest.java
index 5f66b5a89..aa5aa0522 100644
--- a/isochrone/src/test/java/com/graphhopper/isochrone/algorithm/ShortestPathTreeTest.java
+++ b/isochrone/src/test/java/com/graphhopper/isochrone/algorithm/ShortestPathTreeTest.java
@@ -157,14 +157,15 @@ public class ShortestPathTreeTest {
         instance.search(0, result::add);
         assertEquals(9, result.size());
         assertAll(
-                () -> assertEquals(0, result.get(0).time),
-                () -> assertEquals(9000, result.get(1).time),
-                () -> assertEquals(18000, result.get(2).time),
-                () -> assertEquals(25200, result.get(3).time),
-                () -> assertEquals(27000, result.get(4).time),
-                () -> assertEquals(36000, result.get(5).time),
-                () -> assertEquals(54000, result.get(6).time),
-                () -> assertEquals(55800, result.get(7).time)
+                () -> assertEquals(0, result.get(0).time), () -> assertEquals(0, result.get(0).node),
+                () -> assertEquals(9000, result.get(1).time), () -> assertEquals(4, result.get(1).node),
+                () -> assertEquals(18000, result.get(2).time), () -> assertEquals(6, result.get(2).node),
+                () -> assertEquals(25200, result.get(3).time), () -> assertEquals(1, result.get(3).node),
+                () -> assertEquals(27000, result.get(4).time), () -> assertEquals(7, result.get(4).node),
+                () -> assertEquals(36000, result.get(5).time), () -> assertEquals(5, result.get(5).node),
+                () -> assertEquals(54000, result.get(6).time), () -> assertEquals(2, result.get(6).node),
+                () -> assertEquals(55800, result.get(7).time), () -> assertEquals(3, result.get(7).node),
+                () -> assertEquals(60300, result.get(8).time), () -> assertEquals(8, result.get(8).node)
         );
     }
 
diff --git a/web/src/test/resources/com/graphhopper/http/resources/cargo_bike.yml b/web/src/test/resources/com/graphhopper/http/resources/cargo_bike.yml
index 8b086f30d..f9485d8a5 100644
--- a/web/src/test/resources/com/graphhopper/http/resources/cargo_bike.yml
+++ b/web/src/test/resources/com/graphhopper/http/resources/cargo_bike.yml
@@ -16,7 +16,7 @@ priority:
   surface:
     sand: 0.5
   # prefer better tracks
-  track_type: { other: 1.0, grade1: 1.0, "*": 0.9 }
+  track_type: { grade1: 1.0, "*": 0.9 }
   # prefer official bike routes
   bike_network: { other: 0.5 }
   # avoid all situations where we have to get off the bike