fixed connection check for costmatrix when the connection edge is on correlated edge #4663
Conversation
…a correlated edge of origin OR target
| // Find pred on opposite side | ||
| GraphId oppedge = pred.opp_edgeid(); | ||
| EdgeStatusInfo oppedgestatus = edgestatus_reverse_.Get(oppedge); | ||
| auto opp_pred = edgelabels_reverse_[oppedgestatus.index()]; | ||
|
|
There was a problem hiding this comment.
the diff is strange.. I moved the block above that, not this one.. we should early-return on
if (pred.internal_turn() != InternalTurn::kNoTurn) {
return false;
}
| // TODO(nils): this can't be happen, it'd mean that this is a trivial route | ||
| // , which is not supposed to be here! same in SetReverseConnection | ||
| float oppcost = | ||
| (opp_predidx == kInvalidLabel) ? 0.f : edgelabels_reverse_[opp_predidx].cost().cost; |
There was a problem hiding this comment.
this check shouldn't be necessary, it'd mean we have a trivial route in bidir a*
| @@ -896,29 +904,36 @@ void CostMatrix::CheckReverseConnections(const uint32_t target, | |||
| uint32_t d = std::abs(static_cast<int>(rev_pred.path_distance()) + | |||
| static_cast<int>(fwd_label.path_distance()) - | |||
| static_cast<int>(fwd_label.transition_cost().secs)); | |||
| best_connection_[source_idx].Update(fwd_edgeid, rev_pred.edgeid(), Cost(s, s), d); | |||
| best_connection_[source_idx].found = true; | |||
| best_connection_[idx].Update(fwd_edgeid, rev_pred.edgeid(), Cost(s, s), d); | |||
There was a problem hiding this comment.
this is mostly changing source_idx name to idx; it's the connection's index, not the source index
| // Update status and update threshold if this is the last location | ||
| // to find for this source or target | ||
| UpdateStatus(source, target); | ||
| } else if (fwd_predidx == kInvalidLabel) { |
There was a problem hiding this comment.
this is the actual bug fix. before we'd go directly into the else below in case it wasn't a trivial route. but that block of code looks at the opposing's transition_cost, which is not what we'd expect if the opposing is a correlated edge: in SetTargets we use transition_cost to store the full edge's length and the full duration for it.
so this needs to be handled in a different way to get the connection's length & duration
| auto opp_pred_label = fwd_edgelabels[fwd_predidx]; | ||
| auto cost = rev_pred.cost() + opp_pred_label.cost() + fwd_label.transition_cost(); |
There was a problem hiding this comment.
the else I just refactored to be terser
|
I was preparing for the bidir A* for costmatrix and reviewing the costmatrix code for the zillionth time. each time I find smth else! I doubt this was the last time 😅 I guess I should add a test for this.. |
|
I can't reproduce this behavior in gurka. I can see that it finds a connection which uses that another PR to put on hold. in theory it's a problem but until it's not proven I won't spend time on it. |
in costmatrix we already had a special check for trivial routes when checking a connecting edge, but not for the case where the connecting edge is the correlated edge on one tree, but not the other. statistically that should happen more often than real trivial routes (i.e. one edge routes). in that case we were just returning kinda bogus.