"soft" ignore restrictions #4606

chrstnbwnkl · 2024-02-24T09:35:03Z

Issue

Follow-up to #4574, fixes #4571. ~~ Open to better ideas for a name. I'll list more as well if I come up with any. @nilsnolde's first take was ignore_safe_restrictions. ~~

Edit: sticking with ignore_non_vehicular_restrictions now.

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

nilsnolde

Not sure about another name either (other than ignore_safe_restrictions which is also not very good), possibly only ignore_turn_conditional_restrictions, bit long but describes best what it does.

There's a few more things to do, ideally they'd be tested with a parameterized test somewhere.

Also I know these kind of options weren't documented before, but we really should start documenting them. Would you be up for that? If you feel like including the other ignore_*'s as well, that'd be much appreciated. There's a collection of undocumented options here: #3760. One after the other:)

valhalla/sif/dynamiccost.h

src/sif/truckcost.cc

…s, etc.

chrstnbwnkl · 2024-02-26T14:43:38Z

I think ignore_turn_conditional_restrictions does not convey that we are ignoring e.g. maxaxles, so in that regard ignore_safe_restrictions actually describes it better IMO 😄

Anyway just ignore everything around naming in this PR for now, it's a bit all over the place. I'll add some more tests that test some more mode specific cases where ignore_<whatever>_restrictions shouldn't lead to a successful route.

test/gurka/test_truck_restrictions.cc

nilsnolde

apart from naming: there's still costings missing.. ped not necessary, but why not the rest?

test/gurka/test_ignore_restrictions.cc

nilsnolde

looks good to me now!

then we can focus on the naming of it. I don't have a very strong opinion, though the only option I don't really like tbh is the current ignore_common_restrictions.

valhalla/sif/dynamiccost.h

nilsnolde · 2024-03-04T10:03:36Z

valhalla/sif/dynamiccost.h

