Fix for #2980, block highway=cycleway with access=no #2981

ratrun · 2024-04-21T13:57:17Z

No description provided.

karussell · 2024-04-21T15:03:33Z

Why do we treat cycleway as special? Shouldn't we allow vehicle=no for all highway values? And reduce to walking speed.

# Conflicts: # core/src/test/java/com/graphhopper/routing/util/parsers/BikeTagParserTest.java # core/src/test/java/com/graphhopper/routing/util/parsers/OSMGetOffBikeParserTest.java

ratrun · 2024-04-21T15:59:10Z

Why do we treat cycleway as special? Shouldn't we allow vehicle=no for all highway values?

Yes, you are right. Thanks! I'll update.

Reduction to walking speed is included now

ratrun · 2024-04-21T16:39:29Z

Please recheck. I think that all requested improvements are included now.

zidel · 2024-04-21T16:54:33Z

bicycle=no should probably also be included in the fallback to pushing. Not sure how often it occurs, but should that fallback branch check if walking is allowed?

karussell · 2024-04-21T17:07:01Z

core/src/main/java/com/graphhopper/routing/util/parsers/BikeCommonAccessParser.java

@@ -76,10 +76,13 @@ public WayAccess getAccess(ReaderWay way) {
                return WayAccess.CAN_SKIP;
        }

-        // use the way if it is tagged for bikes
-        if (way.hasTag("bicycle", "dismount") || way.hasTag("highway", "cycleway"))


what if we have vehicle=no and bicycle=no or even access=no? I think we have to somehow integrate this and the next case into the "restriction loop" (for(String value:restrict))

We need to block these combinations, I prepared a change.

ratrun · 2024-04-21T17:56:49Z

bicycle=no should probably also be included in the fallback to pushing. Not sure how often it occurs, but should that fallback branch check if walking is allowed?

No, I think that this would be too controversial as it would allow bicycles also on highway=path, highway=footway or highway=path in combination with bicycle=no. This would allow bicycles basically everywhere where foot is allowed.

ratrun · 2024-04-22T05:05:28Z

Why do we treat cycleway as special? Shouldn't we allow vehicle=no for all highway values?

Yes, you are right. Thanks! I'll update.

Looking at the details of this change I found out that it would also modify the access result for the tag combination highway=track or highway=path with vehicle=no. Currently we are blocking this combination and we would allow it afterwards. As this combination is often used in forest areas and most people don't get off their bikes there I'm not sure if this is what we really want. I just verified that komoot allows this combination, but there are many discussions about it even in Germany (Stichwort "Bundeswaldgesetz") and in Austria it is currently clearly forbidden according to the "Forstgesetz".

So I'm not sure if this change is really a good idea.

…re if this is a good idea

otbutz · 2024-04-22T07:13:59Z

No, I think that this would be too controversial as it would allow bicycles also on highway=path, highway=footway or highway=path in combination with bicycle=no. This would allow bicycles basically everywhere where foot is allowed.

In Germany, at least, that's fine as long as you're pushing your bike. But that means we have to assume walking speed and display a dismount warning.

ratrun · 2024-04-22T15:55:42Z

But that means we have to assume walking speed and display a dismount warning.

I forgot about this recent cool feature. With this functionality my previous concerns are not valid any more. Therefore I'm setting the getOffBikeEnc now and I verified that the dismount warning is displayed correctly for the tag combinations highway!=cycleway together with vehicle=no. I should have covered all comments, please re-check.

karussell · 2024-04-22T19:00:50Z

Partly you are right @ratrun because e.g. on openstreetmap.org there are no route hints and most mappers probably use this. Could we further avoid this? Maybe e.g. avoid the cases where get_off_bike==true in the custom model bike.json?

bicycle=no should probably also be included in the fallback to pushing.

yes, could be. @ratrun (?)

Not sure how often it occurs, but should that fallback branch check if walking is allowed?

Yes, this would be important if the mapping is a bit strange e.g. for vehicle=no and foot=no instead of access=no.

ratrun · 2024-04-23T04:42:58Z

Could we further avoid this? Maybe e.g. avoid the cases where get_off_bike==true in the custom model bike.json?

