Unpaved roads #3240

maksimandrianov · 2021-07-28T13:21:31Z

Issue

Added the new option exclude_unpaved. This value indicates the willingness to take unpaved roads. If exclude_unpaved is set to 1, unpaved roads are excluded from the process of generating the route path, otherwise included.

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog
If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

maksimandrianov · 2021-08-04T13:33:29Z

The light demo of using the exclude_unpaved option:

Include unpaved	Exclude unpaved	OSM
		Unpaved OSM way
		Unpaved OSM way
		Unpaved OSM way

kevinkreiser · 2021-08-04T13:53:35Z

@maksimandrianov can you please fill out the description of the pr just for book-keeping purposes. just a brief description of the changes. also dont forget to add it to the costing documentation in /docs

maksimandrianov · 2021-08-04T14:26:14Z

There are two implementations of the exclusion unpaved roads from a route in this PR:

Do not allow the use of unpaved roads in the beginning and end of a route. (The commit - b88ba5e)
Allow the use of unpaved roads in the beginning and end of a route. (This branch)

Examples:

Do not allow	Allow	OSM
No route found		Unpaved OSM way Unpaved OSM way Unpaved OSM way
No route found		Unpaved OSM way

merkispavel · 2021-08-05T14:23:54Z

There are two implementations of the exclusion unpaved roads from a route in this PR:
-Do not allow the use of unpaved roads in the beginning and end of a route. (The commit - b88ba5e)
-Allow the use of unpaved roads in the beginning and end of a route. (This branch)

origin and destination points are usually set intentionally so if one of them gets snapped to the unpaved road - why shouldn't we use it. The second (up-to-date) implementation definitely makes sense to me 👍

Though there's a small pitfall. In this case we don't say "get out of the unpaved road asap" so we can make completely-unpaved route in theory. It doesn't seem like a blocker for this PR imho, just wanted to share

gknisely · 2021-08-05T15:26:16Z

@kevinkreiser @dnesbitt61 don't we usually have a range(0 to 1) for use_* options? Not sure if the naming (use_unpaved_roads) is correct here. Thoughts? cc @maksimandrianov

kevinkreiser · 2021-08-05T16:49:57Z

@gknisely yes exactly, use_* options are in the range of 0-1, meaning they are not hard avoids but a soft avoid on a finite scale.

if you want to exclude certain types of roads from the start or end of the route, as in its not allowed to use those as candidates, then you should look into how loki does excludes. that is done in a different part of the request using the search filter. here is where it gets applied: https://github.com/valhalla/valhalla/blob/master/src/loki/search.cc#L38-L42

gknisely · 2021-08-05T16:53:20Z

@gknisely yes exactly, use_* options are in the range of 0-1, meaning they are not hard avoids but a soft avoid on a finite scale.

if you want to exclude certain types of roads from the start or end of the route, as in its not allowed to use those as candidates, then you should look into how loki does excludes. that is done in a different part of the request using the search filter. here is where it gets applied: https://github.com/valhalla/valhalla/blob/master/src/loki/search.cc#L38-L42

@maksimandrianov I think that the above is a better implementation. Thoughts ^

kevinkreiser · 2021-08-06T17:33:01Z

I agree that 0 should not be a hard exclusion of unpaved roads in the path but that they should be severely penalized. in the cases where unpaved roads are the only way between two locations we need to allow for that and this is the strategy that the project has followed since we started allowing tweaking costing in this way.

we can do hard exclusion on the search candidates as i pointed out above but have yet to add hard exclusion in the path finding phase. not that we couldnt do that, but we need to do a bit of a design process there to make sure it works for all cases where we would want to do that. id argue that is not so useful and not so common that a person will tolerate absolutely no unpaved roads or no highways or not tolls for example. if that is the only way to go, they'd rather have A route than none or than one that makes them drive 2 hours out of their way

genadz · 2021-08-10T07:54:07Z

@merkispavel

Though there's a small pitfall. In this case we don't say "get out of the unpaved road asap" so we can make completely-unpaved route in theory. It doesn't seem like a blocker for this PR imho, just wanted to share

To avoid that we can penalize unpaved roads more than ordinary roads.

genadz · 2021-08-10T08:16:02Z

I agree that 0 should not be a hard exclusion of unpaved roads in the path but that they should be severely penalized. in the cases where unpaved roads are the only way between two locations we need to allow for that and this is the strategy that the project has followed since we started allowing tweaking costing in this way.

