Allow expansion into a region when not_thru_pruning_ is false on 2nd pass #2978

gknisely · 2021-04-01T18:02:20Z

Issue

This PR resolves an edge case where we may encounter only_restrictions with edges being marked as not_thru. Basically the only way to get in this area is via this edge based on the restriction, but it is also marked as not_thru. To resolve this issue, we set the not_thru to false for all edges only if the global not_thru_pruning_ is false. not_thru_pruning_ is only set to false on the 2nd pass in route_action. See the gurka test not_thru_pruning_ as it is based on the following real world example:

This way/edge is marked as a not_thru edge.

But the only way to get into this area is this way/edge due to the only restriction.

So basically in the forward or reverse expansion in bidirectional A* we stop expanding due to the edge being marked as not_thru.
I tried removing logic to not mark this edge as not_thru in Mjolnir; however, the following edge is also marked as not_thru so we would just uturn on the original edge and still not traverse into this area.

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

Requirements / Relations

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

…we trasition to is marked not_thru, set not_thru to false for the edgelabel.

scripts/valhalla_build_config

mandeepsandhu · 2021-04-01T19:57:03Z

src/thor/bidirectional_astar.cc

+  // to false here only if allow_restricted_thru is true and we are not restricted.
+  // This bool is only set to true on the 2nd pass in route_action. See the gurka
+  // test allow_restricted_thru
+  bool thru = (pred.not_thru_pruning() || !meta.edge->not_thru());


minor nit: I think a var name of not_thru_pruning would be more appropriate since it reflects the not_thru pruning state of the current edge label (i.e whether we're culling-out not_thru edges or not)

I would call this not_thru_pruning and not confuse the issue with the restriction name.

mandeepsandhu · 2021-04-01T20:00:07Z

src/thor/timedep_reverse.cc

@@ -37,6 +37,11 @@ void TimeDepReverse::Clear() {
  }
  edgelabels_rev_.clear();
  adjacencylist_rev_.clear();
+
+  // Set the ferry flag to false
+  has_ferry_ = false;


I assume this isn't related to the not_thru issue? the has_ferry flag was simply missing from this implementation?

@mandeepsandhu correct.

cc @danpat note the missing has_ferry

mandeepsandhu · 2021-04-01T20:47:16Z

src/thor/bidirectional_astar.cc

+  // based on the restriction, but it is also marked as not_thru. We set the not_thru
+  // to false here only if allow_restricted_thru is true.  This bool is only set to true
+  // on the 2nd pass in route_action.  See the gurka test allow_restricted_thru
+  bool thru = (allow_restricted_thru() ? false : (pred.not_thru_pruning() || !meta.edge->not_thru()));


Why is the condition being checked different than the forward direction? Here we're switching off pruning without checking if the predecessor edge has restriction or not (i.e using pred.restrictions()).

@mandeepsandhu unfortunately we can't. I will explain when I fill in the details above.

I would simply have a not_thru_pruning option that is true on the first pass and false on the 2nd pass. No need (in my opinion) to check for restrictions - just disable all not_thru pruning on the 2nd pass. This was mostly added as a performance optimization for cycling and pedestrian (where hierarchies are not useful).

mandeepsandhu · 2021-04-01T20:48:33Z

test/gurka/test_bidir_search.cc

-        }
-      },
-      std::exception);
+  auto result = gurka::do_action(valhalla::Options::route, map, {"A", "F"}, "auto", {});


hmmm, why is not failing to find a route anymore with your change? This is a simple not_thru case and the previous open edge does not have any restriction on it. I thought this case would still throw a no-route exception since there's no other restriction involved.

@mandeepsandhu in the reverse search not_thru is not considered anymore on the 2nd pass

…on the second pass

dnesbitt61 · 2021-04-07T12:17:04Z

src/thor/bidirectional_astar.cc

