Repository: graphhopper/graphhopper
Pull request #1880 (2020-01-29 16:26:06)

SpatialRule interface rework
This PR aims to improve our current spatial rule interface.

Summary of the changes:
* enforce border polygons being passed via constructor
* enforce implementation of `getId()`
* use Enum constants instead of Strings for `getMaxSpeed()` and `getAccess()`
* merge DefaultSpatialRule into AbstractSpatialRule
* allow rules to overwrite existing values instead of only being used as a fallback

otbutz: > Can you move the RoadClass changes into another PR? E.g. we could add also `platform` or `crossing` and more: https://taginfo.openstreetmap.org/keys/highway#values

Sure. I thought about adding other values but hesitated as we use the RoadClass for encoding and i didn't want to blow up the size of the graph for unused values. It's something different though if you already have plans for them.

> > merge DefaultSpatialRule into AbstractSpatialRule
> 
> What is the reason here?

Except for `SpatialRule.EMPTY` and a few test classes, all other SpatialRule implementations already extended `DefaultSpatialRule`. IMHO it's perfectly fine for an abstract class to have some default values which can be overridden by child classes. So the class hierarchy is rather simple now:

![old](https://user-images.githubusercontent.com/22315436/74341740-7fab5200-4da8-11ea-8ea1-27f4bd7b2e39.png)

![new](https://user-images.githubusercontent.com/22315436/74341758-8934ba00-4da8-11ea-830b-e33f83bd8a11.png)


> > allow rules to overwrite existing values instead of only being used as a fallback
> 
> Can you link to the code change here?

OSMMaxSpeedParser:
https://github.com/graphhopper/graphhopper/pull/1880/files#diff-c4f1bfac73d1aca26183a54eadc495e2L58-R60

The rule was only used if no max speed was present.

OSMRoadAccessParser:
https://github.com/graphhopper/graphhopper/pull/1880/files#diff-a27f9ca4c9c813f614ae87230209b92aL63-R66

The rule was only used if the road access wasn't already restricted.
karussell: > I thought about adding other values but hesitated as we use the RoadClass for encoding and i didn't want to blow up the size of the graph for unused values.

So as soon as we are over 15 distinct values it does not matter if we have 16 or 31 :)

> IMHO it's perfectly fine for an abstract class to have some default values which can be overridden by child classes. So the class hierarchy is rather simple now

Yes, I like this. But I do not like the car centric view as a default this might be misleading. Even road-centric might be wrong. But not so sure if this is a problem as it is in the abstract class...

> allow rules to overwrite existing values instead of only being used as a fallback

Ah, ok. This makes sense and I like it. Still should we add it to the changelog as it is a breaking change (?)
otbutz: > > IMHO it's perfectly fine for an abstract class to have some default values which can be overridden by child classes. So the class hierarchy is rather simple now
> 
> Yes, I like this. But I do not like the car centric view for the default, as this might be misleading.

`getRoadAccess()` already has the parameter `TransportationMode` which might be used to differentiate a bit more. The problem is that `OSMRoadAccessParser` only passes `TransportationMode.MOTOR_VEHICLE` and we would have to store multiple values per edge:

![Screenshot_2020-02-12 OSM tags for routing Access restrictions – OpenStreetMap Wiki](https://user-images.githubusercontent.com/22315436/74350920-b7b99180-4db6-11ea-9d79-e1cd861469ac.png)
Source: https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Access_restrictions#Default

It's the same for `getMaxSpeed()` as the maximum speed for an edge might depend on the type of transport. e.g motorway speed limits for cars and trucks

> Even road-centric might be wrong.

Indoor navigation or what is the use case here?

> But not so sure if this is a problem as it is in the abstract class...

Which is still subject to change :wink: 

> > allow rules to overwrite existing values instead of only being used as a fallback
> 
> Ah, ok. This makes sense and I like it. Still should we add it to the changelog as it is a breaking change (?)

I can modify the two existing rules in order to follow the old fallback logic if that's better.

otbutz: > Can you move the RoadClass changes into another PR? E.g. we could add also `platform` or `crossing` and more: https://taginfo.openstreetmap.org/keys/highway#values

I found the following possible candidates:
* platform (pedestrian)
* steps (pedestrian)
* corridor (pedestrian, indoor navigation)
* construction (?)

`crossing` seems to be only valid for points: https://wiki.openstreetmap.org/wiki/Tag%3Ahighway%3Dcrossing
karussell: > > Even road-centric might be wrong.

> Indoor navigation or what is the use case here?

E.g. rail routing

> crossing seems to be only valid for points:

:+1: 

> I can modify the two existing rules in order to follow the old fallback logic if that's better

Sorry, I think we need to revert this. The problem is that a more specific information on the road should not be overwritten from the country-specific default. 

> The problem is that OSMRoadAccessParser only passes TransportationMode.MOTOR_VEHICLE and we would have to store multiple values per edge:

Hmmh, yeah. For multiple max_speed values like for truck we could introduce max_speed_truck? Not sure.
otbutz: > > I can modify the two existing rules in order to follow the old fallback logic if that's better
> 
> Sorry, I think we need to revert this. The problem is that a more specific information on the road should not be overwritten from the country-specific default.

SpatialRules *should* be able to overwrite existing values but our existing country rules are only intended as a fallback strategy. I'm going to rework the interface to take this into account.

> > The problem is that OSMRoadAccessParser only passes TransportationMode.MOTOR_VEHICLE and we would have to store multiple values per edge:
> 
> Hmmh, yeah. For multiple max_speed values like for truck we could introduce max_speed_truck? Not sure.

We could follow the OSM tagging standard and add support for `maxspeed:hgv`?
karussell: Thanks - I like the reworked interface with the maxSpeed as a parameter and return type! Which allows the SpatialRule to adapt or overwrite the provided maxSpeed.

But for which code part or use case do we need the new getDefaultAccess and getDefaultMaxSpeed methods?
otbutz: > Thanks - I like the reworked interface with the maxSpeed as a parameter and return type! Which allows the SpatialRule to adapt or overwrite the provided maxSpeed.
> 
> But for which code part or use case do we need the new getDefaultAccess and getDefaultMaxSpeed methods?

I can move them as `protected abstract` into the `AbstractSpatialRule` class. The idea is to isolate the  `if (current != undefined) { return current } else { return getDefault() }` pattern and keep the child classes clean.
boldtrn: > Except for SpatialRule.EMPTY and a few test classes, all other SpatialRule implementations already extended DefaultSpatialRule. IMHO it's perfectly fine for an abstract class to have some default values which can be overridden by child classes

Are there currently default values like in the DefaultSpatialRule in the AbstractSpatialRule class, not sure if I overlooked this? The idea of the DefaultSpatialRule was to remove some of the code duplication that we would add for almost all countries and only change them, if something differs. On the other hand, having everything setup per country makes it easier to understand the code without always checking the hierarchy values. We still only support Ger and Aut where we already have quite some duplication. I think once we add 10-20 countries, we will copy paste a lot of boiler plate code for every country. So I think I would favor to have the common values set up in some parent class.

> I can move them as protected abstract into the AbstractSpatialRule class.

Yes, I think this would be good, I also stumbled over this when reading the interface :).
otbutz: > Are there currently default values like in the DefaultSpatialRule in the AbstractSpatialRule class, not sure if I overlooked this? The idea of the DefaultSpatialRule was to remove some of the code duplication that we would add for almost all countries and only change them, if something differs. On the other hand, having everything setup per country makes it easier to understand the code without always checking the hierarchy values. We still only support Ger and Aut where we already have quite some duplication. I think once we add 10-20 countries, we will copy paste a lot of boiler plate code for every country. So I think I would favor to have the common values set up in some parent class.

Having to compare two classes in order to check which values are being used, is a rather tedious and error prone process. If we keep the logic down to a simple switch-case it's easier to have each rule define its all values IMHO.
boldtrn: > If we keep the logic down to a simple switch-case it's easier to have each rule define its all values IMHO.

Yes, that's a valid argument as well. I am just a bit unsure, since many countries share the same values or some default values will almost always apply. 

For example, for motor vehicles, this will be almost always be copied, which is not that nice IMHO:
```
        case PATH:
        case BRIDLEWAY:
        case CYCLEWAY:
        case FOOTWAY:
        case PEDESTRIAN:
            return RoadAccess.NO;
        default:
            return RoadAccess.YES;
```

But, it's not a big issue for me, we can always refactor this when we see that it starts to become an issue, it doesn't change the interface.
karussell: > > But for which code part or use case do we need the new getDefaultAccess and getDefaultMaxSpeed methods?

> I can move them as protected abstract into the AbstractSpatialRule class

But why do we need this method? I would found it more transparent if all SpatialRules would do all the work (same argument 'Having to compare two classes in order to check which values are being used').

But to reduce some minor code duplication I would find it acceptable if it is removed from the interface and is package protected in AbstractSpatialRule.
karussell: Should we also consider the proposed change in #1427 from @boldtrn here?
otbutz: > Should we also consider the proposed change in #1427 from @boldtrn here?

If we want external rule files, we have to settle for a subset of possible input variables to determine the values for maxspeed/access/etc. We could use this approach for the interface and use a more restricted one for file based rules but i'm not sure if we want to stick to code based spatial rules in the future.
karussell: @otbutz Thanks for the changes.

Regarding the method return type: why do we return -1 and do not return currentMaxSpeed (or the currentAccess)? With that could avoid the separate getDefaultMaxSpeed method and make this method final:

```java
public class AustriaSpatialRule ...
   @Override
   public final double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
        if (currentMaxSpeed > 0 || transport != TransportationMode.MOTOR_VEHICLE) {
            return currentMaxSpeed;
        }
        switch...
        ...
   }
```

And e.g. for `getAccess` currently always RoadAccess.YES is returned although I would expect no if I specifiy NO for currentRoadAccess (?)
otbutz: > @otbutz Thanks for the changes.
> 
> Regarding the method return type: why do we return -1 and do not return currentMaxSpeed (or the currentAccess)?

Good catch. I will change both methods to return the current value in AbstractSpatialRule.

> With that could avoid the separate getDefaultMaxSpeed method

I already removed them but forgot to push my rebased branch. sigh...

> And e.g. for `getAccess` currently always RoadAccess.YES is returned although I would expect `NO` if I specifiy `NO` for currentRoadAccess (?)

For both AustriaSpatialRule and GermanySpatialRule:
```java
        if (currentRoadAccess != RoadAccess.YES) {
            return currentRoadAccess;
        }
```
So if there is any restriction it will stick to it. For AbstractSpatialRule you're right.
karussell: > I already removed them but forgot to push my rebased branch. sigh...

Ah, thanks, no worries :) !