we can do hard exclusion on the search candidates as i pointed out above but have yet to add hard exclusion in the path finding phase. not that we couldnt do that, but we need to do a bit of a design process there to make sure it works for all cases where we would want to do that. id argue that is not so useful and not so common that a person will tolerate absolutely no unpaved roads or no highways or not tolls for example. if that is the only way to go, they'd rather have A route than none or than one that makes them drive 2 hours out of their way

@kevinkreiser I think in this case hard exclusion is more preferable and clear way. There are several reasons for that I see:

It's expected and completely clear behavior to return a route without unpaved roads in case I set exclude_unpaved=true. I will be very surprise if I get a route with unpaved edges. From my perspective it's worse to get a route with unpaved roads in case there is a route without unpaved roads rather than return "No route" in case the only way to go is to use unpaved edges.
Choosing correct penalty it's very difficult task. This value should be aligned with other many constants that we already use for other options. Moreover, it should be aligned with other options that we will introduce in the future. So, we can't guarantee for 100% that avoid_unpaved will work correctly with all combinations of options that we have for now (like use_tracks, use_tolls and other that have big penalties).
I do believe that there are very few cases when unpaved road is the only way to go from the origin to the destination. As for me it's okay to return "No route", a user takes a responsibility for setting parameters. But, if we really want to cover such cases and return a route, we can do that on the second pass of the path finding algorithm.

P.S. if we treated all use_*=0 parameters as hard exclusion and handle "No route" cases only on the second pass we could improve performance for such requests (due to lower costs).

maksimandrianov · 2021-08-10T10:47:37Z

@gknisely yes exactly, use_* options are in the range of 0-1, meaning they are not hard avoids but a soft avoid on a finite scale.

if you want to exclude certain types of roads from the start or end of the route, as in its not allowed to use those as candidates, then you should look into how loki does excludes. that is done in a different part of the request using the search filter. here is where it gets applied: https://github.com/valhalla/valhalla/blob/master/src/loki/search.cc#L38-L42

But we want to see the hard exclusion here. I cannot imagine how the level of avoidance of unpaved roads can help drivers.
I have used the dest_only example, when implemented this.

maksimandrianov · 2021-08-10T10:51:22Z

@kevinkreiser @dnesbitt61 don't we usually have a range(0 to 1) for use_* options? Not sure if the naming (use_unpaved_roads) is correct here. Thoughts? cc @maksimandrianov

If it is a problem I can rename this variable. exclude_unpaved_roads? Or do you have any suggestion of a better readable name?

maksimandrianov · 2021-08-10T14:55:36Z

3. we could change all use_ options to work like this but that would be a breaking change. existing requests would start hard excluding stuff if set to 0

Unfortunately, if use_* set to 0, it does not mean hard exclusion - https://github.com/valhalla/valhalla/blame/master/docs/api/turn-by-turn/api-reference.md#L105

I will fix naming, thank you for the detailed explanation!

genadz · 2021-08-10T14:56:28Z

i dont think its ok to have different semantics for options of the same type. all use_ options should work the same

completely agree, we shouldn't put different meanings for two use_* options

kevinkreiser · 2021-08-10T15:02:52Z

@maksimandrianov @genadz yeah i think if we change the name to something else, then we could also migrate the other use_ options to match that naming and depricate as i mentioned in the last part of my previous comment

so now the real question is, what is a better name than use_

kevinkreiser · 2021-08-10T15:06:21Z

src/sif/autocost.cc

