some bug fixes ported over from #4638 #4642
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -237,7 +237,7 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, | |
| // 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 = kInvalidRestriction; | ||
| if (FORWARD) { | ||
| // Why is is_dest false? | ||
| // We have to consider next cases: | ||
|
|
@@ -309,7 +309,9 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, | |
| : 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. | ||
| // We allow settling not_thru edges so we can connect both trees on them. | ||
|
There was a problem hiding this comment. 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 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, | |
| 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, 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 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, | |
| 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, not_thru_pruning, | ||
| (pred.closure_pruning() || !costing_->IsClosed(opp_edge, t2)), | ||
|
There was a problem hiding this comment. 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 @@ void BidirectionalAStar::Expand(baldr::GraphReader& graphreader, | |
| // 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 | ||
| bool is_uturn = pred.opp_local_idx() == meta.edge->localedgeidx(); | ||
|
There was a problem hiding this comment. 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 = (!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 @@ void BidirectionalAStar::SetOrigin(GraphReader& graphreader, | |
| dist = astarheuristic_reverse_.GetDistance(nodeinfo->latlng(endtile->header()->base_ll())); | ||
| } | ||
| edgelabels_forward_.emplace_back(kInvalidLabel, edgeid, directededge, cost, sortcost, dist, mode_, | ||
| kInvalidRestriction, !(costing_->IsClosed(directededge, tile)), | ||
| static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
| sif::InternalTurn::kNoTurn, 0, | ||
| directededge->destonly() || | ||
|
|
@@ -1028,9 +1030,9 @@ void BidirectionalAStar::SetOrigin(GraphReader& graphreader, | |
| // 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() || edgelabels_forward_.back().destonly(); | ||
|
There was a problem hiding this comment. 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 @@ void BidirectionalAStar::SetDestination(GraphReader& graphreader, | |
| dist, mode_, c, !opp_dir_edge->not_thru(), | ||
| !(costing_->IsClosed(directededge, tile)), | ||
| static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
| 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. however, in |
||
| adjacencylist_reverse_.add(idx); | ||
|
|
||
| // setting this edge as reached, sending the opposing because this is the reverse tree | ||
|
|
@@ -1124,9 +1126,9 @@ void BidirectionalAStar::SetDestination(GraphReader& graphreader, | |
| // 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() || edgelabels_reverse_.back().destonly(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
ped with destonly penalty sounds weird:)
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.
hm, maybe it has a point since we also turn
privateroads intodestination_onlyand even pedestrian shouldn't take those..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.
it's a bit unfortunate though, "residents only" is pointless for ped but we can't distinguish