Patch:
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxSpeedParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxSpeedParser.java
index 00e756659..441b27f68 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxSpeedParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMMaxSpeedParser.java
@@ -22,10 +22,11 @@ import com.graphhopper.routing.profiles.DecimalEncodedValue;
 import com.graphhopper.routing.profiles.EncodedValue;
 import com.graphhopper.routing.profiles.EncodedValueLookup;
 import com.graphhopper.routing.profiles.MaxSpeed;
+import com.graphhopper.routing.profiles.RoadClass;
 import com.graphhopper.routing.util.AbstractFlagEncoder;
 import com.graphhopper.routing.util.spatialrules.SpatialRule;
+import com.graphhopper.routing.util.spatialrules.TransportationMode;
 import com.graphhopper.storage.IntsRef;
-import com.graphhopper.util.shapes.GHPoint;
 
 import java.util.List;
 
@@ -55,13 +56,12 @@ public class OSMMaxSpeedParser implements TagParser {
     public IntsRef handleWayTags(IntsRef edgeFlags, ReaderWay way, boolean ferry, IntsRef relationFlags) {
         double maxSpeed = AbstractFlagEncoder.parseSpeed(way.getTag("maxspeed"));
 
-        if (maxSpeed < 0) {
-            GHPoint estmCentre = way.getTag("estimated_center", null);
-            SpatialRule spatialRule = way.getTag("spatial_rule", null);
-            if (estmCentre != null && spatialRule != null)
-                maxSpeed = spatialRule.getMaxSpeed(way.getTag("highway", ""), maxSpeed);
+        SpatialRule spatialRule = way.getTag("spatial_rule", null);
+        if (spatialRule != null) {
+            RoadClass roadClass = RoadClass.find(way.getTag("highway", ""));
+            maxSpeed = spatialRule.getMaxSpeed(roadClass, TransportationMode.MOTOR_VEHICLE, maxSpeed);
         }
-
+            
         double fwdSpeed = AbstractFlagEncoder.parseSpeed(way.getTag("maxspeed:forward"));
         if (fwdSpeed < 0 && maxSpeed > 0)
             fwdSpeed = maxSpeed;
diff --git a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMRoadAccessParser.java b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMRoadAccessParser.java
index 375f5086c..bd631583d 100644
--- a/core/src/main/java/com/graphhopper/routing/util/parsers/OSMRoadAccessParser.java
+++ b/core/src/main/java/com/graphhopper/routing/util/parsers/OSMRoadAccessParser.java
@@ -22,6 +22,7 @@ import com.graphhopper.routing.profiles.EncodedValue;
 import com.graphhopper.routing.profiles.EncodedValueLookup;
 import com.graphhopper.routing.profiles.EnumEncodedValue;
 import com.graphhopper.routing.profiles.RoadAccess;
+import com.graphhopper.routing.profiles.RoadClass;
 import com.graphhopper.routing.util.spatialrules.SpatialRule;
 import com.graphhopper.routing.util.spatialrules.TransportationMode;
 import com.graphhopper.storage.IntsRef;
@@ -60,11 +61,9 @@ public class OSMRoadAccessParser implements TagParser {
             }
         }
 
-        if (accessValue == RoadAccess.YES) {
-            SpatialRule spatialRule = readerWay.getTag("spatial_rule", null);
-            if (spatialRule != null)
-                accessValue = spatialRule.getAccess(readerWay.getTag("highway", ""), TransportationMode.MOTOR_VEHICLE, YES);
-        }
+        SpatialRule spatialRule = readerWay.getTag("spatial_rule", null);
+        if (spatialRule != null)
+            accessValue = spatialRule.getAccess(RoadClass.find(readerWay.getTag("highway", "")), TransportationMode.MOTOR_VEHICLE, YES);
 
         roadAccessEnc.setEnum(false, edgeFlags, accessValue);
         return edgeFlags;
diff --git a/core/src/main/java/com/graphhopper/routing/util/spatialrules/AbstractSpatialRule.java b/core/src/main/java/com/graphhopper/routing/util/spatialrules/AbstractSpatialRule.java
index 4f5a56572..423f7dde3 100644
--- a/core/src/main/java/com/graphhopper/routing/util/spatialrules/AbstractSpatialRule.java
+++ b/core/src/main/java/com/graphhopper/routing/util/spatialrules/AbstractSpatialRule.java
@@ -17,42 +17,52 @@
  */
 package com.graphhopper.routing.util.spatialrules;
 
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
 import org.locationtech.jts.geom.Polygon;
 
+import com.graphhopper.routing.profiles.RoadAccess;
+import com.graphhopper.routing.profiles.RoadClass;
+
 /**
  * @author Robin Boldt
  */
 public abstract class AbstractSpatialRule implements SpatialRule {
 
-    protected List<Polygon> polygons = Collections.emptyList();
-
-    public List<Polygon> getBorders() {
-        return polygons;
+    private final List<Polygon> borders;
+    
+    public AbstractSpatialRule(List<Polygon> borders) {
+        this.borders = borders;
     }
-
-    public SpatialRule setBorders(List<Polygon> polygons) {
-        this.polygons = polygons;
-        return this;
+    
+    public AbstractSpatialRule(Polygon border) {
+        this(Collections.singletonList(border));
+    }
+    
+    @Override
+    public double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
+        return currentMaxSpeed;
     }
 
-    public SpatialRule addBorder(Polygon polygon) {
-        if (polygons.isEmpty())
-            polygons = new ArrayList<>();
-        this.polygons.add(polygon);
-        return this;
+    @Override
+    public RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess) {
+        return currentRoadAccess;
+    }
+    
+    public List<Polygon> getBorders() {
+        return borders;
     }
 
     @Override
     public boolean equals(Object obj) {
-        if (obj instanceof SpatialRule) {
-            if (((SpatialRule) obj).getId().equals(this.getId()))
-                return true;
+        if (this == obj) {
+            return true;
+        }
+        if (!(obj instanceof SpatialRule)) {
+            return false;
         }
-        return false;
+        return this.getId().equals(((SpatialRule) obj).getId());
     }
 
     @Override
diff --git a/core/src/main/java/com/graphhopper/routing/util/spatialrules/CountriesSpatialRuleFactory.java b/core/src/main/java/com/graphhopper/routing/util/spatialrules/CountriesSpatialRuleFactory.java
index 5b0e17ade..425743657 100644
--- a/core/src/main/java/com/graphhopper/routing/util/spatialrules/CountriesSpatialRuleFactory.java
+++ b/core/src/main/java/com/graphhopper/routing/util/spatialrules/CountriesSpatialRuleFactory.java
@@ -17,13 +17,9 @@ public class CountriesSpatialRuleFactory implements SpatialRuleLookupBuilder.Spa
             Country country = Country.valueOf(toUpperCase(id));
             switch (country) {
                 case AUT:
-                    AustriaSpatialRule austriaSpatialRule = new AustriaSpatialRule();
-                    austriaSpatialRule.setBorders(polygons);
-                    return austriaSpatialRule;
+                    return new AustriaSpatialRule(polygons);
                 case DEU:
-                    GermanySpatialRule germanySpatialRule = new GermanySpatialRule();
-                    germanySpatialRule.setBorders(polygons);
-                    return germanySpatialRule;
+                    return new GermanySpatialRule(polygons);
             }
         } catch (Exception ex) {
         }
diff --git a/core/src/main/java/com/graphhopper/routing/util/spatialrules/DefaultSpatialRule.java b/core/src/main/java/com/graphhopper/routing/util/spatialrules/DefaultSpatialRule.java
deleted file mode 100644
index 137c48756..000000000
--- a/core/src/main/java/com/graphhopper/routing/util/spatialrules/DefaultSpatialRule.java
+++ /dev/null
@@ -1,76 +0,0 @@
-/*
- *  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.routing.util.spatialrules;
-
-import com.graphhopper.routing.profiles.RoadAccess;
-
-/**
- * Default implementation for the SpatialRule that contains the current default values
- */
-public class DefaultSpatialRule extends AbstractSpatialRule {
-
-    @Override
-    public double getMaxSpeed(String highwayTag, double _default) {
-        // We tried to estimate reasonable values: https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Maxspeed#Motorcar
-        // We did not always used the highest value available, but we used a high value
-        switch (highwayTag) {
-            case "motorway":
-                return 130;
-            case "trunk":
-                return 130;
-            case "primary":
-                return 100;
-            case "secondary":
-                return 100;
-            case "tertiary":
-                return 100;
-            case "unclassified":
-                return 100;
-            case "residential":
-                return 90;
-            case "living_street":
-                return 20;
-            default:
-                return _default;
-        }
-    }
-
-    @Override
-    public RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default) {
-        // As defined in: https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Access-Restriction
-        // We tried to find generally forbidden tags
-        if (transportationMode == TransportationMode.MOTOR_VEHICLE) {
-            switch (highwayTag) {
-                case "path":
-                case "bridleway":
-                case "cycleway":
-                case "footway":
-                case "pedestrian":
-                    return RoadAccess.NO;
-                default:
-                    return _default;
-            }
-        }
-        return _default;
-    }
-
-    @Override
-    public String getId() {
-        throw new UnsupportedOperationException("No id for the DefaultSpatialRule");
-    }
-}
diff --git a/core/src/main/java/com/graphhopper/routing/util/spatialrules/SpatialRule.java b/core/src/main/java/com/graphhopper/routing/util/spatialrules/SpatialRule.java
index 95e9172d3..afb0b047a 100644
--- a/core/src/main/java/com/graphhopper/routing/util/spatialrules/SpatialRule.java
+++ b/core/src/main/java/com/graphhopper/routing/util/spatialrules/SpatialRule.java
@@ -18,6 +18,7 @@
 package com.graphhopper.routing.util.spatialrules;
 
 import com.graphhopper.routing.profiles.RoadAccess;
+import com.graphhopper.routing.profiles.RoadClass;
 
 import java.util.List;
 import java.util.Objects;
@@ -35,23 +36,24 @@ import org.locationtech.jts.geom.Polygon;
 public interface SpatialRule {
 
     /**
-     * Return the max speed for a certain highway type. If there is no max speed defined, _default will be returned.
+     * Return the max speed for a certain road class and transportation mode.
      *
-     * @param highway  The highway type, e.g. primary, secondary
-     * @param _default The default max speed
-     * @return max speed
+     * @param roadClass       The highway type, e.g. {@link RoadClass#MOTORWAY}
+     * @param transport       The mode of transportation
+     * @param currentMaxSpeed The current max speed value or -1 if no value has been set yet
+     * @return the maximum speed value to be used
      */
-    double getMaxSpeed(String highway, double _default);
-
+    double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed);
+    
     /**
-     * Returns the {@link RoadAccess} for a certain highway type and transportation mode. If nothing is defined,
-     * _default will be returned.
+     * Returns the {@link RoadAccess} for a certain highway type and transportation transport.
      *
-     * @param highwayTag         The highway type, e.g. primary, secondary
-     * @param transportationMode The mode of transportation
-     * @param _default           The default AccessValue
+     * @param roadClass          The highway type, e.g. {@link RoadClass#MOTORWAY}
+     * @param transport          The mode of transportation
+     * @param currentRoadAccess  The current road access value (default: {@link RoadAccess#YES})
+     * @return the type of access to be used
      */
-    RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default);
+    RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess);
 
     /**
      * Returns the borders in which the SpatialRule is valid
@@ -63,15 +65,16 @@ public interface SpatialRule {
      */
     String getId();
 
+
     SpatialRule EMPTY = new SpatialRule() {
         @Override
-        public double getMaxSpeed(String highwayTag, double _default) {
-            return _default;
+        public double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
+            return currentMaxSpeed;
         }
 
         @Override
-        public RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default) {
-            return _default;
+        public RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess) {
+            return currentRoadAccess;
         }
 
         // should we use Country.DEFAULT here?
diff --git a/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/AustriaSpatialRule.java b/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/AustriaSpatialRule.java
index a5e262af3..95e57dbec 100644
--- a/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/AustriaSpatialRule.java
+++ b/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/AustriaSpatialRule.java
@@ -17,9 +17,14 @@
  */
 package com.graphhopper.routing.util.spatialrules.countries;
 
+import java.util.List;
+
+import org.locationtech.jts.geom.Polygon;
+
 import com.graphhopper.routing.profiles.Country;
 import com.graphhopper.routing.profiles.RoadAccess;
-import com.graphhopper.routing.util.spatialrules.DefaultSpatialRule;
+import com.graphhopper.routing.profiles.RoadClass;
+import com.graphhopper.routing.util.spatialrules.AbstractSpatialRule;
 import com.graphhopper.routing.util.spatialrules.TransportationMode;
 
 /**
@@ -27,31 +32,64 @@ import com.graphhopper.routing.util.spatialrules.TransportationMode;
  *
  * @author Robin Boldt
  */
-public class AustriaSpatialRule extends DefaultSpatialRule {
+public class AustriaSpatialRule extends AbstractSpatialRule {
 
+    public AustriaSpatialRule(List<Polygon> borders) {
+        super(borders);
+    }
+    
     @Override
-    public double getMaxSpeed(String highwayTag, double _default) {
-        // As defined in: https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Maxspeed#Motorcar
-        switch (highwayTag) {
-            case "trunk":
-                return 100;
-            case "residential":
-                return 50;
-            default:
-                return super.getMaxSpeed(highwayTag, _default);
+    public double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
+        if (currentMaxSpeed > 0 || transport != TransportationMode.MOTOR_VEHICLE) {
+            return currentMaxSpeed;
+        }
+        
+        switch (roadClass) {
+        case MOTORWAY:
+            return 130;
+        case TRUNK:
+            return 100;
+        case PRIMARY:
+            return 100;
+        case SECONDARY:
+            return 100;
+        case TERTIARY:
+            return 100;
+        case UNCLASSIFIED:
+            return 100;
+        case RESIDENTIAL:
+            return 50;
+        case LIVING_STREET:
+            return 20;
+        default:
+            return -1;
         }
     }
-
+    
     @Override
-    public RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default) {
-        if (transportationMode == TransportationMode.MOTOR_VEHICLE) {
-            if (highwayTag.equals("living_street"))
-                return RoadAccess.DESTINATION;
-            if (highwayTag.equals("track"))
-                return RoadAccess.FORESTRY;
+    public RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess) {
+        if (currentRoadAccess != RoadAccess.YES) {
+            return currentRoadAccess;
+        }
+        
+        if (transport != TransportationMode.MOTOR_VEHICLE) {
+            return RoadAccess.YES;
+        }
+        
+        switch (roadClass) {
+        case LIVING_STREET:
+            return RoadAccess.DESTINATION;
+        case TRACK:
+            return RoadAccess.FORESTRY;
+        case PATH:
+        case BRIDLEWAY:
+        case CYCLEWAY:
+        case FOOTWAY:
+        case PEDESTRIAN:
+            return RoadAccess.NO;
+        default:
+            return RoadAccess.YES;
         }
-
-        return super.getAccess(highwayTag, transportationMode, _default);
     }
 
     @Override
diff --git a/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/GermanySpatialRule.java b/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/GermanySpatialRule.java
index 04f654fc5..7cb35e5a4 100644
--- a/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/GermanySpatialRule.java
+++ b/core/src/main/java/com/graphhopper/routing/util/spatialrules/countries/GermanySpatialRule.java
@@ -17,9 +17,15 @@
  */
 package com.graphhopper.routing.util.spatialrules.countries;
 
+import java.util.List;
+
+import org.locationtech.jts.geom.Polygon;
+
 import com.graphhopper.routing.profiles.Country;
+import com.graphhopper.routing.profiles.MaxSpeed;
 import com.graphhopper.routing.profiles.RoadAccess;
-import com.graphhopper.routing.util.spatialrules.DefaultSpatialRule;
+import com.graphhopper.routing.profiles.RoadClass;
+import com.graphhopper.routing.util.spatialrules.AbstractSpatialRule;
 import com.graphhopper.routing.util.spatialrules.TransportationMode;
 
 /**
@@ -27,36 +33,68 @@ import com.graphhopper.routing.util.spatialrules.TransportationMode;
  *
  * @author Robin Boldt
  */
-public class GermanySpatialRule extends DefaultSpatialRule {
+public class GermanySpatialRule extends AbstractSpatialRule {
+    
+    public GermanySpatialRule(List<Polygon> borders) {
+        super(borders);
+    }
 
     /**
-     * Germany contains roads with no speed limit. For these roads, this method will return Integer.MAX_VALUE.
+     * Germany contains roads with no speed limit. For these roads, this method
+     * will return {@link MaxSpeed#UNLIMITED_SIGN_SPEED}.
+     * <p>
      * Your implementation should be able to handle these cases.
      */
     @Override
-    public double getMaxSpeed(String highwayTag, double _default) {
+    public double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
+        if (currentMaxSpeed > 0 || transport != TransportationMode.MOTOR_VEHICLE) {
+            return currentMaxSpeed;
+        }
+        
         // As defined in: https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Maxspeed#Motorcar
-        switch (highwayTag) {
-            case "motorway":
-            case "trunk":
-                return Integer.MAX_VALUE;
-            case "residential":
+        switch (roadClass) {
+            case MOTORWAY:
+            case TRUNK:
+                return MaxSpeed.UNLIMITED_SIGN_SPEED;
+            case PRIMARY:
+                return 100;
+            case SECONDARY:
+                return 100;
+            case TERTIARY:
                 return 100;
-            case "living_street":
+            case UNCLASSIFIED:
+                return 100;
+            case RESIDENTIAL:
+                return 100;
+            case LIVING_STREET:
                 return 4;
             default:
-                return super.getMaxSpeed(highwayTag, _default);
+                return -1;
         }
     }
-
+    
     @Override
-    public RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default) {
-        if (transportationMode == TransportationMode.MOTOR_VEHICLE) {
-            if (highwayTag.equals("track"))
-                return RoadAccess.DESTINATION;
+    public RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess) {
+        if (currentRoadAccess != RoadAccess.YES) {
+            return currentRoadAccess;
+        }
+        
+        if (transport != TransportationMode.MOTOR_VEHICLE) {
+            return RoadAccess.YES;
         }
 
-        return super.getAccess(highwayTag, transportationMode, _default);
+        switch (roadClass) {
+        case TRACK:
+            return RoadAccess.DESTINATION;
+        case PATH:
+        case BRIDLEWAY:
+        case CYCLEWAY:
+        case FOOTWAY:
+        case PEDESTRIAN:
+            return RoadAccess.NO;
+        default:
+            return RoadAccess.YES;
+        }
     }
 
     @Override
diff --git a/reader-osm/src/main/java/com/graphhopper/reader/osm/GraphHopperOSM.java b/reader-osm/src/main/java/com/graphhopper/reader/osm/GraphHopperOSM.java
index 7f3a130f5..51e90f984 100644
--- a/reader-osm/src/main/java/com/graphhopper/reader/osm/GraphHopperOSM.java
+++ b/reader-osm/src/main/java/com/graphhopper/reader/osm/GraphHopperOSM.java
@@ -21,7 +21,7 @@ import com.graphhopper.GraphHopper;
 import com.graphhopper.json.geo.JsonFeatureCollection;
 import com.graphhopper.reader.DataReader;
 import com.graphhopper.routing.lm.PrepareLandmarks;
-import com.graphhopper.routing.util.spatialrules.DefaultSpatialRule;
+import com.graphhopper.routing.util.spatialrules.AbstractSpatialRule;
 import com.graphhopper.routing.util.spatialrules.SpatialRule;
 import com.graphhopper.routing.util.spatialrules.SpatialRuleLookup;
 import com.graphhopper.routing.util.spatialrules.SpatialRuleLookupBuilder;
@@ -74,15 +74,17 @@ public class GraphHopperOSM extends GraphHopper {
 
         if (landmarkSplittingFeatureCollection != null && !landmarkSplittingFeatureCollection.getFeatures().isEmpty()) {
             SpatialRuleLookup ruleLookup = SpatialRuleLookupBuilder.buildIndex(
-                    Collections.singletonList(landmarkSplittingFeatureCollection), "area", new SpatialRuleLookupBuilder.SpatialRuleFactory() {
+                    Collections.singletonList(landmarkSplittingFeatureCollection), "area",
+                    new SpatialRuleLookupBuilder.SpatialRuleFactory() {
                         @Override
-                        public SpatialRule createSpatialRule(final String id, List<Polygon> polygons) {
-                            return new DefaultSpatialRule() {
+                        public SpatialRule createSpatialRule(final String id,
+                                        List<Polygon> polygons) {
+                            return new AbstractSpatialRule(polygons) {
                                 @Override
                                 public String getId() {
                                     return id;
                                 }
-                            }.setBorders(polygons);
+                            };
                         }
                     });
             for (PrepareLandmarks prep : getLMPreparationHandler().getPreparations()) {


Test patch:
diff --git a/core/src/test/java/com/graphhopper/routing/lm/LandmarkStorageTest.java b/core/src/test/java/com/graphhopper/routing/lm/LandmarkStorageTest.java
index 589acacd4..b599b558a 100644
--- a/core/src/test/java/com/graphhopper/routing/lm/LandmarkStorageTest.java
+++ b/core/src/test/java/com/graphhopper/routing/lm/LandmarkStorageTest.java
@@ -21,7 +21,7 @@ import com.graphhopper.routing.RoutingAlgorithmTest;
 import com.graphhopper.routing.util.CarFlagEncoder;
 import com.graphhopper.routing.util.EncodingManager;
 import com.graphhopper.routing.util.FlagEncoder;
-import com.graphhopper.routing.util.spatialrules.DefaultSpatialRule;
+import com.graphhopper.routing.util.spatialrules.AbstractSpatialRule;
 import com.graphhopper.routing.util.spatialrules.SpatialRule;
 import com.graphhopper.routing.util.spatialrules.SpatialRuleLookup;
 import com.graphhopper.routing.weighting.FastestWeighting;
@@ -36,8 +36,10 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.locationtech.jts.geom.Envelope;
+import org.locationtech.jts.geom.Polygon;
 
 import java.util.Arrays;
+import java.util.Collections;
 
 import static org.junit.Assert.*;
 
@@ -186,13 +188,13 @@ public class LandmarkStorageTest {
         RoutingAlgorithmTest.initBiGraph(ghStorage);
 
         LandmarkStorage storage = new LandmarkStorage(ghStorage, new RAMDirectory(), new LMProfile(new FastestWeighting(encoder)), 2);
-        final SpatialRule ruleRight = new DefaultSpatialRule() {
+        final SpatialRule ruleRight = new AbstractSpatialRule(Collections.<Polygon>emptyList()) {
             @Override
             public String getId() {
                 return "right";
             }
         };
-        final SpatialRule ruleLeft = new DefaultSpatialRule() {
+        final SpatialRule ruleLeft = new AbstractSpatialRule(Collections.<Polygon>emptyList()) {
             @Override
             public String getId() {
                 return "left";
diff --git a/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupBuilderTest.java b/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupBuilderTest.java
index a8542af89..38e17493a 100644
--- a/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupBuilderTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupBuilderTest.java
@@ -64,23 +64,23 @@ public class SpatialRuleLookupBuilderTest {
 
         // Berlin
         assertEquals(RoadAccess.DESTINATION, spatialRuleLookup.lookupRule(52.5243700, 13.4105300).
-                getAccess("track", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.TRACK, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
         assertEquals(RoadAccess.YES, spatialRuleLookup.lookupRule(52.5243700, 13.4105300).
-                getAccess("primary", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.PRIMARY, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
 
         // Paris -> empty rule
         assertEquals(RoadAccess.YES, spatialRuleLookup.lookupRule(48.864716, 2.349014).
-                getAccess("track", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.TRACK, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
         assertEquals(RoadAccess.YES, spatialRuleLookup.lookupRule(48.864716, 2.349014).
-                getAccess("primary", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.PRIMARY, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
 
         // Austria
         assertEquals(RoadAccess.FORESTRY, spatialRuleLookup.lookupRule(48.204484, 16.107888).
-                getAccess("track", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.TRACK, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
         assertEquals(RoadAccess.YES, spatialRuleLookup.lookupRule(48.210033, 16.363449).
-                getAccess("primary", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.PRIMARY, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
         assertEquals(RoadAccess.DESTINATION, spatialRuleLookup.lookupRule(48.210033, 16.363449).
-                getAccess("living_street", TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
+                getAccess(RoadClass.LIVING_STREET, TransportationMode.MOTOR_VEHICLE, RoadAccess.YES));
     }
 
     @Test
@@ -122,8 +122,7 @@ public class SpatialRuleLookupBuilderTest {
         final GeometryFactory fac = new GeometryFactory();
         org.locationtech.jts.geom.Polygon polygon = fac.createPolygon(new Coordinate[]{
                 new Coordinate(0, 0), new Coordinate(0, 1), new Coordinate(1, 1), new Coordinate(1, 0), new Coordinate(0, 0)});
-        final GermanySpatialRule germany = new GermanySpatialRule();
-        germany.setBorders(Collections.singletonList(polygon));
+        final GermanySpatialRule germany = new GermanySpatialRule(Collections.singletonList(polygon));
 
         SpatialRuleLookup index = new SpatialRuleLookup() {
             @Override
@@ -196,7 +195,7 @@ public class SpatialRuleLookupBuilderTest {
         e2.setFlags(em.handleWayTags(way2, map, relFlags));
         assertEquals(RoadAccess.YES, e2.get(tmpRoadAccessEnc));
 
-        assertEquals(index.getSpatialId(new GermanySpatialRule()), e1.get(countrySpatialIdEnc));
+        assertEquals(index.getSpatialId(new GermanySpatialRule(Collections.singletonList(polygon))), e1.get(countrySpatialIdEnc));
         assertEquals(index.getSpatialId(SpatialRule.EMPTY), e2.get(countrySpatialIdEnc));
 
         ReaderWay livingStreet = new ReaderWay(29L);
@@ -219,31 +218,21 @@ public class SpatialRuleLookupBuilderTest {
 
             @Override
             public SpatialRule createSpatialRule(final String id, final List<Polygon> borders) {
-                return new SpatialRule() {
-
+                return new AbstractSpatialRule(borders) {
+                    
                     @Override
-                    public double getMaxSpeed(String highway, double _default) {
+                    public double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
                         return 100;
                     }
 
                     @Override
-                    public String getId() {
-                        return id;
-                    }
-
-                    @Override
-                    public List<Polygon> getBorders() {
-                        return borders;
-                    }
-
-                    @Override
-                    public RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default) {
+                    public RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess) {
                         return RoadAccess.YES;
                     }
 
                     @Override
-                    public String toString() {
-                        return getId();
+                    public String getId() {
+                        return id;
                     }
                 };
             }
diff --git a/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupJTSTest.java b/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupJTSTest.java
index 69fbfcdce..dda374e5a 100644
--- a/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupJTSTest.java
+++ b/core/src/test/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupJTSTest.java
@@ -1,6 +1,8 @@
 package com.graphhopper.routing.util.spatialrules;
 
 import com.graphhopper.routing.profiles.RoadAccess;
+import com.graphhopper.routing.profiles.RoadClass;
+
 import org.junit.Test;
 import org.locationtech.jts.geom.Coordinate;
 import org.locationtech.jts.geom.Envelope;
@@ -26,21 +28,21 @@ public class SpatialRuleLookupJTSTest {
     public void testSpatialLookup() {
         Polygon deBorder = FAC.createPolygon(new Coordinate[] { new Coordinate(1, 1), new Coordinate(2, 1), new Coordinate(2, 2), new Coordinate(1, 2), new Coordinate(1, 1) });
         List<SpatialRule> spatialRules = new ArrayList<>();
-        SpatialRule germany = new DefaultSpatialRule() {
+        SpatialRule germany = new AbstractSpatialRule(deBorder) {
             @Override
             public String getId() {
                 return "DEU";
             }
-        }.addBorder(deBorder);
+        };
         spatialRules.add(germany);
         
         Polygon atBorder = FAC.createPolygon(new Coordinate[] { new Coordinate(5, 5), new Coordinate(6, 5), new Coordinate(6, 6), new Coordinate(5, 6), new Coordinate(5, 5) });
-        SpatialRule austria = new DefaultSpatialRule() {
+        SpatialRule austria = new AbstractSpatialRule(atBorder) {
             @Override
             public String getId() {
                 return "AUT";
             }
-        }.addBorder(atBorder);
+        };
         spatialRules.add(austria);
 
         // create lookup with bbox just for DEU (for space reduction)
@@ -61,12 +63,12 @@ public class SpatialRuleLookupJTSTest {
         String germanPolygonJson = "[9.921906,54.983104],[9.93958,54.596642],[10.950112,54.363607],[10.939467,54.008693],[11.956252,54.196486],[12.51844,54.470371],[13.647467,54.075511],[14.119686,53.757029],[14.353315,53.248171],[14.074521,52.981263],[14.4376,52.62485],[14.685026,52.089947],[14.607098,51.745188],[15.016996,51.106674],[14.570718,51.002339],[14.307013,51.117268],[14.056228,50.926918],[13.338132,50.733234],[12.966837,50.484076],[12.240111,50.266338],[12.415191,49.969121],[12.521024,49.547415],[13.031329,49.307068],[13.595946,48.877172],[13.243357,48.416115],[12.884103,48.289146],[13.025851,47.637584],[12.932627,47.467646],[12.62076,47.672388],[12.141357,47.703083],[11.426414,47.523766],[10.544504,47.566399],[10.402084,47.302488],[9.896068,47.580197],[9.594226,47.525058],[8.522612,47.830828],[8.317301,47.61358],[7.466759,47.620582],[7.593676,48.333019],[8.099279,49.017784],[6.65823,49.201958],[6.18632,49.463803],[6.242751,49.902226],[6.043073,50.128052],[6.156658,50.803721],[5.988658,51.851616],[6.589397,51.852029],[6.84287,52.22844],[7.092053,53.144043],[6.90514,53.482162],[7.100425,53.693932],[7.936239,53.748296],[8.121706,53.527792],[8.800734,54.020786],[8.572118,54.395646],[8.526229,54.962744],[9.282049,54.830865],[9.921906,54.983104]";
         Polygon germanPolygon = parsePolygonString(germanPolygonJson);
 
-        spatialRules.add(new DefaultSpatialRule() {
+        spatialRules.add(new AbstractSpatialRule(germanPolygon) {
             @Override
             public String getId() {
                 return "DEU";
             }
-        }.addBorder(germanPolygon));
+        });
         SpatialRuleLookup spatialRuleLookup = new SpatialRuleLookupJTS(spatialRules, new Envelope(-180, 180, -90, 90));
 
         // Far from the border of Germany, in Germany
@@ -147,14 +149,15 @@ public class SpatialRuleLookupJTSTest {
     }
 
     private SpatialRule getSpatialRule(Polygon p, final String name) {
-        return new AbstractSpatialRule() {
+        return new AbstractSpatialRule(p) {
+            
             @Override
-            public double getMaxSpeed(String highwayTag, double _default) {
-                return _default;
+            public double getMaxSpeed(RoadClass roadClass, TransportationMode transport, double currentMaxSpeed) {
+                return currentMaxSpeed;
             }
 
             @Override
-            public RoadAccess getAccess(String highwayTag, TransportationMode transportationMode, RoadAccess _default) {
+            public RoadAccess getAccess(RoadClass roadClass, TransportationMode transport, RoadAccess currentRoadAccess) {
                 return RoadAccess.DESTINATION;
             }
 
@@ -162,6 +165,6 @@ public class SpatialRuleLookupJTSTest {
             public String getId() {
                 return name;
             }
-        }.addBorder(p);
+        };
     }
 }