Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor path pruning for not_thru/destonly/closures #4638

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

refactor path pruning for not_thru/destonly/closures #4638

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a369698
all refactored (and fixed), but only not_thru is reviewed so far
nilsnolde Mar 18, 2024
e9a94a1
reviewed destonly as well; still no tests
nilsnolde Mar 18, 2024
8b4d88e
small b ug
nilsnolde Mar 18, 2024
5038dbe
some amends
nilsnolde Mar 18, 2024
54f9db4
some exes not updated
nilsnolde Mar 18, 2024
b8b4b18
fix astar.cc
nilsnolde Mar 18, 2024
607d3cd
changelog
nilsnolde Mar 18, 2024
c5749f6
don't change the default
nilsnolde Mar 19, 2024
5865eb9
revert the changes to map matching algos; still doesn't fix the mapma…
nilsnolde Mar 19, 2024
579482f
some fixes
nilsnolde Mar 19, 2024
997a40f
braces
nilsnolde Mar 19, 2024
d990027
destonly needs to be a separate EdgeLabel property because transition…
nilsnolde Mar 19, 2024
6cd0e92
see easier diff
nilsnolde Mar 19, 2024
6eba010
now I see the disadvantage of putting not_thru_pruning on the cost ra…
nilsnolde Mar 20, 2024
9a5be0a
add not_thru_pruning to all other costing models; this fails a condit…
nilsnolde Mar 20, 2024
7abd7f3
disable not_thru pruning for algos which don't have a second pass
nilsnolde Mar 20, 2024
689707a
disable failing test_route test
nilsnolde Mar 20, 2024
b71e92b
forgot to add back dest_only to default constructor
nilsnolde Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/mjolnir/graphenhancer.cc
View file
Open in desktop
Expand Up @@ -534,6 +534,7 @@ bool IsNotThruEdge(GraphReader& reader,

// Return false if we get back to the start node or hit an
// edge with higher classification
// TODO(nils): why does higher classification immediately tell it's not "not_thru"?
if (diredge->classification() < RoadClass::kTertiary || diredge->endnode() == startnode) {
return false;
}
Expand Down
18 changes: 12 additions & 6 deletions src/sif/autocost.cc
View file
Open in desktop
Expand Up @@ -423,7 +423,8 @@ bool AutoCost::Allowed(const baldr::DirectedEdge* edge,
if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_turn_restrictions_) ||
edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) ||
(!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && edge->destonly()) ||
(not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) ||
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the not_thru check into Allowed(Reverse)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that means it will NOT be put in the adjacency list and that is what's breaking the test below in gurka_route

