Do not skip connections in bidiastar #2802
Conversation
genadz
force-pushed
the
fix_strange_detour
branch
from
5748283
to
3c0f232
Compare
|
When I made changes some tests failed. The reason: we have a bug related to complex restrictions. This bug should be fixed here #2796 . So, this PR is blocked right now |
genadz
force-pushed
the
fix_strange_detour
branch
from
f51b96c
to
260ec53
Compare
genadz
force-pushed
the
fix_strange_detour
branch
from
260ec53
to
c4eaa00
Compare
|
RAD tests passed. It was only one different route, I checked it manually, no worries. |
| @@ -758,7 +758,7 @@ bool BidirectionalAStar::SetForwardConnection(GraphReader& graphreader, const BD | |||
|
|
|||
| // Set a threshold to extend search | |||
| if (threshold_ == std::numeric_limits<float>::max()) { | |||
| threshold_ = (pred.sortcost() + cost_diff_) + kThresholdDelta; | |||
| threshold_ = std::max(pred.sortcost() + cost_diff_, opp_pred.sortcost()) + kThresholdDelta; | |||
There was a problem hiding this comment.
as far as we settle edge right after we pop it from the queue, pred.sortcost() + cost_diff may be lower than opp_pred.sortcost(). so, we should choose bigger in order no to prune search too early
genadz
force-pushed
the
fix_strange_detour
branch
from
ee93788
to
a9d6331
Compare
| A-1--B--------C--------D---------H---------------------------------------I | ||
| | | | ||
| K---------------------------------------J | ||
| )"; |
There was a problem hiding this comment.
special testcase that reproduces bug in master
| A---1-------B-------------C------D------------------------E | ||
| | | | ||
| K--------------------L | ||
| )"; |
There was a problem hiding this comment.
special testcase that reproduces bug in master
| test::customize_edges(map.config, [¬_thru_edgeids](const GraphId& edgeid, DirectedEdge& edge) { | ||
| if (std::find(not_thru_edgeids.begin(), not_thru_edgeids.end(), edgeid) != not_thru_edgeids.end()) | ||
| edge.set_not_thru(true); | ||
| }); |
There was a problem hiding this comment.
its too bad this didnt happen automatically in the graphenhancer. i suppose you had to do this as a workaround?
There was a problem hiding this comment.
right. I needed not_thru attributes to reproduce the bug
|
@genadz im sorry i have to ask it, hows the performance difference 😄 |
the same as |
|
@kevinkreiser thanks for review! |
Issue
This PR should fix 2 bugs:
1) for some cases bidirastar builds extra detours.
After debugging:
in bidirectional astar: we mark edge as
permanentonly before call expansion method. But it's not correct, because we can mark edge earlier right after we popped this edge from the queue.Current behavior may loose some connections in the following case: let's suppose
A->B->C->Dis the part of the shortest route. On some iteration we poppedB->Clabel asfwd_predfrom the forward queue. Then we applied reverse search from the nodeC(suppose cost forD->Cless than cost forB->C). Then we poppedC->Blabel asrev_predfrom the reverse queue. And, looks like now we should set the connectionB->C, but we don't do it because the edgeB->Chasn't marked aspermanentfor now. So, we skip this and continue forward expansion from the nodeC.So, now imagine that we have 2 not-thru regions
AandBand one connection edge between these regions. In some cases if we skip this connection path cannot be found or may be found with extra detour.2) For some cases bidirastar returns pathes with double u-turns.
It happens due to the following: let's suppose we have a complex restriction
no_u_turn: from AB via BC to CD. Then, forward search reachesBC, checks that it's a deadend (because of the restriction) and makes a u-turnCB. After that reverse search reachesCB, checks that it's a deadend (because of the restriction), makes a u-turnBCand meetsCBedge from forward search. So, now we have the following path:AB -> BC -> CB -> BC -> CD- there is no rule to forbid such path so we accept it.Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?