Repository: graphhopper/graphhopper
Pull request #2265 (2021-03-25 11:38:51)

Use restricted values for Bicycle Oneways
Related to #2245. As introduced in 2245, we don't check the :forward and :backward tags for the restricted values.

This PR introduces the same changes for bicycle as well. I did change one more important thing that I think we need to discuss about, so I haven't changed the tests yet.

We had a special handling for the key `bicycle:backward`, which is based on [this Wiki entry](https://wiki.openstreetmap.org/wiki/Key:access#One-way_restrictions). I don't think that this entry is accurate. I checked at least 10 ways that have oneway tags and bicycle:backwards ([Overpass](https://overpass-turbo.eu/s/15oQ)), the ways I found and that I could double check on overpass don't have a special meaning for bicycle:backward, it is just used as any other access value. I also could not find any relevant notions of this special handling in othe resources.

Here are a few examples:
- [oneway:bicycle=yes, bicycle:backward=no](https://www.openstreetmap.org/way/147064435)
- [bicycle:backward=no](https://www.openstreetmap.org/way/60607170#map=19/49.79901/9.94374&layers=D), see [Mapillary](https://www.mapillary.com/map/im/dZfOUJUs-1O5qigdBSztLQ)
- [bicycle:backward=no,oneway:bicycle=yes, Changenote: Radweg nur für eine Richtung](https://www.openstreetmap.org/way/285630396)

So I would like to challenge this "special handling", I haven't found one example where it is used as described in the Wiki (it might exist though).

CC: @ratrun 

ratrun: As the special handling for `oneway` togheter with `bicycle:backward` is not even translated on the German wiki I don't mind if we get rid of it. The combination is also pretty rare (See https://taginfo.openstreetmap.org/keys/bicycle%3Abackward#values). 

I made a comparison for your first example. Here is our solution in the opposite direction of the oneway: https://graphhopper.com/maps/?point=49.018178%2C8.375509&point=49.01619%2C8.3753&locale=de&vehicle=bike&weighting=fastest&elevation=true&turn_costs=false&use_miles=false&layer=Omniscale compared to brouter web https://brouter.de/brouter-web/#map=18/49.01730/8.37551/osm-mapnik-german_style&lonlats=8.375477,49.017696;8.375337,49.015428;8.375444,49.017388&profile=fastbike-lowtraffic
I expect that your modifications will make us obey the oneway as well, just as brouter does.
boldtrn: Thanks a lot for your feedback @ratrun. I updated the PR and included the checks for intended values as well. So for example `oneway=yes` and `bicycle:backward=yes` results in an edge that is forward and backward accessible for bicycles.
karussell: > which is based on this Wiki entry

For future reference can you write what we expected before? And should we ask to change the OSM wiki?
boldtrn: > And should we ask to change the OSM wiki?

I think so yes.

Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java b/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java
index 57b5d3af3..de9914acf 100644
--- a/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java
+++ b/core/src/main/java/com/graphhopper/routing/util/BikeCommonFlagEncoder.java
@@ -512,22 +512,26 @@ abstract public class BikeCommonFlagEncoder extends AbstractFlagEncoder {
         avgSpeedEnc.setDecimal(false, edgeFlags, speed);
 
         // handle oneways
-        boolean isOneway = way.hasTag("oneway", oneways)
+        // oneway=-1 requires special handling
+        boolean isOneway = (way.hasTag("oneway", oneways) && !way.hasTag("oneway", "-1") && !way.hasTag("bicycle:backward", intendedValues))
+                || (way.hasTag("oneway", "-1") && !way.hasTag("bicycle:forward", intendedValues))
                 || way.hasTag("oneway:bicycle", oneways)
-                || way.hasTag("vehicle:backward")
-                || way.hasTag("vehicle:forward")
-                || way.hasTag("bicycle:forward") && (way.hasTag("bicycle:forward", "yes") || way.hasTag("bicycle:forward", "no"));
+                || (way.hasTag("vehicle:backward", restrictedValues) && !way.hasTag("bicycle:forward", intendedValues))
+                || (way.hasTag("vehicle:forward", restrictedValues) && !way.hasTag("bicycle:backward", intendedValues))
+                || way.hasTag("bicycle:forward", restrictedValues)
+                || way.hasTag("bicycle:backward", restrictedValues);
 
         if ((isOneway || roundaboutEnc.getBool(false, edgeFlags))
                 && !way.hasTag("oneway:bicycle", "no")
-                && !way.hasTag("bicycle:backward")
                 && !way.hasTag("cycleway", oppositeLanes)
                 && !way.hasTag("cycleway:left", oppositeLanes)
-                && !way.hasTag("cycleway:right", oppositeLanes)) {
+                && !way.hasTag("cycleway:right", oppositeLanes)
+                && !way.hasTag("cycleway:left:oneway", "-1")
+                && !way.hasTag("cycleway:right:oneway", "-1")) {
             boolean isBackward = way.hasTag("oneway", "-1")
                     || way.hasTag("oneway:bicycle", "-1")
-                    || way.hasTag("vehicle:forward", "no")
-                    || way.hasTag("bicycle:forward", "no");
+                    || way.hasTag("vehicle:forward", restrictedValues)
+                    || way.hasTag("bicycle:forward", restrictedValues);
             accessEnc.setBool(isBackward, edgeFlags, true);
 
         } else {


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java b/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java
index 3c4ff70ab..90126a205 100644
--- a/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/BikeFlagEncoderTest.java
@@ -403,20 +403,34 @@ public class BikeFlagEncoderTest extends AbstractBikeFlagEncoderTester {
         assertTrue(encoder.getAccessEnc().getBool(true, flags));
         way.clearTags();
 
-        // attention bicycle:backward=no/yes has a completely different meaning!
-        // https://wiki.openstreetmap.org/wiki/Key:access#One-way_restrictions
         way.setTag("highway", "tertiary");
         way.setTag("oneway", "yes");
         way.setTag("bicycle:backward", "no");
         flags = encoder.handleWayTags(encodingManager.createEdgeFlags(), way, encoder.getAccess(way));
         assertTrue(encoder.getAccessEnc().getBool(false, flags));
+        assertFalse(encoder.getAccessEnc().getBool(true, flags));
+
+        way.setTag("bicycle:backward", "yes");
+        flags = encoder.handleWayTags(encodingManager.createEdgeFlags(), way, encoder.getAccess(way));
+        assertTrue(encoder.getAccessEnc().getBool(false, flags));
         assertTrue(encoder.getAccessEnc().getBool(true, flags));
 
+        way.clearTags();
+        way.setTag("highway", "residential");
+        way.setTag("oneway", "yes");
         way.setTag("bicycle:backward", "yes");
         flags = encoder.handleWayTags(encodingManager.createEdgeFlags(), way, encoder.getAccess(way));
         assertTrue(encoder.getAccessEnc().getBool(false, flags));
         assertTrue(encoder.getAccessEnc().getBool(true, flags));
 
+        way.clearTags();
+        way.setTag("highway", "residential");
+        way.setTag("oneway", "-1");
+        way.setTag("bicycle:forward", "yes");
+        flags = encoder.handleWayTags(encodingManager.createEdgeFlags(), way, encoder.getAccess(way));
+        assertTrue(encoder.getAccessEnc().getBool(false, flags));
+        assertTrue(encoder.getAccessEnc().getBool(true, flags));
+
         way.clearTags();
         way.setTag("highway", "tertiary");
         way.setTag("bicycle:forward", "use_sidepath");