Hard avoids highway ferry #4524

ADMUKHE · 2024-01-24T09:39:59Z

This PR is an extension of PR #3733 . In addition to the exclude_bridges, exclude_tunnels ,exclude_tolls options, this PR introduces exclude_ferries, exclude_highways. This new options works similar to exclude_tolls/tunnels/bridges.

We noticed that in PR #3733 , the PR was not merged since some perf tests are required. We want to help with the perf tests so any hint on where to start with the perf tests would be great!

The default behaviour of valhalla remains same as the PR #3733 . It returns 400 and by default does not allow the use of hard excludes unless otherwise set using the service_limits.status.allow_hard_exclusions parameter.

The reason for the seperate PR is we are not sure if IGNF is using their branch for other purposes. Nevertheless we added their changes as well since we would anyway need to add the service_limits.status.allow_hard_exclusions parameter.

Also we did not add the changes in the #4318 since that deals with the soft avoid options which is also different from the issue this PR wants to resolve.

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

#3733

…ls/tolls to avoid confusion with searchFilter exclude_bridge/tunnel parameter

…ion name

…r.cc

…hods

nilsnolde

not sure how I feel about all these hard exclusion properties tbh. most will seriously screw up tons of routes. there's also a question how shortcuts are handled and if it's even doing the right thing in the current state. tests to prove would be good.

nilsnolde · 2024-01-25T20:22:57Z

src/sif/autocost.cc

@@ -425,7 +425,18 @@ bool AutoCost::Allowed(const baldr::DirectedEdge* edge,
      edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) ||
      (!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
      (pred.closure_pruning() && IsClosed(edge, tile)) ||
-      (exclude_unpaved_ && !pred.unpaved() && edge->unpaved()) || !IsHOVAllowed(edge)) {
+      (exclude_unpaved_ && !pred.unpaved() && edge->unpaved()) || !IsHOVAllowed(edge) ||


can we please:

initialize a has_excludes or so at the beginning of each cost model which is true if any of them is set? this is just way too many checks right now for every edge which are basically never triggered on average

make this a function in DynamicCost, it's used in every cost model, avoids a lot of duplication

what about shortcuts btw?

@nilsnolde for shortcuts we do check if edge is shortcut and exclude_bridge/tunnel is true then we do not use it. As per your discussion in #3733 you decided on this change! Can you please specify exactly what could be the issue with shortcuts if we exclude edges part of shortcut?

#3733 (comment) here its mentioned that we can disable shortcut use

the initial PR is 1.5 years old, I have a hard time remembering what I did last week;) that's the problem with opening new PRs based on old ones, I won't go back and read across several PRs. that's why it's important for the person who raises a PR to be as informative as needed for reviewers to have an easy-ish time.

now that you pointed it out I squinted again at the code and yeah I see the last line in each cost model update handles shortcuts in a way that should be fine (I think).

that's kinda a case in point of above though: those changes are so repetitive it's very easy to miss these small important updates in a review and it'd be very nice to have it commented in the PR so those misunderstandings don't happen. keep in mind we/I do this on a voluntary basis and the easier you make the reviewer's lives the likelier they'll actually put in the unpaid time to do the review. just as a recent example, e.g. in #4516 one can see how reviewers will have an easier time when the PR person is flagging non-trivial changes explicitly and explaining a little bit right where the changes happen (not in the PR description).

nilsnolde · 2024-01-25T20:24:24Z

src/loki/worker.cc