I think that this would require to change the BooleanEncodedValue of GetOffBike into an IntEncodedValue or an EnumEncodedValue such that it becomes possible to define in the custom model somehow to use bicycle=dismount but block the combination of highway!=cycleway with vehicle=no. This way it would also become possible to define a separate value encoded value for the bicycle=no case and cover the case for the Openstreetmap.org website, where no route hints are available.

otbutz · 2024-04-23T07:57:27Z

e.g. on openstreetmap.org there are no route hints and most mappers probably use this. Could we further avoid this?

A bit of a slippery slope, isn't it? Following that line of reasoning, we'd have to block off all sections of road marked for dismounts or carrying other important routing information. Or to put it another way, this problem needs to be solved in the right place, which IMHO is the OSM website.

I think that this would require to change the BooleanEncodedValue of GetOffBike into an IntEncodedValue or an EnumEncodedValue

That sounds like a good idea. Being told to get off the bike by a traffic sign and voluntarily switching to pedestrian are two different things and should be treated as such.

ratrun · 2024-04-24T05:04:53Z

Being told to get off the bike by a traffic sign and voluntarily switching to pedestrian are two different things and should be treated as such.

I'm not sure if we can support such a distinction. My suggestion for this new GetOffBike EnumEncodedValue is:

FALSE (current false value)
TRUE_WAY_TYPE (covering not suitable highway types and railway=platform)
TRUE_BICYCLE_NO (covering tag bicycle=no, but allowed for foot)
TRUE_OTHER (covering all other cases like bicycle=dismount, vehicle=no)

otbutz · 2024-04-24T07:24:51Z

NO
IMPLICIT -> acting as a pedestrian
TRAFFIC_SIGN -> bicycle=dismount, vehicle=no

TRUE_BICYCLE_NO (covering tag bicycle=no, but allowed for foot)

If we're not allowed to use the road either way, we can also keep FALSE/NO.

ratrun · 2024-04-27T15:21:26Z

@otbutz

NO
IMPLICIT -> acting as a pedestrian
TRAFFIC_SIGN -> bicycle=dismount, vehicle=no

OK.

If we're not allowed to use the road either way, we can also keep FALSE/NO.

I'm not sure if I understood this one. Using the value NO for a GetOffBike value does not sound very logical in this case. I would interpret the NO value as I don't need to get off the bike here and may keep on cycling. The NO value is used for a example on a road tagged as highway=cycleway without any access restrictions, which is pretty much the opposite in terms of suitability for bicycles. Or do you mean this is OK because we have the bike_access encoded value anyway in addition and this has higher importance?

@karussell What do you think about the idea of changing the current BooleanEncodedValue into a EnumEncodedValue ? If you agree on such a change, shall I extend this PR here our would you like a separate PR for that? I think that I should be able to also provide a corresponding PR for the graphhopper-maps project as well.

otbutz · 2024-04-29T07:03:33Z

Or do you mean this is OK because we have the bike_access encoded value anyway in addition and this has higher importance?

Exactly. If access is prohibited, there's no point in duplicating this information in a GetOffBike enum value.

karussell · 2024-05-13T17:55:33Z

IMPLICIT -> acting as a pedestrian

isn't this accessibility information already available via foot_access && !bike_access?

@karussell What do you think about the idea of changing the current BooleanEncodedValue into a EnumEncodedValue ?

Currently I do not see a reason for this.

otbutz · 2024-05-14T07:38:07Z

isn't this accessibility information already available via foot_access && !bike_access?

Good point

ratrun · 2024-05-16T15:19:29Z

isn't this accessibility information already available via foot_access && !bike_access?

How would a custom model for bicycles theoretically look like which routes on such a combination? I put foot_access and bike_access to graph.encoded_values and tried

{
  "priority": [
    { "if": "!bike_access && foot_access",  "multiply_by": "1.0" }
  ],
  "speed": [
    { "if": "!bike_access && foot_access",  "limit_to": "5.0" }
  ]
}

but this does not work. These edges are not considered as they are still not accessible for a bike. I guess that this is an issue which would need to be considered separately even if we changed the EV for the get_off_bike EV into an enum.

Now that we have discussed the enum idea this PR is ready for merge from my point of view. Could you please check again?

