From 274400ab00f6f2d43c149c40ba1d14ebbd251c19 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Wed, 20 Mar 2024 15:13:06 +0100 Subject: [PATCH 1/5] some bug fixes ported over from #4638 --- src/sif/pedestriancost.cc | 1 - src/sif/recost.cc | 2 +- src/thor/astar_bss.cc | 4 +-- src/thor/bidirectional_astar.cc | 42 ++++++++++++++++--------------- src/thor/costmatrix.cc | 32 ++++++++++++----------- src/thor/dijkstras.cc | 9 +++---- src/thor/multimodal.cc | 4 +-- src/thor/route_action.cc | 1 + src/thor/timedistancebssmatrix.cc | 12 ++++----- src/thor/timedistancematrix.cc | 36 ++++++++++++++++---------- src/thor/unidirectional_astar.cc | 8 +++--- 11 files changed, 82 insertions(+), 69 deletions(-) diff --git a/src/sif/pedestriancost.cc b/src/sif/pedestriancost.cc index 3d71a925fa..f45dacc399 100644 --- a/src/sif/pedestriancost.cc +++ b/src/sif/pedestriancost.cc @@ -550,7 +550,6 @@ class PedestrianCost : public DynamicCost { (edge->use() == baldr::Use::kRailFerry && pred->use() != baldr::Use::kRailFerry); // Additional penalties without any time cost - c.cost += destination_only_penalty_ * (edge->destonly() && !pred->destonly()); c.cost += alley_penalty_ * (edge->use() == baldr::Use::kAlley && pred->use() != baldr::Use::kAlley); c.cost += diff --git a/src/sif/recost.cc b/src/sif/recost.cc index f7c422af81..499fa9398b 100644 --- a/src/sif/recost.cc +++ b/src/sif/recost.cc @@ -91,7 +91,7 @@ void recost_forward(baldr::GraphReader& reader, // TODO: if this edge begins a restriction, we need to start popping off edges into queue // so that we can find if we reach the end of the restriction. then we need to replay the // queued edges as normal - uint8_t time_restrictions_TODO = -1; + uint8_t time_restrictions_TODO = baldr::kInvalidRestriction; // if its not time dependent set to 0 for Allowed method below const uint64_t localtime = offset_time.valid ? offset_time.local_time : 0; // we should call 'Allowed' method even if 'ignore_access' flag is true in order to diff --git a/src/thor/astar_bss.cc b/src/thor/astar_bss.cc index f0857de070..c6cf719117 100644 --- a/src/thor/astar_bss.cc +++ b/src/thor/astar_bss.cc @@ -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, 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(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(); diff --git a/src/thor/bidirectional_astar.cc b/src/thor/bidirectional_astar.cc index 61de6281f9..9fa73b9b66 100644 --- a/src/thor/bidirectional_astar.cc +++ b/src/thor/bidirectional_astar.cc @@ -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 = -1; + 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. - 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. + 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, thru, + sortcost, dist, mode_, transition_cost, not_thru_pruning, (pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), static_cast(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, 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)), static_cast(flow_sources & kDefaultFlowMask), costing_->TurnType(meta.edge->localedgeidx(), nodeinfo, opp_edge, opp_pred_edge), @@ -363,7 +365,7 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, // we've just added this edge to the queue, but we won't expand from it if it's a not-thru edge that // will be pruned. In that case we want to allow uturns. - return !(pred.not_thru_pruning() && meta.edge->not_thru()); + return not_thru_pruning_ && !(pred.not_thru_pruning() && meta.edge->not_thru()); } template @@ -422,13 +424,13 @@ 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 - uturn_meta = pred.opp_local_idx() == meta.edge->localedgeidx() ? meta : uturn_meta; + bool is_uturn = pred.opp_local_idx() == meta.edge->localedgeidx(); + 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(graphreader, pred, opp_pred_edge, nodeinfo, pred_idx, meta, - shortcuts, tile, offset_time)) || + !is_uturn && ExpandInner(graphreader, pred, opp_pred_edge, nodeinfo, + pred_idx, meta, shortcuts, tile, offset_time) || disable_uturn; } @@ -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_, - -1, !(costing_->IsClosed(directededge, tile)), + kInvalidRestriction, !(costing_->IsClosed(directededge, tile)), static_cast(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(); + pruning_disabled_at_origin_ = + pruning_disabled_at_origin_ || !edgelabels_forward_.back().closure_pruning() || + !edgelabels_forward_.back().not_thru_pruning() || edgelabels_forward_.back().destonly(); } // 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(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())); 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(); + pruning_disabled_at_destination_ = + pruning_disabled_at_destination_ || !edgelabels_reverse_.back().closure_pruning() || + !edgelabels_reverse_.back().not_thru_pruning() || edgelabels_reverse_.back().destonly(); } } diff --git a/src/thor/costmatrix.cc b/src/thor/costmatrix.cc index db67b68210..12304abe57 100644 --- a/src/thor/costmatrix.cc +++ b/src/thor/costmatrix.cc @@ -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. + 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(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(flow_sources & kDefaultFlowMask), costing_->TurnType(meta.edge->localedgeidx(), nodeinfo, opp_edge, opp_pred_edge), @@ -552,7 +552,7 @@ bool CostMatrix::ExpandInner(baldr::GraphReader& graphreader, pred_dist, newcost.cost); } - return !(pred.not_thru_pruning() && meta.edge->not_thru()); + return not_thru_pruning_ && !(pred.not_thru_pruning() && meta.edge->not_thru()); } template @@ -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(directededge->length())); - BDEdgeLabel edge_label(kInvalidLabel, edgeid, oppedge, directededge, cost, mode_, ec, d, false, - true, static_cast(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(flow_sources & kDefaultFlowMask), + InternalTurn::kNoTurn, kInvalidRestriction, static_cast(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(directededge->length())); BDEdgeLabel edge_label(kInvalidLabel, opp_edge_id, edgeid, opp_dir_edge, cost, mode_, ec, d, - false, true, static_cast(flow_sources & kDefaultFlowMask), - InternalTurn::kNoTurn, -1, + !opp_dir_edge->not_thru(), !(costing_->IsClosed(directededge, tile)), + static_cast(flow_sources & kDefaultFlowMask), + InternalTurn::kNoTurn, kInvalidRestriction, static_cast(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). diff --git a/src/thor/dijkstras.cc b/src/thor/dijkstras.cc index dae5ac87b1..24174d02c3 100644 --- a/src/thor/dijkstras.cc +++ b/src/thor/dijkstras.cc @@ -178,7 +178,7 @@ void Dijkstras::ExpandInner(baldr::GraphReader& graphreader, // Check if the edge is allowed or if a restriction occurs EdgeStatus* todo = nullptr; - uint8_t restriction_idx = -1; + uint8_t restriction_idx = kInvalidRestriction; // is_dest is false, because it is a traversal algorithm in this context, not a path search // algorithm. In other words, destination edges are not defined for this Dijkstra's algorithm. const bool is_dest = false; @@ -259,7 +259,7 @@ void Dijkstras::ExpandInner(baldr::GraphReader& graphreader, } else { bdedgelabels_.emplace_back(pred_idx, edgeid, oppedgeid, directededge, newcost, mode_, transition_cost, path_dist, false, - (pred.closure_pruning() || !costing_->IsClosed(directededge, tile)), + (pred.closure_pruning() || !costing_->IsClosed(opp_edge, t2)), static_cast(flow_sources & kDefaultFlowMask), costing_->TurnType(directededge->localedgeidx(), nodeinfo, opp_edge, opp_pred_edge), @@ -524,7 +524,7 @@ void Dijkstras::ExpandForwardMultiModal(GraphReader& graphreader, // costing - assume if you get a transit edge you walked to the transit stop uint32_t tripid = 0; uint32_t blockid = 0; - uint8_t restriction_idx = -1; + uint8_t restriction_idx = kInvalidRestriction; const bool is_dest = false; if (directededge->IsTransitLine()) { // Check if transit costing allows this edge @@ -818,11 +818,10 @@ void Dijkstras::SetOriginLocations(GraphReader& graphreader, // Construct the edge label. Set the predecessor edge index to invalid // to indicate the origin of the path. uint32_t idx = bdedgelabels_.size(); - int restriction_idx = -1; bdedgelabels_.emplace_back(kInvalidLabel, edgeid, opp_edge_id, directededge, cost, mode_, Cost{}, path_dist, false, !(costing_->IsClosed(directededge, tile)), static_cast(flow_sources & kDefaultFlowMask), - InternalTurn::kNoTurn, restriction_idx, multipath_ ? path_id : 0, + InternalTurn::kNoTurn, kInvalidRestriction, multipath_ ? path_id : 0, directededge->destonly() || (costing_->is_hgv() && directededge->destonly_hgv())); // Set the origin flag diff --git a/src/thor/multimodal.cc b/src/thor/multimodal.cc index d57086263c..498ad6bd6f 100644 --- a/src/thor/multimodal.cc +++ b/src/thor/multimodal.cc @@ -751,7 +751,7 @@ bool MultiModalPathAlgorithm::ExpandFromNode(baldr::GraphReader& graphreader, // Add edge label, add to the adjacency list and set edge status uint32_t idx = edgelabels.size(); edgelabels.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode_, - walking_distance, baldr::kInvalidRestriction, true, false, + walking_distance, baldr::kInvalidRestriction, false, false, InternalTurn::kNoTurn); *es = {EdgeSet::kTemporary, idx}; adjlist.add(idx); @@ -809,7 +809,7 @@ bool MultiModalPathAlgorithm::CanReachDestination(const valhalla::Location& dest // we cannot do transition_cost on this label yet because we have no predecessor, but when we find // it, we will do an update on it and set the real transition cost based on the path to it edgelabels.emplace_back(kInvalidLabel, oppedge, diredge, cost, cost.cost, mode_, length, - baldr::kInvalidRestriction, true, false, InternalTurn::kNoTurn); + baldr::kInvalidRestriction, false, false, InternalTurn::kNoTurn); adjlist.add(label_idx); edgestatus.Set(oppedge, EdgeSet::kTemporary, label_idx, tile); label_idx++; diff --git a/src/thor/route_action.cc b/src/thor/route_action.cc index 0ed8fc0095..9d048ee216 100644 --- a/src/thor/route_action.cc +++ b/src/thor/route_action.cc @@ -305,6 +305,7 @@ std::vector> thor_worker_t::get_path(PathAlgorithm* // If bidirectional A* disable use of destination-only edges on the // first pass. If there is a failure, we allow them on the second pass. // Other path algorithms can use destination-only edges on the first pass. + // TODO(nils): why not others with destonly pruning? it gets a 2nd pass as well cost->set_allow_destination_only(path_algorithm == &bidir_astar ? false : true); cost->set_pass(0); diff --git a/src/thor/timedistancebssmatrix.cc b/src/thor/timedistancebssmatrix.cc index cb993af199..b5493c8b11 100644 --- a/src/thor/timedistancebssmatrix.cc +++ b/src/thor/timedistancebssmatrix.cc @@ -94,7 +94,7 @@ void TimeDistanceBSSMatrix::Expand(GraphReader& graphreader, // Skip this edge if permanently labeled (best path already found to this // directed edge), if no access is allowed to this edge (based on costing // method), or if a complex restriction prevents this path. - uint8_t restriction_idx = -1; + uint8_t restriction_idx = kInvalidRestriction; const bool is_dest = dest_edges_.find(edgeid.value) != dest_edges_.cend(); if (FORWARD) { if (!current_costing->Allowed(directededge, is_dest, pred, tile, edgeid, 0, 0, @@ -138,7 +138,7 @@ void TimeDistanceBSSMatrix::Expand(GraphReader& graphreader, // Add to the adjacency list and edge labels. uint32_t idx = edgelabels_.size(); edgelabels_.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode, - path_distance, restriction_idx, true, false, InternalTurn::kNoTurn); + path_distance, restriction_idx, false, false, InternalTurn::kNoTurn); *es = {EdgeSet::kTemporary, idx}; adjacencylist_.add(idx); } @@ -315,11 +315,11 @@ void TimeDistanceBSSMatrix::SetOrigin(GraphReader& graphreader, const valhalla:: dist = static_cast(directededge->length() * percent_along); } else { - opp_edge_id = graphreader.GetOpposingEdgeId(edgeid); + opp_edge_id = graphreader.GetOpposingEdgeId(edgeid, endtile); if (!opp_edge_id.Is_Valid()) { continue; } - opp_dir_edge = graphreader.GetOpposingEdge(edgeid); + opp_dir_edge = graphreader.GetOpposingEdge(edgeid, endtile); cost = pedestrian_costing_->EdgeCost(opp_dir_edge, endtile, time_info, flow_sources) * edge.percent_along(); dist = static_cast(directededge->length() * edge.percent_along()); @@ -335,11 +335,11 @@ void TimeDistanceBSSMatrix::SetOrigin(GraphReader& graphreader, const valhalla:: // of the path. Set the origin flag if (FORWARD) { edgelabels_.emplace_back(kInvalidLabel, edgeid, directededge, cost, cost.cost, - travel_mode_t::kPedestrian, dist, baldr::kInvalidRestriction, true, + travel_mode_t::kPedestrian, dist, baldr::kInvalidRestriction, false, false, InternalTurn::kNoTurn); } else { edgelabels_.emplace_back(kInvalidLabel, opp_edge_id, opp_dir_edge, cost, cost.cost, - travel_mode_t::kPedestrian, dist, baldr::kInvalidRestriction, true, + travel_mode_t::kPedestrian, dist, baldr::kInvalidRestriction, false, false, InternalTurn::kNoTurn); } edgelabels_.back().set_origin(); diff --git a/src/thor/timedistancematrix.cc b/src/thor/timedistancematrix.cc index a3762e8ae7..db4e0774a8 100644 --- a/src/thor/timedistancematrix.cc +++ b/src/thor/timedistancematrix.cc @@ -105,7 +105,7 @@ void TimeDistanceMatrix::Expand(GraphReader& graphreader, // Skip this edge if permanently labeled (best path already found to this // directed edge), if no access is allowed to this edge (based on costing // method), or if a complex restriction prevents this path. - uint8_t restriction_idx = -1; + uint8_t restriction_idx = kInvalidRestriction; const bool is_dest = dest_edges_.find(edgeid) != dest_edges_.cend(); if (FORWARD) { if (!costing_->Allowed(directededge, is_dest, pred, tile, edgeid, offset_time.local_time, @@ -149,16 +149,26 @@ void TimeDistanceMatrix::Expand(GraphReader& graphreader, // Add to the adjacency list and edge labels. uint32_t idx = edgelabels_.size(); - sif::InternalTurn turn_type = - FORWARD ? costing_->TurnType(pred.opp_local_idx(), nodeinfo, directededge) - : costing_->TurnType(directededge->localedgeidx(), nodeinfo, opp_edge, opp_pred_edge); - - edgelabels_.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode_, - path_distance, restriction_idx, - (pred.closure_pruning() || !costing_->IsClosed(directededge, tile)), - static_cast(flow_sources & kDefaultFlowMask), turn_type, 0, - directededge->destonly() || - (costing_->is_hgv() && directededge->destonly_hgv())); + if (FORWARD) { + edgelabels_.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode_, + path_distance, restriction_idx, false, + (pred.closure_pruning() || !(costing_->IsClosed(directededge, tile))), + 0 != (flow_sources & kDefaultFlowMask), + costing_->TurnType(pred.opp_local_idx(), nodeinfo, directededge), 0, + directededge->destonly() || + (costing_->is_hgv() && directededge->destonly_hgv())); + } else { + edgelabels_.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode_, + path_distance, restriction_idx, false, + (pred.closure_pruning() || !(costing_->IsClosed(opp_edge, t2))), + 0 != (flow_sources & kDefaultFlowMask), + costing_->TurnType(directededge->localedgeidx(), nodeinfo, opp_edge, + opp_pred_edge), + 0, + opp_edge->destonly() || + (costing_->is_hgv() && opp_edge->destonly_hgv())); + } + *es = {EdgeSet::kTemporary, idx}; adjacencylist_.add(idx); } @@ -370,8 +380,8 @@ void TimeDistanceMatrix::SetOrigin(GraphReader& graphreader, baldr::kInvalidRestriction, !costing_->IsClosed(directededge, tile), static_cast(flow_sources & kDefaultFlowMask), InternalTurn::kNoTurn, 0, - opp_dir_edge->destonly() || - (costing_->is_hgv() && opp_dir_edge->destonly_hgv())); + directededge->destonly() || + (costing_->is_hgv() && directededge->destonly_hgv())); } edgelabels_.back().set_origin(); adjacencylist_.add(edgelabels_.size() - 1); diff --git a/src/thor/unidirectional_astar.cc b/src/thor/unidirectional_astar.cc index b740f4b935..15a055334d 100644 --- a/src/thor/unidirectional_astar.cc +++ b/src/thor/unidirectional_astar.cc @@ -281,7 +281,7 @@ inline bool UnidirectionalAStar::ExpandInner( edgelabels_.emplace_back(pred_idx, meta.edge_id, opp_edge_id, meta.edge, cost, sortcost, dist, mode_, transition_cost, (pred.not_thru_pruning() || !meta.edge->not_thru()), - (pred.closure_pruning() || !(costing_->IsClosed(meta.edge, tile))), + (pred.closure_pruning() || !(costing_->IsClosed(opp_edge, endtile))), 0 != (flow_sources & kDefaultFlowMask), costing_->TurnType(meta.edge->localedgeidx(), nodeinfo, opp_edge, opp_pred_edge), @@ -466,7 +466,7 @@ std::vector> UnidirectionalAStar::SetOrigin( !(costing_->IsClosed(directededge, tile)), 0 != (flow_sources & kDefaultFlowMask), sif::InternalTurn::kNoTurn, kInvalidRestriction, 0, - opp_dir_edge->destonly() || - (costing_->is_hgv() && opp_dir_edge->destonly_hgv())); + directededge->destonly() || + (costing_->is_hgv() && directededge->destonly_hgv())); } auto& edge_label = edgelabels_.back(); From 039fa9d0107be65551d5487b5321183ab3b896c6 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Wed, 20 Mar 2024 16:12:11 +0100 Subject: [PATCH 2/5] minor fixes --- src/thor/bidirectional_astar.cc | 10 +++++----- src/thor/costmatrix.cc | 2 +- src/thor/timedistancematrix.cc | 4 ++-- valhalla/sif/costfactory.h | 6 ++++++ 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/thor/bidirectional_astar.cc b/src/thor/bidirectional_astar.cc index 9fa73b9b66..1d31b9bef2 100644 --- a/src/thor/bidirectional_astar.cc +++ b/src/thor/bidirectional_astar.cc @@ -365,7 +365,7 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, // we've just added this edge to the queue, but we won't expand from it if it's a not-thru edge that // will be pruned. In that case we want to allow uturns. - return not_thru_pruning_ && !(pred.not_thru_pruning() && meta.edge->not_thru()); + return !(pred.not_thru_pruning() && meta.edge->not_thru()); } template @@ -428,10 +428,10 @@ void BidirectionalAStar::Expand(baldr::GraphReader& graphreader, 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(graphreader, pred, opp_pred_edge, nodeinfo, - pred_idx, meta, shortcuts, tile, offset_time) || - disable_uturn; + disable_uturn = (!is_uturn && ExpandInner(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 diff --git a/src/thor/costmatrix.cc b/src/thor/costmatrix.cc index 12304abe57..b260177e41 100644 --- a/src/thor/costmatrix.cc +++ b/src/thor/costmatrix.cc @@ -552,7 +552,7 @@ bool CostMatrix::ExpandInner(baldr::GraphReader& graphreader, pred_dist, newcost.cost); } - return not_thru_pruning_ && !(pred.not_thru_pruning() && meta.edge->not_thru()); + return !(pred.not_thru_pruning() && meta.edge->not_thru()); } template diff --git a/src/thor/timedistancematrix.cc b/src/thor/timedistancematrix.cc index db4e0774a8..c8abb4a633 100644 --- a/src/thor/timedistancematrix.cc +++ b/src/thor/timedistancematrix.cc @@ -151,7 +151,7 @@ void TimeDistanceMatrix::Expand(GraphReader& graphreader, uint32_t idx = edgelabels_.size(); if (FORWARD) { edgelabels_.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode_, - path_distance, restriction_idx, false, + path_distance, restriction_idx, (pred.closure_pruning() || !(costing_->IsClosed(directededge, tile))), 0 != (flow_sources & kDefaultFlowMask), costing_->TurnType(pred.opp_local_idx(), nodeinfo, directededge), 0, @@ -159,7 +159,7 @@ void TimeDistanceMatrix::Expand(GraphReader& graphreader, (costing_->is_hgv() && directededge->destonly_hgv())); } else { edgelabels_.emplace_back(pred_idx, edgeid, directededge, newcost, newcost.cost, mode_, - path_distance, restriction_idx, false, + path_distance, restriction_idx, (pred.closure_pruning() || !(costing_->IsClosed(opp_edge, t2))), 0 != (flow_sources & kDefaultFlowMask), costing_->TurnType(directededge->localedgeidx(), nodeinfo, opp_edge, diff --git a/valhalla/sif/costfactory.h b/valhalla/sif/costfactory.h index a512f59720..a88eaed361 100644 --- a/valhalla/sif/costfactory.h +++ b/valhalla/sif/costfactory.h @@ -101,6 +101,7 @@ class CostFactory { mode_costing_t CreateModeCosting(const Options& options, TravelMode& mode) { mode_costing_t mode_costing; + mode = TravelMode::kMaxTravelMode; // Set travel mode and construct costing(s) for this type for (const auto& costing : kCostingTypeMapping.at(options.costing_type())) { valhalla::sif::cost_ptr_t cost = Create(options.costings().find(costing)->second); @@ -112,6 +113,11 @@ class CostFactory { // For multi-modal we set the initial mode to pedestrian. (TODO - allow other initial modes) mode = valhalla::sif::TravelMode::kPedestrian; } + // this should never happen + if (mode == TravelMode::kMaxTravelMode) { + throw std::runtime_error("sif::CostFactory couldn't find a valid TravelMode for " + + Costing_Enum_Name(options.costing_type())); + } return mode_costing; } From f20fdd5cc2062e226282b06a23116490a8dd87ee Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Wed, 20 Mar 2024 16:25:24 +0100 Subject: [PATCH 3/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f2ced4e40..df262d415b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ * FIXED: some entry points to creating geotiff isochrones output did not register the geotiff driver before attempting to use it [#4628](https://github.com/valhalla/valhalla/pull/4628) * FIXED: libgdal wasn't installed in docker image, so it never worked in docker [#4629](https://github.com/valhalla/valhalla/pull/4629) * FIXED: CostMatrix shapes for routes against trivial oneways [#4633](https://github.com/valhalla/valhalla/pull/4633) + * FIXED: a few fixes around the routing algorithms [#4626](https://github.com/valhalla/valhalla/pull/4642) * **Enhancement** * UPDATED: French translations, thanks to @xlqian [#4159](https://github.com/valhalla/valhalla/pull/4159) * CHANGED: -j flag for multithreaded executables to override mjolnir.concurrency [#4168](https://github.com/valhalla/valhalla/pull/4168) From c8a557c83f0b86633cbe10f0be14c14bdad75f14 Mon Sep 17 00:00:00 2001 From: Nils Date: Mon, 25 Mar 2024 14:02:31 +0100 Subject: [PATCH 4/5] Update pedestriancost.cc --- src/sif/pedestriancost.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sif/pedestriancost.cc b/src/sif/pedestriancost.cc index f45dacc399..dce186833c 100644 --- a/src/sif/pedestriancost.cc +++ b/src/sif/pedestriancost.cc @@ -550,6 +550,7 @@ class PedestrianCost : public DynamicCost { (edge->use() == baldr::Use::kRailFerry && pred->use() != baldr::Use::kRailFerry); // Additional penalties without any time cost + c.cost += destination_only_penalty_ * (edge->destonly() && !pred->destonly()); c.cost += alley_penalty_ * (edge->use() == baldr::Use::kAlley && pred->use() != baldr::Use::kAlley); c.cost += @@ -602,7 +603,7 @@ PedestrianCost::PedestrianCost(const Costing& costing) speed_ = costing_options.walking_speed(); step_penalty_ = costing_options.step_penalty(); elevator_penalty_ = costing_options.elevator_penalty(); - max_grade_ = costing_options.max_grade(); + max_grade_ = costing_options.max_grade();private if (type_ == PedestrianType::kFoot) { max_hiking_difficulty_ = static_cast(costing_options.max_hiking_difficulty()); From 8387b1b70117d150cf92542b8b242438d3da997d Mon Sep 17 00:00:00 2001 From: Nils Date: Mon, 25 Mar 2024 14:03:12 +0100 Subject: [PATCH 5/5] Update pedestriancost.cc --- src/sif/pedestriancost.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sif/pedestriancost.cc b/src/sif/pedestriancost.cc index dce186833c..3d71a925fa 100644 --- a/src/sif/pedestriancost.cc +++ b/src/sif/pedestriancost.cc @@ -603,7 +603,7 @@ PedestrianCost::PedestrianCost(const Costing& costing) speed_ = costing_options.walking_speed(); step_penalty_ = costing_options.step_penalty(); elevator_penalty_ = costing_options.elevator_penalty(); - max_grade_ = costing_options.max_grade();private + max_grade_ = costing_options.max_grade(); if (type_ == PedestrianType::kFoot) { max_hiking_difficulty_ = static_cast(costing_options.max_hiking_difficulty());