Add use_tunnels option #4318

ADMUKHE · 2023-10-03T21:27:04Z

Add Tunnel factor to Autocost

Presently valhalla cannot avoid roads if tunnel is present. These changes will add tunnel factor in Edge Cost Calculations.

The program was tested solely for our own use cases, which might differ otherwise. I acknowledge that these changes have the potential to impact other use cases, and it is essential to exercise caution and thorough testing when merging them to the master. Your expert opinion is requested to help us review, test, and adapt these modifications as needed. I am also looking forward to other implementation ideas and discussions about how we can tackle this issue. Aditionally i am trying to learn more about valhalla and need your comments to improve my coding

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.

Adrika Mukherjee adrika.mukherjee@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH
Provider Information

johannes-no · 2023-10-04T13:25:50Z

Since this is a soft avoidance of tunnels, there should be a boolean set like has_tunnel in the route summary indicating whether the route uses tunnels.

* more sustainable way to work with protobuf in cmake * let's piggyback on CMAKE_FIND_PACKAGE_PREFER_CONFIG

nilsnolde

IMO it only makes sense to expose a use_tunnel option. PR looks generally fine, just LOTS of unrelated adverse changes in the test.

Also

I'd personally like to see a gurka test as well for the costing option, parameterized for all modes that use this. Ideally we'd not create an additional test file, but rather consolidate the other test_use_* into one single file for the sake of compilation speed (i.e. test_use_lit.cc, test_use_living_streets.cc, test_use_tracks.cc)
an additional edge and check in gurka/test_use_feature.cc
we should do this for all motorized vehicles I think? for ped (and bike?) we had a discussion before around safety issues, I think that'd need to be differently handled than the rest

nilsnolde · 2023-10-19T00:09:22Z

test/parse_request.cc

@@ -333,7 +334,7 @@ std::string get_request_str(const std::string& parent_key,
  return R"({")" + parent_key + R"(":{)" + get_kv_str(key, expected_value) + R"(}})";
 }

