SpatialRule interface rework

[otbutz]

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

[karussell]

Thanks! Looks good.

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

merge DefaultSpatialRule into AbstractSpatialRule

What is the reason here?

allow rules to overwrite existing values instead of only being used as a fallback

Can you link to the code change here?

[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:

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 for the default, as 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:

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 😉

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)
• 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:

👍

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]

Really nice work @otbutz 👍.

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:

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:

        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 :) !

Looks good now.

[karussell]

Thanks @otbutz !