+        (pair.second.options().exclude_bridges() || pair.second.options().exclude_tolls() ||
+         pair.second.options().exclude_tunnels() || pair.second.options().exclude_highways() ||
+         pair.second.options().exclude_ferries())) {
+      throw valhalla_exception_t{145};


make this a warning please, not an exception. there's nothing a user can do about it anyways, so he's forced to send the same request again, might as well let it pass right away instead of throwing.

nilsnolde · 2024-01-25T20:24:45Z

proto/options.proto

@@ -312,6 +312,21 @@ message Costing {
    uint32 axle_count = 81;
    float use_lit = 82;
    bool disable_hierarchy_pruning = 83;
+    oneof has_exclude_bridges {


why oneof?isn't this defaulting to false`?

nilsnolde · 2024-01-25T20:29:16Z

test/gurka/test_hard_exclusions.cc

+                                                          "truck",         "pedestrian", "bicycle",
+                                                          "motor_scooter", "motorcycle"};
+
+const std::vector<std::string> kExclusionParameters = {"exclude_bridges", "exclude_tunnels",


what about ferry and highway/trunk?

nilsnolde · 2024-01-25T20:35:23Z

CHANGELOG.md

@@ -134,6 +134,12 @@
   * ADDED: "has_transit_tiles" & "osm_changeset" to verbose status response [#4062](https://github.com/valhalla/valhalla/pull/4062)
   * ADDED: time awareness to CostMatrix for e.g. traffic support [#4071](https://github.com/valhalla/valhalla/pull/4071)
   * UPDATED: transifex translations [#4102](https://github.com/valhalla/valhalla/pull/4102)
+   * ADDED: Added `exclude_tolls` request parameter [#3733](https://github.com/valhalla/valhalla/pull/3733)


one entry is enough

nilsnolde · 2024-01-25T20:45:15Z

We noticed that in PR #3733, the PR was not merged since some perf tests are required. We want to help with the perf tests so any hint on where to start with the perf tests would be great!

run a valhalla service with at least europe
run this with time: https://github.com/valhalla/valhalla/blob/master/run_route_scripts/run_with_server.py with e.g. this file: https://github.com/valhalla/valhalla/blob/master/test_requests/de_benchmark_routes.txt which has a lot of routes of varying lengths
do 2. multiple times while 1. is running for both master and your PR; the first run will be meaningless as the service isn't initiated

best do it on a bare minimum linux server with as little background stuff running as possible, to not pollute the pseudo benchmarks.

azarz and others added 27 commits December 12, 2023 14:08

Add exclude_toll, exclude_bridge and exclude_tunnel options

9ca7f76

Add exclude toll bridge tunnel to pedestrian

ba8b62c

Add exclude bridge tunnel toll parameters to documentation

0bc0b75

Rename exclude_bridge/tunnel/toll in costing to exclude_bridges/tunne…

602ac18

…ls/tolls to avoid confusion with searchFilter exclude_bridge/tunnel parameter

Rename test_requests/use_unpaved_roads_option.txt to match actual opt…

31a1d5f

…ion name

add tests exclude_bridges/tunnels/tolls

0a63d28

Correct problems in test_exclude_bridges/tunnels/tolls files

a10c237

Add back pedestrian to exlude tests

8f1d680

Update exlude options doc: mention that if true, no path may be found

a38c15c

Add PR number in CHANGELOG.md

a2802dc

Add tests for unallowed hard exclusions

74992c7

Add server conf parameter to allow hard exclusions

086c43b

Check if hard exclusions are allowed before applying them

fdd288e

Update changelog with new server config parameter

3b0a6af

Add hard exclusions to all costing models, update doc, fix loki/worke…

9d8830a

…r.cc

No shortcut use when exlude bridge or exclude tunnel on

7afe46a

Fix shortcut check for exlude tunnel and bridge in AllowedReverse met…

ffaaf59

…hods

More concise testing for hard exclusions

018efc7

Even more concise testing of hard exclusions

01fb698

add hard avoids

6ce2920

added hard avoids for highway and ferry

766f9cc

modifies changelog

bdbfcdb

Merge branch 'valhalla:master' into hard-avoids-highway-ferry

ab50f5e

modified option name

af0bb4e

modified change log

a48a5e2

ran pre-commits and clang tidy

52e74cd

modified changelog

fbbfeac

nilsnolde requested changes Jan 25, 2024

View reviewed changes

Merge branch 'master' into hard-avoids-highway-ferry

4a55af9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hard avoids highway ferry #4524

Hard avoids highway ferry #4524

ADMUKHE commented Jan 24, 2024 •

edited

nilsnolde left a comment

nilsnolde Jan 25, 2024

nilsnolde Jan 25, 2024

ADMUKHE Jan 26, 2024

ADMUKHE Jan 26, 2024

nilsnolde Jan 26, 2024

nilsnolde Jan 25, 2024

nilsnolde Jan 25, 2024

nilsnolde Jan 25, 2024

nilsnolde Jan 25, 2024

nilsnolde commented Jan 25, 2024

Hard avoids highway ferry #4524

Are you sure you want to change the base?

Hard avoids highway ferry #4524

Conversation

ADMUKHE commented Jan 24, 2024 • edited

Tasklist

Requirements / Relations

nilsnolde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde commented Jan 25, 2024

ADMUKHE commented Jan 24, 2024 •

edited