@@ -605,7 +608,7 @@ class DynamicCost {
        if (access_type == baldr::AccessType::kTimedAllowed)
          time_allowed = true;

-        if (current_time == 0) {
+        if (current_time == 0 || ignore_non_vehicular_restrictions_) {


I'd put this into the if on line 602, so time_allowed will stay false if this option is given and we don't need it in the return below.

nilsnolde · 2024-03-04T10:05:44Z

src/sif/dynamiccost.cc

-      gate_cost_{0.f, kDefaultGateCost, kMaxPenalty}, gate_penalty_{0.f, kDefaultGatePenalty,
-                                                                    kMaxPenalty},
+      gate_cost_{0.f, kDefaultGateCost, kMaxPenalty},
+      gate_penalty_{0.f, kDefaultGatePenalty, kMaxPenalty},


it doesn't seem like anything changed here. is it possible that you installed clang-format also from pacman rather than mason? looks like a typical "new version" edit

nilsnolde · 2024-03-04T13:40:40Z

proto/options.proto

+    oneof has_ignore_non_vehicular_restrictions {
+      bool ignore_non_vehicular_restrictions = 84;
+    }


nilsnolde · 2024-03-04T15:46:43Z

I edited the things we talked about @kevinkreiser , should be good now

nilsnolde · 2024-03-04T15:49:40Z

oops that broke smth

…more/most of them, should refactor some), similar to the ranged equivalent

nilsnolde · 2024-03-04T15:59:24Z

valhalla/sif/dynamiccost.h

+#define JSON_PBF_DEFAULT_V2(costing_options, def, json, json_key, option_name)                       \
+  {                                                                                                  \
+    costing_options->set_##option_name(                                                              \
+        rapidjson::get<std::remove_cv<                                                               \
+            std::remove_reference<decltype(def)>::type>::type>(json, json_key,                       \
+                                                               costing_options->option_name()        \
+                                                                   ? costing_options->option_name()  \
+                                                                   : def));                          \
+  }


added one for non-oneof things. most boolean options have false as default, we should refactor some and remove the oneofs where it can be done.

nilsnolde · 2024-03-04T23:16:32Z

so the normal release build works, but the arm & x86 lint-debug build fail. that's weird.. they're failing with

2024/03/04 16:17:01.373591 [INFO] Parsing nodes...
terminate called after throwing an instance of 'std::runtime_error'
  what():  test_linguistic_node.bin(open): No such file or directory
make[3]: *** [test/CMakeFiles/run-graphparser.dir/build.make:73: test/graphparser.log] Error 134

does that ring a bell @gknisely? seems related to #4605?

the failing test also seems to be pretty unrelated to this PRs changes, so it seems flaky.

gknisely · 2024-03-06T15:02:38Z

so the normal release build works, but the arm & x86 lint-debug build fail. that's weird.. they're failing with
2024/03/04 16:17:01.373591 [INFO] Parsing nodes...
terminate called after throwing an instance of 'std::runtime_error'
  what():  test_linguistic_node.bin(open): No such file or directory
make[3]: *** [test/CMakeFiles/run-graphparser.dir/build.make:73: test/graphparser.log] Error 134
does that ring a bell @gknisely? seems related to #4605?

the failing test also seems to be pretty unrelated to this PRs changes, so it seems flaky.

Should just be creating a temporary file. Odd that this is failing. Are you sure unrelated?

nilsnolde · 2024-03-06T17:02:05Z

Are you sure unrelated?

@gknisely at least this PR doesn't touch any of the graphbuilder logic or the test that fails. Just costings and some of those tests. So I'd say yeah, I'm very sure. Also the release build works. It's "just" the debug and arm build that fail that way. Locally it also doesn't fail.

nilsnolde · 2024-03-06T19:09:53Z

hm yeah, now it's the exact opposite! the release build is failing with the same problem, debug & arm builds are fine.. had a quick look at the test, but can't see anything wrong. the only way I'd see this failing would be if gtest would execute tests in parallel which I don't think it does.

nilsnolde · 2024-03-07T11:36:48Z

since the flaky test also fails on master's arm workflow currently, this PR should be good to merge.

nilsnolde · 2024-03-13T09:11:25Z

we previously looked at this @kevinkreiser and there were just a few things needed to be done before merging (done here), like remove the oneof and formatting the code properly. I also needed to add another "V2" macro for non-oneofs: #4606 (comment).

nilsnolde · 2024-03-13T21:25:49Z

once it passes the master merge, I'd merge this too. it's been reviewed and amended.

chrstnbwnkl added 5 commits February 22, 2024 18:31

added param, simple test

c81081c

time deny test case

260e1f8

forgot maxaxle

076f07b

Merge branch 'master' into cb-ignore-common-restrictions

e94197a

changelog

fc9bcc8

chrstnbwnkl marked this pull request as ready for review February 24, 2024 09:35

format

ad940fc

nilsnolde requested changes Feb 24, 2024

View reviewed changes

valhalla/sif/dynamiccost.h Outdated Show resolved Hide resolved

src/sif/truckcost.cc Outdated Show resolved Hide resolved

chrstnbwnkl and others added 2 commits February 26, 2024 15:33

comments: add parameterized test, make available to more costing mode…

ca7504e

…s, etc.

Merge branch 'master' into cb-ignore-common-restrictions

33f2442

more rigorous testing

0c71b90

chrstnbwnkl commented Feb 27, 2024

View reviewed changes

test/gurka/test_truck_restrictions.cc Show resolved Hide resolved

nilsnolde requested changes Feb 28, 2024

View reviewed changes

test/gurka/test_ignore_restrictions.cc Outdated Show resolved Hide resolved

added remaining costing modes, adapted test

d80085f

nilsnolde reviewed Feb 28, 2024

View reviewed changes

chrstnbwnkl added 2 commits February 29, 2024 15:08

renaming

b6cb707

changelog

b32d9c0

nilsnolde reviewed Mar 4, 2024

View reviewed changes

valhalla/sif/dynamiccost.h Show resolved Hide resolved

nilsnolde reviewed Mar 4, 2024

View reviewed changes

Update api-reference.md

7dbcdf4

nilsnolde reviewed Mar 4, 2024

View reviewed changes

nilsnolde added 2 commits March 4, 2024 16:39

do the last bits

21b727d

Merge branch 'master' into cb-ignore-common-restrictions

0e717b3

nilsnolde requested a review from kevinkreiser March 4, 2024 15:46

nilsnolde previously approved these changes Mar 4, 2024

View reviewed changes

nilsnolde dismissed their stale review via 2ce6b8b March 4, 2024 15:58

also have a JSON_PBF_DEFAULT_V2 for non-oneof options (actually lots …

2ce6b8b

…more/most of them, should refactor some), similar to the ranged equivalent

nilsnolde reviewed Mar 4, 2024

View reviewed changes

add boost format.hpp header to a test

25c441f

gknisely mentioned this pull request Mar 11, 2024

Conflict with signinfo's temporary linguistic node sequence file caused test failures. #4625

Merged

5 tasks

Merge branch 'master' into cb-ignore-common-restrictions

a53b91f

nilsnolde self-requested a review March 13, 2024 18:08

nilsnolde previously approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'master' into cb-ignore-common-restrictions

b113cda

nilsnolde dismissed their stale review via b113cda March 13, 2024 18:09

Merge branch 'master' into cb-ignore-common-restrictions

c66910f

nilsnolde approved these changes Mar 13, 2024

View reviewed changes

nilsnolde merged commit 28eaf7a into valhalla:master Mar 13, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"soft" ignore restrictions #4606

"soft" ignore restrictions #4606

chrstnbwnkl commented Feb 24, 2024 •

edited

nilsnolde left a comment •

edited

chrstnbwnkl commented Feb 26, 2024

nilsnolde left a comment

nilsnolde left a comment

nilsnolde Mar 4, 2024

nilsnolde Mar 4, 2024

nilsnolde Mar 4, 2024

nilsnolde commented Mar 4, 2024

nilsnolde commented Mar 4, 2024

nilsnolde Mar 4, 2024

nilsnolde commented Mar 4, 2024 •

edited

gknisely commented Mar 6, 2024

nilsnolde commented Mar 6, 2024

nilsnolde commented Mar 6, 2024

nilsnolde commented Mar 7, 2024

nilsnolde commented Mar 13, 2024

nilsnolde commented Mar 13, 2024

"soft" ignore restrictions #4606

"soft" ignore restrictions #4606

Conversation

chrstnbwnkl commented Feb 24, 2024 • edited

Issue

Tasklist

nilsnolde left a comment • edited

Choose a reason for hiding this comment

chrstnbwnkl commented Feb 26, 2024

nilsnolde left a comment

Choose a reason for hiding this comment

nilsnolde left a comment

Choose a reason for hiding this comment

nilsnolde Mar 4, 2024

Choose a reason for hiding this comment

nilsnolde Mar 4, 2024

Choose a reason for hiding this comment

nilsnolde Mar 4, 2024

Choose a reason for hiding this comment

nilsnolde commented Mar 4, 2024

nilsnolde commented Mar 4, 2024

nilsnolde Mar 4, 2024

Choose a reason for hiding this comment

nilsnolde commented Mar 4, 2024 • edited

gknisely commented Mar 6, 2024

nilsnolde commented Mar 6, 2024

nilsnolde commented Mar 6, 2024

nilsnolde commented Mar 7, 2024

nilsnolde commented Mar 13, 2024

nilsnolde commented Mar 13, 2024

chrstnbwnkl commented Feb 24, 2024 •

edited

nilsnolde left a comment •

edited

nilsnolde commented Mar 4, 2024 •

edited