(pred.closure_pruning() && IsClosed(edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && edge->unpaved()) || !IsHOVAllowed(edge)) {
return false;
Expand All @@ -448,7 +449,8 @@ bool AutoCost::AllowedReverse(const baldr::DirectedEdge* edge,
if (!IsAccessible(opp_edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((opp_edge->restrictions() & (1 << pred.opp_local_idx())) && !ignore_turn_restrictions_) ||
opp_edge->surface() == Surface::kImpassable || IsUserAvoidEdge(opp_edgeid) ||
(!allow_destination_only_ && !pred.destonly() && opp_edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && opp_edge->destonly()) ||
(not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) ||
(pred.closure_pruning() && IsClosed(opp_edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && opp_edge->unpaved()) || !IsHOVAllowed(opp_edge)) {
return false;
Expand Down Expand Up @@ -785,7 +787,8 @@ bool BusCost::Allowed(const baldr::DirectedEdge* edge,
if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_restrictions_) ||
edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) ||
(!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && edge->destonly()) ||
(not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) ||
(pred.closure_pruning() && IsClosed(edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && edge->unpaved())) {
return false;
Expand All @@ -810,7 +813,8 @@ bool BusCost::AllowedReverse(const baldr::DirectedEdge* edge,
if (!IsAccessible(opp_edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((opp_edge->restrictions() & (1 << pred.opp_local_idx())) && !ignore_turn_restrictions_) ||
opp_edge->surface() == Surface::kImpassable || IsUserAvoidEdge(opp_edgeid) ||
(!allow_destination_only_ && !pred.destonly() && opp_edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && opp_edge->destonly()) ||
(not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) ||
(pred.closure_pruning() && IsClosed(opp_edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && opp_edge->unpaved())) {
return false;
Expand Down Expand Up @@ -964,7 +968,8 @@ bool TaxiCost::Allowed(const baldr::DirectedEdge* edge,
if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_restrictions_) ||
edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) ||
(!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && edge->destonly()) ||
(not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) ||
(pred.closure_pruning() && IsClosed(edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && edge->unpaved())) {
return false;
Expand All @@ -989,7 +994,8 @@ bool TaxiCost::AllowedReverse(const baldr::DirectedEdge* edge,
if (!IsAccessible(opp_edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((opp_edge->restrictions() & (1 << pred.opp_local_idx())) && !ignore_turn_restrictions_) ||
opp_edge->surface() == Surface::kImpassable || IsUserAvoidEdge(opp_edgeid) ||
(!allow_destination_only_ && !pred.destonly() && opp_edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && opp_edge->destonly()) ||
(not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) ||
(pred.closure_pruning() && IsClosed(opp_edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && opp_edge->unpaved())) {
return false;
Expand Down
19 changes: 12 additions & 7 deletions src/sif/dynamiccost.cc
View file
Open in desktop
Expand Up @@ -146,9 +146,9 @@ DynamicCost::DynamicCost(const Costing& costing,
const TravelMode mode,
uint32_t access_mask,
bool penalize_uturns)
: pass_(0), allow_transit_connections_(false), allow_destination_only_(true),
allow_conditional_destination_(false), travel_mode_(mode), access_mask_(access_mask),
closure_factor_(kDefaultClosureFactor), flow_mask_(kDefaultFlowMask),
: pass_(0), allow_transit_connections_(false), destination_only_pruning_(true),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed the members to be consistent (now it has the opposite meaning of before, so I had to change all logic where this is used or set)

conditional_destination_only_pruning_(true), not_thru_pruning_(true), travel_mode_(mode),
access_mask_(access_mask), closure_factor_(kDefaultClosureFactor), flow_mask_(kDefaultFlowMask),
shortest_(costing.options().shortest()),
ignore_restrictions_(costing.options().ignore_restrictions()),
ignore_non_vehicular_restrictions_(costing.options().ignore_non_vehicular_restrictions()),
Expand Down Expand Up @@ -239,13 +239,18 @@ void DynamicCost::SetAllowTransitConnections(const bool allow) {
}

// Sets the flag indicating whether destination only edges are allowed.
void DynamicCost::set_allow_destination_only(const bool allow) {
allow_destination_only_ = allow;
void DynamicCost::set_destination_only_pruning(const bool allow) {
destination_only_pruning_ = allow;
}

// Sets the flag indicating whether edges with valid restriction conditional=destination are allowed.
void DynamicCost::set_allow_conditional_destination(const bool allow) {
allow_conditional_destination_ = allow;
void DynamicCost::set_conditional_destination_only_pruning(const bool allow) {
conditional_destination_only_pruning_ = allow;
}

// Sets the flag indicating whether we should prune paths leading into not_thru regions.
void DynamicCost::set_not_thru_pruning(const bool allow) {
not_thru_pruning_ = allow;
}

// Returns the maximum transfer distance between stops that you are willing
Expand Down
4 changes: 2 additions & 2 deletions src/sif/motorcyclecost.cc
View file
Open in desktop
Expand Up @@ -357,7 +357,7 @@ bool MotorcycleCost::Allowed(const baldr::DirectedEdge* edge,
if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_turn_restrictions_) ||
(edge->surface() > kMinimumMotorcycleSurface) || IsUserAvoidEdge(edgeid) ||
(!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && edge->destonly()) ||
(pred.closure_pruning() && IsClosed(edge, tile))) {
return false;
}
Expand All @@ -381,7 +381,7 @@ bool MotorcycleCost::AllowedReverse(const baldr::DirectedEdge* edge,
if (!IsAccessible(opp_edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((opp_edge->restrictions() & (1 << pred.opp_local_idx())) && !ignore_turn_restrictions_) ||
(opp_edge->surface() > kMinimumMotorcycleSurface) || IsUserAvoidEdge(opp_edgeid) ||
(!allow_destination_only_ && !pred.destonly() && opp_edge->destonly()) ||
(destination_only_pruning_ && !pred.destonly_pruning() && opp_edge->destonly()) ||
(pred.closure_pruning() && IsClosed(opp_edge, tile))) {
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/sif/motorscootercost.cc
View file
Open in desktop
Expand Up @@ -378,7 +378,7 @@ bool MotorScooterCost::Allowed(const baldr::DirectedEdge* edge,
if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_turn_restrictions_) ||
(edge->surface() > kMinimumScooterSurface) || IsUserAvoidEdge(edgeid) ||
(!allow_destination_only_ && !pred.destonly() && edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && edge->destonly()) ||
(pred.closure_pruning() && IsClosed(edge, tile))) {
return false;
}
Expand All @@ -402,7 +402,7 @@ bool MotorScooterCost::AllowedReverse(const baldr::DirectedEdge* edge,
if (!IsAccessible(opp_edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((opp_edge->restrictions() & (1 << pred.opp_local_idx())) && !ignore_turn_restrictions_) ||
(opp_edge->surface() > kMinimumScooterSurface) || IsUserAvoidEdge(opp_edgeid) ||
(!allow_destination_only_ && !pred.destonly() && opp_edge->destonly()) ||
(destination_only_pruning_ && pred.destonly_pruning() && opp_edge->destonly()) ||
(pred.closure_pruning() && IsClosed(opp_edge, tile))) {
return false;
}
Expand Down
1 change: 0 additions & 1 deletion src/sif/pedestriancost.cc
View file
Open in desktop
Expand Up @@ -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 +=
Expand Down
2 changes: 1 addition & 1 deletion src/sif/recost.cc
View file
Open in desktop
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/sif/truckcost.cc
View file
Open in desktop
Expand Up @@ -441,7 +441,7 @@ inline bool TruckCost::Allowed(const baldr::DirectedEdge* edge,
if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((pred.restrictions() & (1 << edge->localedgeidx())) && (!ignore_turn_restrictions_)) ||
edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) ||
(!allow_destination_only_ && !pred.destonly() && edge->destonly_hgv()) ||
(destination_only_pruning_ && pred.destonly_pruning() && edge->destonly_hgv()) ||
(pred.closure_pruning() && IsClosed(edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && edge->unpaved())) {
return false;
Expand All @@ -465,7 +465,7 @@ bool TruckCost::AllowedReverse(const baldr::DirectedEdge* edge,
if (!IsAccessible(opp_edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) ||
((opp_edge->restrictions() & (1 << pred.opp_local_idx())) && !ignore_turn_restrictions_) ||
opp_edge->surface() == Surface::kImpassable || IsUserAvoidEdge(opp_edgeid) ||
(!allow_destination_only_ && !pred.destonly() && opp_edge->destonly_hgv()) ||
(destination_only_pruning_ && pred.destonly_pruning() && opp_edge->destonly_hgv()) ||
(pred.closure_pruning() && IsClosed(opp_edge, tile)) ||
(exclude_unpaved_ && !pred.unpaved() && opp_edge->unpaved())) {
return false;
Expand Down
8 changes: 5 additions & 3 deletions src/thor/astar_bss.cc
View file
Open in desktop
Expand Up @@ -219,9 +219,11 @@ void AStarBSSAlgorithm::ExpandForward(GraphReader& graphreader,
}

// Add to the adjacency list and edge labels.
// TODO(nils): no pruning is enabled for this algo for now, needs some investigation if a second
// pass is possible for this algo (bike doesn't allow that)
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, false, InternalTurn::kNoTurn,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no pruning at all, see comment

baldr::kInvalidRestriction);
*current_es = {EdgeSet::kTemporary, idx};
adjacencylist_.add(idx);
Expand Down Expand Up @@ -446,8 +448,8 @@ 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,
sif::InternalTurn::kNoTurn);
travel_mode_t::kPedestrian, baldr::kInvalidRestriction, false, false,
false, sif::InternalTurn::kNoTurn);
// Set the origin flag and path distance
edge_label.set_origin();
edge_label.set_path_distance(d);
Expand Down