@@ -324,11 +326,17 @@ inline bool BidirectionalAStar::ExpandForwardInner(GraphReader& graphreader,
  float sortcost =
      newcost.cost + astarheuristic_forward_.Get(t2->get_node_ll(meta.edge->endnode()), dist);

+  // There is an edge case where we may encounter only_restrictions with edges being
+  // marked as not_thru.  Basically the only way to get in this area is via this edge


confusing description. The edge that is restricted is not marked as not_thru. I guess the "only_restriction" means the path can only go onto the not_thru edge, but Valhalla doesn't really have the notion of "only" - it just has edges that are restricted / not allowed.

dnesbitt61 · 2021-04-07T12:19:23Z

src/thor/multimodal.cc

@@ -92,6 +92,8 @@ void MultiModalPathAlgorithm::Clear() {

  // Set the ferry flag to false
  has_ferry_ = false;
+  // Set not thru pruning to true
+  not_thru_pruning_ = true;


aren't multi-modal routes always forward search (not bidirectional) - and we don't use not_thru pruning except in bidirectional A*?

dnesbitt61 · 2021-04-07T12:19:40Z

src/thor/timedep_forward.cc

@@ -43,6 +43,8 @@ void TimeDepForward::Clear() {

  // Set the ferry flag to false
  has_ferry_ = false;
+  // Set not thru pruning to true
+  not_thru_pruning_ = true;


same comment as multi-modal

dnesbitt61 · 2021-04-07T12:20:52Z

valhalla/thor/pathalgorithm.h

+  /**
+   * Set the not_thru_pruning_
+   * @param pruning  set the not_thru_pruning_ to pruning value.
+   *                 only set on the second pass


this is set to true on first pass for bidirectional, so this comment line is incorrect

I would consider moving the comments on not_thru pruning from bidirectional_astar.cc into here.

dnesbitt61 · 2021-04-07T12:22:30Z

valhalla/thor/pathalgorithm.h

@@ -106,6 +124,8 @@ class PathAlgorithm {

  bool has_ferry_; // Indicates whether the path has a ferry

+  bool not_thru_pruning_; // Indicates whether to allow access into a restricted no thru region.


take out the word restricted, change to "into a not-thru region"

dnesbitt61 · 2021-04-07T12:23:58Z

src/thor/bidirectional_astar.cc

@@ -549,11 +557,17 @@ inline bool BidirectionalAStar::ExpandReverseInner(GraphReader& graphreader,
  float sortcost =
      newcost.cost + astarheuristic_reverse_.Get(t2->get_node_ll(meta.edge->endnode()), dist);

+  // There is an edge case where we may encounter only_restrictions with edges being


consider moving this lengthy comment into the PathAlgorithm method that sets not_thru pruning.

dnesbitt61 · 2021-04-07T12:25:15Z

src/thor/astar_bss.cc

@@ -57,6 +57,8 @@ void AStarBSSAlgorithm::Clear() {

  // Set the ferry flag to false
  has_ferry_ = false;
+  // Set not thru pruning to true
+  not_thru_pruning_ = true;


I thought only bidirectional A* uses not_thru_pruning?

…alhalla into allow_restricted_thru

gknisely added 8 commits March 30, 2021 09:56

added allow_restricted_thru

bd5dbfb

set allow_restricted_thru to true for the second pass

6df2d0b

if allow_restricted_thru is true, we hit a restriction, and the edge …

9e91534

…we trasition to is marked not_thru, set not_thru to false for the edgelabel.

missing comma

a2db824

moved allow_restricted_thru to pathalgorithm

48e0150

allow_restricted_thru_ is always false for this algorithm

09725c0

set allow_restricted_thru to true for the second pass

4bff81a

added logic for allow_restricted_thru

08757a7

gknisely changed the title ~~Allow restricted thru~~ [WIP] Allow restricted thru Apr 1, 2021

gknisely and others added 2 commits April 1, 2021 14:13

Update bidirectional_astar.cc

35a7e84

lint

180687c

mandeepsandhu reviewed Apr 1, 2021

View reviewed changes

scripts/valhalla_build_config Outdated Show resolved Hide resolved

mandeepsandhu reviewed Apr 1, 2021

View reviewed changes

gknisely and others added 3 commits April 1, 2021 16:29

added allow_restricted_thru test.

5079e93

updated due to allow_restricted_thru logic.

8ee6571

Merge branch 'master' into allow_restricted_thru

6358496

mandeepsandhu reviewed Apr 1, 2021

View reviewed changes

gknisely and others added 9 commits April 6, 2021 08:31

Merge branch 'master' into allow_restricted_thru

64c2c11

if not_thru_pruning_=false then ignore not_thrus

479b957

changed to not_thru_pruning_ and defaults to true by default.

da1363c

the route should not fail due to settting not_thru_pruning_ to false …

cb2b2e3

…on the second pass

renamed.

9c225a2

renamed.

826bcae

Update pathalgorithm.h

d7d6f06

Update bidirectional_astar.cc

4a61b26

Merge branch 'master' into allow_restricted_thru

7f9b364

gknisely changed the title ~~[WIP] Allow restricted thru~~ Allow expansion into a region when not_thru_pruning_ is false on 2nd pass Apr 7, 2021

updated

96e72af

gknisely marked this pull request as ready for review April 7, 2021 02:30

dnesbitt61 reviewed Apr 7, 2021

View reviewed changes

gknisely and others added 4 commits April 7, 2021 08:51

Merge branch 'master' into allow_restricted_thru

89e4205

not used here

7d0976a

moved comments to pathalgorithm.h

72b98ea

Merge branch 'allow_restricted_thru' of https://github.com/valhalla/v…

4899638

…alhalla into allow_restricted_thru

dnesbitt61 previously approved these changes Apr 7, 2021

View reviewed changes

Merge branch 'master' into allow_restricted_thru

5a4b240

gknisely dismissed dnesbitt61’s stale review via 5a4b240 April 7, 2021 15:54

dnesbitt61 previously approved these changes Apr 7, 2021

View reviewed changes

Merge branch 'master' into allow_restricted_thru

a0af4cc

gknisely dismissed dnesbitt61’s stale review via a0af4cc April 7, 2021 16:39

dnesbitt61 approved these changes Apr 7, 2021

View reviewed changes

gknisely merged commit 5936d06 into master Apr 7, 2021

dnesbitt61 mentioned this pull request Apr 16, 2021

Performance Drop in Long Bicycle Route #3007

Closed

gknisely deleted the allow_restricted_thru branch February 22, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow expansion into a region when not_thru_pruning_ is false on 2nd pass #2978

Allow expansion into a region when not_thru_pruning_ is false on 2nd pass #2978

gknisely commented Apr 1, 2021 •

edited

Loading

mandeepsandhu Apr 1, 2021

dnesbitt61 Apr 1, 2021

mandeepsandhu Apr 1, 2021

gknisely Apr 1, 2021

gknisely Apr 1, 2021

mandeepsandhu Apr 1, 2021

gknisely Apr 1, 2021

dnesbitt61 Apr 1, 2021

mandeepsandhu Apr 1, 2021

gknisely Apr 1, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

dnesbitt61 Apr 7, 2021

		@@ -106,6 +124,8 @@ class PathAlgorithm {

		bool has_ferry_; // Indicates whether the path has a ferry

		bool not_thru_pruning_; // Indicates whether to allow access into a restricted no thru region.

Allow expansion into a region when not_thru_pruning_ is false on 2nd pass #2978

Allow expansion into a region when not_thru_pruning_ is false on 2nd pass #2978

Conversation

gknisely commented Apr 1, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

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

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

gknisely commented Apr 1, 2021 •

edited

Loading