karussell · 2024-05-16T16:16:06Z

How would a custom model for bicycles theoretically look like which routes on such a combination?

For the latest stable 9.1 you can modify bike.json and include foot_access similarly how we soften the reverse direction of oneways with backward_bike_access.

(For older versions you would have to use the roads profile)

this PR is ready for merge from my point of view. Could you please check again?

will do

karussell

Instead of the current code in BikeCommonAccessParser: could we not just remove the vehicle key from the restrictionKeys? Because without it this would make the code more complex and this also leads to the following inconsistency: for vehicle=forestry the access is forbidden, but for vehicle=no it is allowed.

karussell · 2024-05-21T15:14:43Z

core/src/main/java/com/graphhopper/routing/util/parsers/OSMGetOffBikeParser.java

@@ -35,6 +35,7 @@ public void handleWayTags(int edgeId, EdgeIntAccess edgeIntAccess, ReaderWay way
        boolean notIntended = !way.hasTag("bicycle", INTENDED) &&
                (GET_OFF_BIKE.contains(highway)
                        || way.hasTag("railway", "platform")
+                        || (way.hasTag("vehicle", "no") && highway != null && !highway.equals("cycleway"))


Suggested change

|| (way.hasTag("vehicle", "no") && highway != null && !highway.equals("cycleway"))

|| !"cycleway".equals(highway) && way.hasTag("vehicle", "no")

I'll change that into your suggestion

karussell · 2024-05-21T15:18:12Z

core/src/main/java/com/graphhopper/routing/util/parsers/BikeCommonAccessParser.java

+                        return WayAccess.WAY;
+                    if (firstRestrictedKey.equals("bicycle") && way.hasTag("vehicle", "no")) {
+                        if (way.hasTag("highway", "cycleway"))
+                            return WayAccess.WAY;  // Tagging error, we follow the highway value


Shall we really consider (rare?) tagging mistakes like highway=cycleway && bicycle=no? Or are they very frequent?

Shall we really consider (rare?) tagging mistakes like highway=cycleway && bicycle=no? Or are they very frequent?

I checked with overpass in Europe and except for Belgium where they seem to have ways for Mopeds tagged as such I didn't find a lot. Therefore I'll remove this special treatment.

karussell · 2024-05-21T15:18:25Z

core/src/main/java/com/graphhopper/routing/util/parsers/BikeCommonAccessParser.java

+                if (value.equals("no")) {
+                    if (firstRestrictedKey.equals("vehicle"))
+                        return WayAccess.WAY;
+                    if (firstRestrictedKey.equals("bicycle") && way.hasTag("vehicle", "no")) {


When is way.hasTag("vehicle", "no") true? Because I think we always return earlier for firstRestrictedKey==vehicle && value==no?

Removed now

ratrun · 2024-05-24T13:03:06Z

Instead of the current code in BikeCommonAccessParser: could we not just remove the vehicle key from the restrictionKeys? Because without it this would make the code more complex and this also leads to the following inconsistency: for vehicle=forestry the access is forbidden, but for vehicle=no it is allowed.

I guess that here you mean a removal of the vehicle key in the toOSMRestrictions method of OSMRoadAccessParser.java. I do not get why this would make BikeCommonAccessParser more complex, can you please explain it a little bit more? But the more important good question is if we should consequently also allow ways tagged as vehicle=forestry as EV get_off_bike sections. First I didn't want to change the blocking here because of the impact it might have on routes in forest/agriculture areas, people actually not obeying the law, and the lack of the OSM website to indicate that there are get_off_bike sections contained. But you convinced me that we also should change that.

Fix for graphhopper#2980, block highway=cycleway with access=no

906b364

ratrun mentioned this pull request Apr 21, 2024

highway=cycleway + access=no + bicycle=NULL is used for routing #2980

Open

ratrun added 4 commits April 21, 2024 17:27

Reduce bicycle speed to PUSHING_SECTION_SPEED for the tag vehicle=no

e63a10a

Reduce bicycle speed to PUSHING_SECTION_SPEED for the tag vehicle=no

982092c

Merge remote-tracking branch 'ratrun/issue2980' into issue2980

31c252c

# Conflicts: # core/src/test/java/com/graphhopper/routing/util/parsers/BikeTagParserTest.java # core/src/test/java/com/graphhopper/routing/util/parsers/OSMGetOffBikeParserTest.java

Merge branch 'refs/heads/master' into issue2980

763c45a

Incorporate review comment

81ef8ea

karussell reviewed Apr 21, 2024

View reviewed changes

Allowing combination vehicle=no without bicycle=no even though not su…

5ead1a5

…re if this is a good idea

Set getOffBikeEnc for vehicle=no

91352fc

Cosmetic change

befc6d4

Add test

be90372

Merge branch 'refs/heads/master' into issue2980

3ba603d

karussell reviewed May 21, 2024

View reviewed changes

ratrun added 2 commits May 24, 2024 15:39

Changes triggered by further review comments

7515d65

Remove outdated comment

db4dd8a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #2980, block highway=cycleway with access=no #2981

Fix for #2980, block highway=cycleway with access=no #2981

ratrun commented Apr 21, 2024 •

edited

karussell commented Apr 21, 2024

ratrun commented Apr 21, 2024

ratrun commented Apr 21, 2024

zidel commented Apr 21, 2024

karussell Apr 21, 2024 •

edited

ratrun Apr 22, 2024

ratrun commented Apr 21, 2024

ratrun commented Apr 22, 2024 •

edited

otbutz commented Apr 22, 2024

ratrun commented Apr 22, 2024

karussell commented Apr 22, 2024

ratrun commented Apr 23, 2024

otbutz commented Apr 23, 2024

ratrun commented Apr 24, 2024

otbutz commented Apr 24, 2024

ratrun commented Apr 27, 2024

otbutz commented Apr 29, 2024

karussell commented May 13, 2024 •

edited

otbutz commented May 14, 2024

ratrun commented May 16, 2024

karussell commented May 16, 2024 •

edited

karussell left a comment •

edited

karussell May 21, 2024

ratrun May 24, 2024

karussell May 21, 2024

ratrun May 24, 2024

karussell May 21, 2024

ratrun May 24, 2024

ratrun commented May 24, 2024

	\|\| (way.hasTag("vehicle", "no") && highway != null && !highway.equals("cycleway"))
	\|\| !"cycleway".equals(highway) && way.hasTag("vehicle", "no")

Fix for #2980, block highway=cycleway with access=no #2981

Are you sure you want to change the base?

Fix for #2980, block highway=cycleway with access=no #2981

Conversation

ratrun commented Apr 21, 2024 • edited

karussell commented Apr 21, 2024

ratrun commented Apr 21, 2024

ratrun commented Apr 21, 2024

zidel commented Apr 21, 2024

karussell Apr 21, 2024 • edited

Choose a reason for hiding this comment

ratrun Apr 22, 2024

Choose a reason for hiding this comment

ratrun commented Apr 21, 2024

ratrun commented Apr 22, 2024 • edited

otbutz commented Apr 22, 2024

ratrun commented Apr 22, 2024

karussell commented Apr 22, 2024

ratrun commented Apr 23, 2024

otbutz commented Apr 23, 2024

ratrun commented Apr 24, 2024

otbutz commented Apr 24, 2024

ratrun commented Apr 27, 2024

otbutz commented Apr 29, 2024

karussell commented May 13, 2024 • edited

otbutz commented May 14, 2024

ratrun commented May 16, 2024

karussell commented May 16, 2024 • edited

karussell left a comment • edited

Choose a reason for hiding this comment

karussell May 21, 2024

Choose a reason for hiding this comment

ratrun May 24, 2024

Choose a reason for hiding this comment

karussell May 21, 2024

Choose a reason for hiding this comment

ratrun May 24, 2024

Choose a reason for hiding this comment

karussell May 21, 2024

Choose a reason for hiding this comment

ratrun May 24, 2024

Choose a reason for hiding this comment

ratrun commented May 24, 2024

ratrun commented Apr 21, 2024 •

edited

karussell Apr 21, 2024 •

edited

ratrun commented Apr 22, 2024 •

edited

karussell commented May 13, 2024 •

edited

karussell commented May 16, 2024 •

edited

karussell left a comment •

edited