New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
some bug fixes ported over from #4638 #4642
Changes from all commits
274400a
039fa9d
f20fdd5
c8a557c
8387b1b
910c1ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,7 +221,7 @@ void AStarBSSAlgorithm::ExpandForward(GraphReader& graphreader, | |
// Add to the adjacency list and edge labels. | ||
uint32_t idx = edgelabels_.size(); | ||
edgelabels_.emplace_back(pred_idx, edgeid, GraphId(), directededge, newcost, sortcost, dist, mode, | ||
transition_cost, false, true, false, InternalTurn::kNoTurn, | ||
transition_cost, false, false, false, InternalTurn::kNoTurn, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also this you'll see a lot: this flag is setting |
||
baldr::kInvalidRestriction); | ||
*current_es = {EdgeSet::kTemporary, idx}; | ||
adjacencylist_.add(idx); | ||
|
@@ -446,7 +446,7 @@ void AStarBSSAlgorithm::SetOrigin(GraphReader& graphreader, | |
// of the path. | ||
uint32_t d = static_cast<uint32_t>(directededge->length() * (1.0f - edge.percent_along())); | ||
BDEdgeLabel edge_label(kInvalidLabel, edgeid, directededge, cost, sortcost, dist, | ||
travel_mode_t::kPedestrian, baldr::kInvalidRestriction, true, false, | ||
travel_mode_t::kPedestrian, baldr::kInvalidRestriction, false, false, | ||
sif::InternalTurn::kNoTurn); | ||
// Set the origin flag and path distance | ||
edge_label.set_origin(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,7 +237,7 @@ | |
// or if a complex restriction prevents transition onto this edge. | ||
// if its not time dependent set to 0 for Allowed and Restricted methods below | ||
const uint64_t localtime = time_info.valid ? time_info.local_time : 0; | ||
uint8_t restriction_idx = -1; | ||
uint8_t restriction_idx = kInvalidRestriction; | ||
if (FORWARD) { | ||
// Why is is_dest false? | ||
// We have to consider next cases: | ||
|
@@ -309,7 +309,9 @@ | |
: astarheuristic_reverse_.Get(t2->get_node_ll(meta.edge->endnode()), dist)); | ||
|
||
// not_thru_pruning_ is only set to false on the 2nd pass in route_action. | ||
bool thru = not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false; | ||
// We allow settling not_thru edges so we can connect both trees on them. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this comment is what eventually wasted a good 2 days of work on #4638 when I discovered that at the very end. very subtle.. |
||
bool not_thru_pruning = | ||
not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false; | ||
|
||
// Add edge label, add to the adjacency list and set edge status | ||
uint32_t idx = 0; | ||
|
@@ -321,7 +323,7 @@ | |
dist = astarheuristic_reverse_.GetDistance(t2->get_node_ll(meta.edge->endnode())); | ||
} | ||
edgelabels_forward_.emplace_back(pred_idx, meta.edge_id, opp_edge_id, meta.edge, newcost, | ||
sortcost, dist, mode_, transition_cost, thru, | ||
sortcost, dist, mode_, transition_cost, not_thru_pruning, | ||
(pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
costing_->TurnType(pred.opp_local_idx(), nodeinfo, meta.edge), | ||
|
@@ -337,8 +339,8 @@ | |
dist = astarheuristic_forward_.GetDistance(t2->get_node_ll(meta.edge->endnode())); | ||
} | ||
edgelabels_reverse_.emplace_back(pred_idx, meta.edge_id, opp_edge_id, meta.edge, newcost, | ||
sortcost, dist, mode_, transition_cost, thru, | ||
(pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), | ||
sortcost, dist, mode_, transition_cost, not_thru_pruning, | ||
(pred.closure_pruning() || !costing_->IsClosed(opp_edge, t2)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 100% sure now that we need to look at the opposing edge here because we're expanding on the reverse tree. case in point: AllowedReverse will prune on the opposing edge as well, not the one that gets expanded. it's really just common sense, though easy to miss.. you'll see that in a lot of |
||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
costing_->TurnType(meta.edge->localedgeidx(), nodeinfo, opp_edge, | ||
opp_pred_edge), | ||
|
@@ -422,14 +424,14 @@ | |
// If so, it means we are attempting a u-turn. In that case, lets wait with evaluating | ||
// this edge until last. If any other edges were emplaced, it means we should not | ||
// even try to evaluate a u-turn since u-turns should only happen for deadends | ||
uturn_meta = pred.opp_local_idx() == meta.edge->localedgeidx() ? meta : uturn_meta; | ||
bool is_uturn = pred.opp_local_idx() == meta.edge->localedgeidx(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just putting this in a variable to make it clearer |
||
uturn_meta = is_uturn ? meta : uturn_meta; | ||
|
||
// Expand but only if this isnt the uturn, we'll try that later if nothing else works out | ||
disable_uturn = | ||
(pred.opp_local_idx() != meta.edge->localedgeidx() && | ||
ExpandInner<expansion_direction>(graphreader, pred, opp_pred_edge, nodeinfo, pred_idx, meta, | ||
shortcuts, tile, offset_time)) || | ||
disable_uturn; | ||
disable_uturn = (!is_uturn && ExpandInner<expansion_direction>(graphreader, pred, opp_pred_edge, | ||
nodeinfo, pred_idx, meta, | ||
shortcuts, tile, offset_time)) || | ||
disable_uturn; | ||
} | ||
|
||
// Handle transitions - expand from the end node of each transition | ||
|
@@ -1011,7 +1013,7 @@ | |
dist = astarheuristic_reverse_.GetDistance(nodeinfo->latlng(endtile->header()->base_ll())); | ||
} | ||
edgelabels_forward_.emplace_back(kInvalidLabel, edgeid, directededge, cost, sortcost, dist, mode_, | ||
-1, !(costing_->IsClosed(directededge, tile)), | ||
kInvalidRestriction, !(costing_->IsClosed(directededge, tile)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
sif::InternalTurn::kNoTurn, 0, | ||
directededge->destonly() || | ||
|
@@ -1028,9 +1030,9 @@ | |
// flags on small loops. Set this to false here to override this for now. | ||
edgelabels_forward_.back().set_not_thru(false); | ||
|
||
pruning_disabled_at_origin_ = pruning_disabled_at_origin_ || | ||
!edgelabels_forward_.back().closure_pruning() || | ||
!edgelabels_forward_.back().not_thru_pruning(); | ||
pruning_disabled_at_origin_ = | ||
pruning_disabled_at_origin_ || !edgelabels_forward_.back().closure_pruning() || | ||
!edgelabels_forward_.back().not_thru_pruning() || edgelabels_forward_.back().destonly(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the destination/origin projects on destonly edge, it's also a reason to extend expansion in the opposite direction. |
||
} | ||
|
||
// Set the origin timezone | ||
|
@@ -1109,9 +1111,9 @@ | |
dist, mode_, c, !opp_dir_edge->not_thru(), | ||
!(costing_->IsClosed(directededge, tile)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
sif::InternalTurn::kNoTurn, -1, | ||
opp_dir_edge->destonly() || | ||
(costing_->is_hgv() && opp_dir_edge->destonly_hgv())); | ||
sif::InternalTurn::kNoTurn, kInvalidRestriction, | ||
directededge->destonly() || | ||
(costing_->is_hgv() && directededge->destonly_hgv())); | ||
Comment on lines
+1115
to
+1116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. however, in |
||
adjacencylist_reverse_.add(idx); | ||
|
||
// setting this edge as reached, sending the opposing because this is the reverse tree | ||
|
@@ -1124,9 +1126,9 @@ | |
// flags on small loops. Set this to false here to override this for now. | ||
edgelabels_reverse_.back().set_not_thru(false); | ||
|
||
pruning_disabled_at_destination_ = pruning_disabled_at_destination_ || | ||
!edgelabels_reverse_.back().closure_pruning() || | ||
!edgelabels_reverse_.back().not_thru_pruning(); | ||
pruning_disabled_at_destination_ = | ||
pruning_disabled_at_destination_ || !edgelabels_reverse_.back().closure_pruning() || | ||
!edgelabels_reverse_.back().not_thru_pruning() || edgelabels_reverse_.back().destonly(); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -461,7 +461,7 @@ bool CostMatrix::ExpandInner(baldr::GraphReader& graphreader, | |
auto& edgelabels = edgelabel_[FORWARD][index]; | ||
// Skip this edge if no access is allowed (based on costing method) | ||
// or if a complex restriction prevents transition onto this edge. | ||
uint8_t restriction_idx = -1; | ||
uint8_t restriction_idx = kInvalidRestriction; | ||
if (FORWARD) { | ||
if (!costing_->Allowed(meta.edge, false, pred, tile, meta.edge_id, time_info.local_time, | ||
time_info.timezone_index, restriction_idx) || | ||
|
@@ -512,16 +512,16 @@ bool CostMatrix::ExpandInner(baldr::GraphReader& graphreader, | |
} | ||
|
||
// not_thru_pruning_ is only set to false on the 2nd pass in matrix_action. | ||
// TODO(nils): one of these cases where I think reverse tree should look at the opposing edge, | ||
// not the expanding one, same for quite some attributes below (and same in bidir a*) | ||
bool thru = not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false; | ||
// We allow settling not_thru edges so we can connect both trees on them. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changes in CostMatrix are basically the exact same as in bidir A* |
||
bool not_thru_pruning = | ||
not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false; | ||
|
||
// Add edge label, add to the adjacency list and set edge status | ||
uint32_t idx = edgelabels.size(); | ||
*meta.edge_status = {EdgeSet::kTemporary, idx}; | ||
if (FORWARD) { | ||
edgelabels.emplace_back(pred_idx, meta.edge_id, opp_edge_id, meta.edge, newcost, mode_, tc, | ||
pred_dist, thru, | ||
pred_dist, not_thru_pruning, | ||
(pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
costing_->TurnType(pred.opp_local_idx(), nodeinfo, meta.edge), | ||
|
@@ -530,8 +530,8 @@ bool CostMatrix::ExpandInner(baldr::GraphReader& graphreader, | |
(costing_->is_hgv() && meta.edge->destonly_hgv())); | ||
} else { | ||
edgelabels.emplace_back(pred_idx, meta.edge_id, opp_edge_id, meta.edge, newcost, mode_, tc, | ||
pred_dist, thru, | ||
(pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), | ||
pred_dist, not_thru_pruning, | ||
(pred.closure_pruning() || !costing_->IsClosed(opp_edge, t2)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
costing_->TurnType(meta.edge->localedgeidx(), nodeinfo, opp_edge, | ||
opp_pred_edge), | ||
|
@@ -1024,9 +1024,10 @@ void CostMatrix::SetSources(GraphReader& graphreader, | |
// - "transition_cost" is used to store the traversed secs & length | ||
// - "path_id" is used to store whether the edge is even allowed (e.g. no oneway) | ||
Cost ec(std::round(edgecost.secs), static_cast<uint32_t>(directededge->length())); | ||
BDEdgeLabel edge_label(kInvalidLabel, edgeid, oppedge, directededge, cost, mode_, ec, d, false, | ||
true, static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
InternalTurn::kNoTurn, -1, | ||
BDEdgeLabel edge_label(kInvalidLabel, edgeid, oppedge, directededge, cost, mode_, ec, d, | ||
!directededge->not_thru(), !(costing_->IsClosed(directededge, tile)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
InternalTurn::kNoTurn, kInvalidRestriction, | ||
static_cast<uint8_t>(costing_->Allowed(directededge, tile)), | ||
directededge->destonly() || | ||
(costing_->is_hgv() && directededge->destonly_hgv())); | ||
|
@@ -1108,11 +1109,12 @@ void CostMatrix::SetTargets(baldr::GraphReader& graphreader, | |
// - "path_id" is used to store whether the opp edge is even allowed (e.g. no oneway) | ||
Cost ec(std::round(edgecost.secs), static_cast<uint32_t>(directededge->length())); | ||
BDEdgeLabel edge_label(kInvalidLabel, opp_edge_id, edgeid, opp_dir_edge, cost, mode_, ec, d, | ||
false, true, static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
InternalTurn::kNoTurn, -1, | ||
!opp_dir_edge->not_thru(), !(costing_->IsClosed(directededge, tile)), | ||
static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
InternalTurn::kNoTurn, kInvalidRestriction, | ||
static_cast<uint8_t>(costing_->Allowed(opp_dir_edge, opp_tile)), | ||
opp_dir_edge->destonly() || | ||
(costing_->is_hgv() && opp_dir_edge->destonly_hgv())); | ||
directededge->destonly() || | ||
(costing_->is_hgv() && directededge->destonly_hgv())); | ||
edge_label.set_not_thru(false); | ||
|
||
// Add EdgeLabel to the adjacency list (but do not set its status). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this you'll see a lot, why not use it if it already exists.