@@ -412,7 +412,8 @@ bool AutoCost::Allowed(const baldr::DirectedEdge* edge,
      ((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_restrictions_) ||
      edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) ||
      (!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
-      (pred.closure_pruning() && IsClosed(edge, tile))) {
+      (pred.closure_pruning() && IsClosed(edge, tile)) ||
+      (!use_unpaved_roads_ && !pred.unpaved() && edge->unpaved())) {


i like this trick of using a bit to specify whether the predecessor was or wasnt unpaved instead of keeping track of the pruning state. i think its easier to reason about than setting and unsetting the pruning. we should switch to doing this for closures. cc @mandeepsandhu

dnesbitt61 · 2021-08-10T16:44:29Z

The bicycle costing option avoid_bad_surfaces does a hard exclusion when set to a value of 1. Not sure if I like the naming of this option, but it does mix penalties and exclusion.

kevinkreiser · 2021-08-10T22:54:13Z

The bicycle costing option avoid_bad_surfaces does a hard exclusion when set to a value of 1.

@dnesbitt61 i had the same thought, use the term avoid_, reverse the range (since it has the opposite meaning) and then deprecate all of the use_ options. and i had the same thought that i didnt love the naming but i have a feeling there is no naming that i would love 😄

maksimandrianov · 2021-08-11T13:10:49Z

The bicycle costing option avoid_bad_surfaces does a hard exclusion when set to a value of 1.

@dnesbitt61 i had the same thought, use the term avoid_, reverse the range (since it has the opposite meaning) and then deprecate all of the use_ options. and i had the same thought that i didnt love the naming but i have a feeling there is no naming that i would love 😄

May be do we leave use_* option naming as it is? I agree with @kevinkreiser, because avoid and use are similar in clarity/readability terms.

maksimandrianov · 2021-08-11T13:17:35Z

@kevinkreiser, @genadz the naming was fixed, PTAL

docs/api/turn-by-turn/api-reference.md

src/mjolnir/pbfgraphparser.cc

kevinkreiser

other than the change to the docs wording, this looks good to me. i would like @gknisely 's opinion on the tagging values though

kevinkreiser · 2021-08-11T13:51:16Z

@maksimandrianov please add an entry to the changelog. a new request parameter certainly deserves an entry in the changelog

genadz · 2021-08-11T14:41:51Z

docs/api/turn-by-turn/api-reference.md

@@ -122,6 +122,7 @@ The following options are available for `auto`, `bus`, `hov`, `taxi`, and `truck
 | :-------------------------- | :----------- |
 | `height` | The height of the vehicle (in meters). |
 | `width` | The width of the vehicle (in meters). |
+| `exclude_unpaved` | This value indicates the whether or not the path may include unpaved roads. If `exclude_unpaved` is set to 1, unpaved roads are not allowed to be used in the process of generating the route path, otherwise they are allowed. |


@maksimandrianov should we mention here that it's allowed to start and end with unpaved roads, but it's not allowed to have them in the middle ?

I agree. @maksimandrianov can you make the change log more descriptive too?

Yes, I made

genadz · 2021-08-12T11:33:24Z

docs/api/turn-by-turn/api-reference.md

@@ -122,6 +122,7 @@ The following options are available for `auto`, `bus`, `hov`, `taxi`, and `truck
 | :-------------------------- | :----------- |
 | `height` | The height of the vehicle (in meters). |
 | `width` | The width of the vehicle (in meters). |
+| `exclude_unpaved` | This value indicates the whether or not the path may include unpaved roads. If `exclude_unpaved` is set to 1, unpaved roads are not allowed to be used in the process of generating the route path, otherwise they are allowed. If `exclude_unpaved` is set to 1 it is allowed to start and end with unpaved roads, but is not allowed to have them in the middle of the route path. |


there is some contradiction between second and third sentences. @maksimandrianov Could you rewrite it ?

maksimandrianov force-pushed the andrianov_962_1 branch 4 times, most recently from c514e10 to 8a28ebb Compare August 4, 2021 11:12

maksimandrianov added 10 commits August 4, 2021 16:08

Supported surface=chipseal.

eb6fd50

Supported surface=metal.

a102f2c

Added comment for concrete processing.

2731c4e

Supported surface=stepping_stones.

44adcfa

Added comment for gravel processing.

99bd61e

Supported surface=grass_paver.

974a6e3

Supported the costing_options/<>/use_unpaved_roads option.

b88ba5e

Added gurka tests.

e65d461

Allowed for destination only.

7923fbf

Added test requests.

dba9d7b

maksimandrianov force-pushed the andrianov_962_1 branch from 8a28ebb to dba9d7b Compare August 4, 2021 13:09

maksimandrianov added 2 commits August 4, 2021 17:37

Updated docs.

17bc96a

Fixed test after surface=grass_paver tag supporting.

670d90b

maksimandrianov marked this pull request as ready for review August 5, 2021 13:49

kevinkreiser reviewed Aug 10, 2021

View reviewed changes

Review fixes

1f01c13

kevinkreiser reviewed Aug 11, 2021

View reviewed changes

docs/api/turn-by-turn/api-reference.md Outdated Show resolved Hide resolved

kevinkreiser reviewed Aug 11, 2021

View reviewed changes

src/mjolnir/pbfgraphparser.cc Show resolved Hide resolved

kevinkreiser previously approved these changes Aug 11, 2021

View reviewed changes

maksimandrianov dismissed kevinkreiser’s stale review via 3de0474 August 11, 2021 14:29

genadz reviewed Aug 11, 2021

View reviewed changes

Updated changelog.

fa64f2b

maksimandrianov force-pushed the andrianov_962_1 branch from 3de0474 to 056fb1e Compare August 12, 2021 10:15

Clarified docs.

efc744c

maksimandrianov force-pushed the andrianov_962_1 branch from 056fb1e to efc744c Compare August 12, 2021 10:16

maksimandrianov added 2 commits August 12, 2021 13:18

Merge branch 'master' into andrianov_962_1

1276709

Fixed docs.

51ef99d

genadz reviewed Aug 12, 2021

View reviewed changes

maksimandrianov added 2 commits August 12, 2021 15:26

Fixed docs.

9847f44

Merge branch 'master' into andrianov_962_1

9d15aaf

gknisely approved these changes Aug 12, 2021

View reviewed changes

genadz approved these changes Aug 12, 2021

View reviewed changes

maksimandrianov merged commit a5ea654 into master Aug 12, 2021

azarz mentioned this pull request Apr 1, 2022

New exclude_tolls, exclude_briges and exclude_tunnels costing options #3587

Open

azarz mentioned this pull request Sep 2, 2022

Feature: exclude bridges/tunnels/tolls #3733

Open

5 tasks

nilsnolde deleted the andrianov_962_1 branch February 24, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unpaved roads #3240

Unpaved roads #3240

maksimandrianov commented Jul 28, 2021 •

edited

Loading

maksimandrianov commented Aug 4, 2021 •

edited

Loading

kevinkreiser commented Aug 4, 2021

maksimandrianov commented Aug 4, 2021 •

edited

Loading

merkispavel commented Aug 5, 2021 •

edited

Loading

gknisely commented Aug 5, 2021

kevinkreiser commented Aug 5, 2021

gknisely commented Aug 5, 2021

kevinkreiser commented Aug 6, 2021

genadz commented Aug 10, 2021

genadz commented Aug 10, 2021 •

edited

Loading

maksimandrianov commented Aug 10, 2021

maksimandrianov commented Aug 10, 2021

maksimandrianov commented Aug 10, 2021 •

edited

Loading

genadz commented Aug 10, 2021

kevinkreiser commented Aug 10, 2021 •

edited

Loading

kevinkreiser Aug 10, 2021

dnesbitt61 commented Aug 10, 2021

kevinkreiser commented Aug 10, 2021 •

edited

Loading

maksimandrianov commented Aug 11, 2021

maksimandrianov commented Aug 11, 2021 •

edited

Loading

kevinkreiser left a comment

kevinkreiser commented Aug 11, 2021

genadz Aug 11, 2021

gknisely Aug 11, 2021

maksimandrianov Aug 12, 2021

genadz Aug 12, 2021

maksimandrianov Aug 12, 2021

Unpaved roads #3240

Unpaved roads #3240

Conversation

maksimandrianov commented Jul 28, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

maksimandrianov commented Aug 4, 2021 • edited Loading

kevinkreiser commented Aug 4, 2021

maksimandrianov commented Aug 4, 2021 • edited Loading

merkispavel commented Aug 5, 2021 • edited Loading

gknisely commented Aug 5, 2021

kevinkreiser commented Aug 5, 2021

gknisely commented Aug 5, 2021

kevinkreiser commented Aug 6, 2021

genadz commented Aug 10, 2021

genadz commented Aug 10, 2021 • edited Loading

maksimandrianov commented Aug 10, 2021

maksimandrianov commented Aug 10, 2021

maksimandrianov commented Aug 10, 2021 • edited Loading

genadz commented Aug 10, 2021

kevinkreiser commented Aug 10, 2021 • edited Loading

kevinkreiser Aug 10, 2021

Choose a reason for hiding this comment

dnesbitt61 commented Aug 10, 2021

kevinkreiser commented Aug 10, 2021 • edited Loading

maksimandrianov commented Aug 11, 2021

maksimandrianov commented Aug 11, 2021 • edited Loading

kevinkreiser left a comment

Choose a reason for hiding this comment

kevinkreiser commented Aug 11, 2021

genadz Aug 11, 2021

Choose a reason for hiding this comment

gknisely Aug 11, 2021

Choose a reason for hiding this comment

maksimandrianov Aug 12, 2021

Choose a reason for hiding this comment

genadz Aug 12, 2021

Choose a reason for hiding this comment

maksimandrianov Aug 12, 2021

Choose a reason for hiding this comment

maksimandrianov commented Jul 28, 2021 •

edited

Loading

maksimandrianov commented Aug 4, 2021 •

edited

Loading

maksimandrianov commented Aug 4, 2021 •

edited

Loading

merkispavel commented Aug 5, 2021 •

edited

Loading

genadz commented Aug 10, 2021 •

edited

Loading

maksimandrianov commented Aug 10, 2021 •

edited

Loading

kevinkreiser commented Aug 10, 2021 •

edited

Loading

kevinkreiser commented Aug 10, 2021 •

edited

Loading

maksimandrianov commented Aug 11, 2021 •

edited

Loading