diff --git a/CHANGELOG.md b/CHANGELOG.md index bd6c77b868..a6fa94100b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * FIXED: Missing fork maneuver [#3134](https://github.com/valhalla/valhalla/pull/3134) * FIXED: Update turn channel logic to call out specific turn at the end of the turn channel if needed [#3140](https://github.com/valhalla/valhalla/pull/3140) * FIXED: Fixed cost thresholds for TimeDistanceMatrix. [#3131](https://github.com/valhalla/valhalla/pull/3131) + * FIXED: Use distance threshold in hierarchy limits for bidirectional astar to expand more important lower level roads [#3156](https://github.com/valhalla/valhalla/pull/3156) * **Enhancement** * CHANGED: Refactor base costing options parsing to handle more common stuff in a one place [#3125](https://github.com/valhalla/valhalla/pull/3125) diff --git a/run_route_scripts/run_with_server.py b/run_route_scripts/run_with_server.py index 285777ea30..2e1cd94e58 100755 --- a/run_route_scripts/run_with_server.py +++ b/run_route_scripts/run_with_server.py @@ -85,7 +85,7 @@ def make_request(post_body): parser.add_argument('--output-dir', type=str, help='The directory in which to place the result of each request') parser.add_argument('--concurrency', type=int, help='The number of processes to use to make requests', default=multiprocessing.cpu_count()) parser.add_argument('--format', type=str, help='Supports csv, json, raw and null output formats', default='csv') - parser.add_argument('--headers', type=str, help='Additional http headers to send with the requests. Follows the http header spec, eg. some-header-name: some-header-value', action='append', nargs='*') + parser.add_argument('--headers', type=str, help='Additional http headers to send with the requests. Follows the http header spec, eg. some-header-name: some-header-value', action='append', nargs='*', default=[]) parsed_args = parser.parse_args() # make the output directory diff --git a/src/sif/hierarchylimits.cc b/src/sif/hierarchylimits.cc index ea19f43bb1..5a75445596 100644 --- a/src/sif/hierarchylimits.cc +++ b/src/sif/hierarchylimits.cc @@ -3,5 +3,5 @@ using namespace valhalla::sif; bool HierarchyLimits::StopExpanding(const float dist) const { - return (dist > expansion_within_dist && up_transition_count > max_up_transitions); + return (up_transition_count > max_up_transitions && dist > expansion_within_dist); } diff --git a/src/thor/bidirectional_astar.cc b/src/thor/bidirectional_astar.cc index 5b1bcb09f8..fbff991984 100644 --- a/src/thor/bidirectional_astar.cc +++ b/src/thor/bidirectional_astar.cc @@ -163,7 +163,7 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, // that level. If using a shortcut, set the shortcuts mask. Skip if this is a regular // edge superseded by a shortcut. if (meta.edge->is_shortcut()) { - if (hierarchy_limits[meta.edge_id.level() + 1].StopExpanding()) { + if (hierarchy_limits[meta.edge_id.level() + 1].StopExpanding(pred.distance())) { shortcuts |= meta.edge->shortcut(); } else { return false; @@ -285,6 +285,11 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, uint32_t idx = 0; if (FORWARD) { idx = edgelabels_forward_.size(); + if (hierarchy_limits_forward_[meta.edge_id.level()].max_up_transitions != kUnlimitedTransitions) { + // Override distance to the destination with a distance from the origin. + // It will be used by hierarchy limits + 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, (pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), @@ -294,6 +299,11 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, adjacencylist_forward_.add(idx); } else { idx = edgelabels_reverse_.size(); + if (hierarchy_limits_reverse_[meta.edge_id.level()].max_up_transitions != kUnlimitedTransitions) { + // Override distance to the origin with a distance from the destination. + // It will be used by hierarchy limits + 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)), @@ -390,7 +400,8 @@ bool BidirectionalAStar::Expand(baldr::GraphReader& graphreader, // if this is a downward transition (ups are always allowed) AND we are no longer allowed OR // we cant get the tile at that level (local extracts could have this problem) THEN bail graph_tile_ptr trans_tile = nullptr; - if ((!trans->up() && hierarchy_limits[trans->endnode().level()].StopExpanding()) || + if ((!trans->up() && + hierarchy_limits[trans->endnode().level()].StopExpanding(pred.distance())) || !(trans_tile = graphreader.GetGraphTile(trans->endnode()))) { continue; } @@ -480,6 +491,9 @@ BidirectionalAStar::GetBestPath(valhalla::Location& origin, SetOrigin(graphreader, origin, forward_time_info); SetDestination(graphreader, destination, reverse_time_info); + // Update hierarchy limits + ModifyHierarchyLimits(); + // Find shortest path. Switch between a forward direction and a reverse // direction search based on the current costs. Alternating like this // prevents one tree from expanding much more quickly (if in a sparser @@ -620,7 +634,7 @@ BidirectionalAStar::GetBestPath(valhalla::Location& origin, // Prune path if predecessor is not a through edge or if the maximum // number of upward transitions has been exceeded on this hierarchy level. if ((fwd_pred.not_thru() && fwd_pred.not_thru_pruning()) || - hierarchy_limits_forward_[fwd_pred.endnode().level()].StopExpanding()) { + hierarchy_limits_forward_[fwd_pred.endnode().level()].StopExpanding(fwd_pred.distance())) { continue; } @@ -639,7 +653,7 @@ BidirectionalAStar::GetBestPath(valhalla::Location& origin, // Prune path if predecessor is not a through edge if ((rev_pred.not_thru() && rev_pred.not_thru_pruning()) || - hierarchy_limits_reverse_[rev_pred.endnode().level()].StopExpanding()) { + hierarchy_limits_reverse_[rev_pred.endnode().level()].StopExpanding(rev_pred.distance())) { continue; } @@ -853,6 +867,11 @@ void BidirectionalAStar::SetOrigin(GraphReader& graphreader, // to invalid to indicate the origin of the path. uint32_t idx = edgelabels_forward_.size(); edgestatus_forward_.Set(edgeid, EdgeSet::kTemporary, idx, tile); + if (hierarchy_limits_forward_[edgeid.level()].max_up_transitions != kUnlimitedTransitions) { + // Override distance to the destination with a distance from the origin. + // It will be used by hierarchy limits + 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)), static_cast(flow_sources & kDefaultFlowMask), @@ -940,6 +959,11 @@ void BidirectionalAStar::SetDestination(GraphReader& graphreader, // edge (edgeid) is set. uint32_t idx = edgelabels_reverse_.size(); edgestatus_reverse_.Set(opp_edge_id, EdgeSet::kTemporary, idx, opp_tile); + if (hierarchy_limits_reverse_[opp_edge_id.level()].max_up_transitions != kUnlimitedTransitions) { + // Override distance to the origin with a distance from the destination. + // It will be used by hierarchy limits + dist = astarheuristic_forward_.GetDistance(tile->get_node_ll(opp_dir_edge->endnode())); + } edgelabels_reverse_.emplace_back(kInvalidLabel, opp_edge_id, edgeid, opp_dir_edge, cost, sortcost, dist, mode_, c, !opp_dir_edge->not_thru(), !(costing_->IsClosed(directededge, tile)), @@ -1138,6 +1162,17 @@ std::vector> BidirectionalAStar::FormPath(GraphReader& gra return paths; } +void BidirectionalAStar::ModifyHierarchyLimits() { + // Distance threshold optimized for unidirectional search. For bidirectional case + // they can be lowered. + // Decrease distance thresholds only for arterial roads for now + if (hierarchy_limits_forward_[1].max_up_transitions != kUnlimitedTransitions) + hierarchy_limits_forward_[1].expansion_within_dist /= 5.f; + + if (hierarchy_limits_reverse_[1].max_up_transitions != kUnlimitedTransitions) + hierarchy_limits_reverse_[1].expansion_within_dist /= 5.f; +} + bool IsBridgingEdgeRestricted(GraphReader& graphreader, std::vector& edge_labels_fwd, std::vector& edge_labels_rev, diff --git a/valhalla/sif/edgelabel.h b/valhalla/sif/edgelabel.h index 410f76501d..4f604b6fd5 100644 --- a/valhalla/sif/edgelabel.h +++ b/valhalla/sif/edgelabel.h @@ -187,6 +187,8 @@ class EdgeLabel { /** * Get the distance to the destination. + * In case of bidirectional search it may represent the distance to the start point: + * origin for the forward search and destination for the reverse search. * @return Returns the distance in meters. */ float distance() const { diff --git a/valhalla/sif/hierarchylimits.h b/valhalla/sif/hierarchylimits.h index 19de1c56e2..006f6683b4 100644 --- a/valhalla/sif/hierarchylimits.h +++ b/valhalla/sif/hierarchylimits.h @@ -18,8 +18,9 @@ constexpr float kMaxDistance = std::numeric_limits::max(); // destination. constexpr uint32_t kDefaultMaxUpTransitions[] = {0, 400, 100, 0, 0, 0, 0, 0}; -// Default distances within which expansion is always allowed -// (per level). Used only for A*. +// Default distances within which expansion is always allowed (per level). It's optimized +// for unidirectional search and can be modified by the path algorithm in case of +// bidirectional search. constexpr float kDefaultExpansionWithinDist[] = {kMaxDistance, 100000.0f, 5000.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f}; } // namespace diff --git a/valhalla/thor/bidirectional_astar.h b/valhalla/thor/bidirectional_astar.h index 6be6de2e4a..dc4a63a1d0 100644 --- a/valhalla/thor/bidirectional_astar.h +++ b/valhalla/thor/bidirectional_astar.h @@ -240,6 +240,11 @@ class BidirectionalAStar : public PathAlgorithm { const valhalla::Location& dest, const baldr::TimeInfo& time_info, const bool invariant); + + /** + * Modify default (optimized for unidirectional search) hierarchy limits. + */ + void ModifyHierarchyLimits(); }; // This function checks if the path formed by the two expanding trees