-std::string get_kv_str(const std::string& key, const std::vector<std::string>& values) {
+std::string get_kv_str(const std::string& key) {


why this change?

nilsnolde · 2023-10-19T00:09:34Z

test/parse_request.cc

@@ -457,8 +458,7 @@ void test_filter_action_parsing(const valhalla::FilterAction expected_value,
  validate(key, expected_value, true, request.options().filter_action());
 }

-void test_filter_attributes_parsing(const std::vector<std::string>& expected_values,
-                                    const Options::Action action = Options::trace_attributes) {
+void test_filter_attributes_parsing(const Options::Action action = Options::trace_attributes) {


and why this change?

nilsnolde · 2023-10-19T00:10:10Z

test/parse_request.cc

@@ -1537,7 +1554,6 @@ void test_bicycle_parsing(const Costing::Type costing_type,

 void test_filter_stop_parsing(const Costing::Type costing_type,
                              const valhalla::FilterAction filter_action,
-                              const std::vector<std::string>& filter_ids,


nilsnolde · 2023-10-19T00:10:16Z

test/parse_request.cc

@@ -1555,7 +1571,6 @@ void test_filter_stop_parsing(const Costing::Type costing_type,

 void test_filter_route_parsing(const Costing::Type costing_type,
                               const valhalla::FilterAction filter_action,
-                               const std::vector<std::string>& filter_ids,


nilsnolde · 2023-10-19T00:10:21Z

test/parse_request.cc

@@ -1573,7 +1588,6 @@ void test_filter_route_parsing(const Costing::Type costing_type,

 void test_filter_operator_parsing(const Costing::Type costing_type,
                                  const valhalla::FilterAction filter_action,
-                                  const std::vector<std::string>& filter_ids,


nilsnolde · 2023-10-19T00:10:44Z

valhalla/proto_conversions.h

@@ -566,4 +566,4 @@ inline void from_ll(valhalla::Location* l, const midgard::PointLL& p) {
  l->mutable_ll()->set_lng(p.lng());
 }

-} // namespace valhalla
+} // namespace valhalla


nilsnolde · 2023-10-19T00:35:53Z

src/sif/autocost.cc

@@ -399,6 +402,14 @@ AutoCost::AutoCost(const Costing& costing, uint32_t access_mask)
  toll_factor_ = use_tolls < 0.5f ? (4.0f - 8 * use_tolls) : // ranges from 4 to 0
                     (0.5f - use_tolls) * 0.03f;             // ranges from 0 to -0.15

+  // Preference to use tunnel roads. Sets a tunnel
+  // factor. A tunnel factor of 0 would indicate no adjustment to weighting for tunnel roads.


Suggested change

// factor. A tunnel factor of 0 would indicate no adjustment to weighting for tunnel roads.

// factor. A tunnel factor of 0.5 would indicate no adjustment to weighting for tunnel roads.

nilsnolde · 2023-10-19T00:51:02Z

I'd recommend you go through the Github view of the diff in https://github.com/valhalla/valhalla/pull/4318/files after you commit (ideally in your own IDE before you even push to not needlessly run CI with stuff that'll break for sure).

nilsnolde · 2023-10-19T00:56:10Z

Bonus points for adding comments on your own PR to explain non-obvious changes, which saves tons of time for the reviewers. (e.g. here I personally would have noted that the costing change is a 100% copy of use_tolls)

ADMUKHE · 2023-10-19T11:48:11Z

@nilsnolde for ped and bike then would you suggest to have a seperate PR?

nilsnolde · 2023-10-19T11:58:07Z

If you’re interested in the functionality, I’d prefer a separate PR bcs the discussion would be very different and we’d first open an issue for that. But don’t feel obliged to take care of that too. It’s fine to not expose for those modes for the time being.

kevinkreiser · 2023-10-24T14:39:55Z

there is a slight technical detail here that i thought someone mentioned but i dont see it mentioned here:
https://github.com/valhalla/valhalla/blob/3806832a11fe250e08f6f21c9f55da0e052bfc80/src/mjolnir/shortcutbuilder.cc#L69C1-L71C28

we create shortcuts overtop of bridges and tunnels which means the costing will not avoid a tunnel if its part of a shortcut that the algorithm is using. given that tunnels arent as prevalant as bridges we could probably get away with adding tunnel as an attribute that has to match during shortcut building. bridges could be a pretty big problem though..

kevinkreiser · 2023-10-24T14:40:42Z

i also agree with @nilsnolde that we should consider just adding use_tunnels to all motorized vehicles. we could move the parsing and factor to the base costing class and just use it from the children that make sense.

the rest of the pr looks great!

nilsnolde · 2023-11-15T08:29:35Z

Just as a note: the only thing missing is a gurka test to verify the behavior of the costing option. Then it’s good to merge.

ADMUKHE · 2023-12-04T14:32:30Z

@nilsnolde is it not covered in the test_use_feature test? can you give any reference to any other gurka test specific to costing option in case you are talking about something else?

nilsnolde · 2023-12-04T14:44:01Z

No, that is only testing that it's flagged in the response, it's not testing that the costing is actually doing anything with it. I already described it: #4318 (review). If you have specific questions, feel free to ask.

nilsnolde · 2023-12-04T17:56:42Z

Oh, and an important note: if we use tunnels to break up shortcuts, we'll need to do a thorough perf test and see if it's fine. Ideally that's done by the committer, but it's poorly documented atm. I always wanted to document that a bit better. We might want to add some Switzerland routes for this:)

added use_tunnels option

38ee0b2

ADMUKHE closed this Oct 3, 2023

ADMUKHE reopened this Oct 3, 2023

Add Test for Parser

0e39469

ADMUKHE marked this pull request as ready for review October 3, 2023 22:46

ADMUKHE and others added 10 commits October 17, 2023 09:47

Merge branch 'master' into add_tunnel_option

f270fca

Merge branch 'master' into add_tunnel_option

caa290d

added has_tunnel to summary

4d2593d

more sustainable way to work with protobuf in cmake (valhalla#4334)

a70e31a

* more sustainable way to work with protobuf in cmake * let's piggyback on CMAKE_FIND_PACKAGE_PREFER_CONFIG

update docs generation (valhalla#4343)

00d00b3

added has_tunnel to summary

74c6dbd

added changelog

3765f8b

Merge remote-tracking branch 'origin/master' into add_tunnel_option

18cf9cf

run pre-commit and checks

602ec78

reverted changes for proto conversion

4d4e572

nilsnolde requested changes Oct 19, 2023

View reviewed changes

adrika.mukherjee added 2 commits October 19, 2023 01:29

removed changes from test

886a129

checkout proto_conversion from master

517c3a2

Merge branch 'valhalla:master' into add_tunnel_option

66659c2

adrika.mukherjee added 2 commits October 31, 2023 14:46

added use_tunnels to motorized vehicles

15f64c5

format script run

5beb6cb

ADMUKHE force-pushed the add_tunnel_option branch from 3573129 to 5beb6cb Compare November 3, 2023 10:53

stop creating shortcut over tunnel

e6c4af9

nilsnolde mentioned this pull request Jan 9, 2024

Feature: exclude bridges/tunnels/tolls #3733

Open

5 tasks

ADMUKHE mentioned this pull request Jan 24, 2024

Hard avoids highway ferry #4524

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add use_tunnels option #4318

Add use_tunnels option #4318

ADMUKHE commented Oct 3, 2023 •

edited

johannes-no commented Oct 4, 2023

nilsnolde left a comment

nilsnolde Oct 19, 2023

nilsnolde Oct 19, 2023

nilsnolde Oct 19, 2023

nilsnolde Oct 19, 2023

nilsnolde Oct 19, 2023

nilsnolde Oct 19, 2023

nilsnolde Oct 19, 2023

nilsnolde commented Oct 19, 2023

nilsnolde commented Oct 19, 2023

ADMUKHE commented Oct 19, 2023

nilsnolde commented Oct 19, 2023

kevinkreiser commented Oct 24, 2023

kevinkreiser commented Oct 24, 2023

nilsnolde commented Nov 15, 2023

ADMUKHE commented Dec 4, 2023

nilsnolde commented Dec 4, 2023

nilsnolde commented Dec 4, 2023

	// factor. A tunnel factor of 0 would indicate no adjustment to weighting for tunnel roads.
	// factor. A tunnel factor of 0.5 would indicate no adjustment to weighting for tunnel roads.

Add use_tunnels option #4318

Are you sure you want to change the base?

Add use_tunnels option #4318

Conversation

ADMUKHE commented Oct 3, 2023 • edited

Add Tunnel factor to Autocost

Tasklist

johannes-no commented Oct 4, 2023

nilsnolde left a comment

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde Oct 19, 2023

Choose a reason for hiding this comment

nilsnolde commented Oct 19, 2023

nilsnolde commented Oct 19, 2023

ADMUKHE commented Oct 19, 2023

nilsnolde commented Oct 19, 2023

kevinkreiser commented Oct 24, 2023

kevinkreiser commented Oct 24, 2023

nilsnolde commented Nov 15, 2023

ADMUKHE commented Dec 4, 2023

nilsnolde commented Dec 4, 2023

nilsnolde commented Dec 4, 2023

ADMUKHE commented Oct 3, 2023 •

edited