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

set not_thru for all edges in region #3812

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

set not_thru for all edges in region #3812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/mjolnir/graphenhancer.cc
View file
Open in desktop
Expand Up @@ -1668,6 +1668,9 @@ void enhance(const boost::property_tree::ptree& pt,
// Check for not_thru edge (only on low importance edges). Exclude
// transit edges
if (directededge.classification() > RoadClass::kTertiary) {
// (nils): if we want to update all edges in a not_thru region, we'd have to
// go through more tiles than just this one.. we could record the entire set of
// not_thru edges for all tiles and rip through them again after this big loop?
if (IsNotThruEdge(reader, lock, startnode, directededge)) {
Comment on lines +1671 to 1674
Copy link
Member Author

Choose a reason for hiding this comment

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

so this is going through a tile at a time and each edge at a time for that tile and sets it not thru. if we want to do the whole no_thru area we'd need to possibly look at more tiles than this one and hence can't do it anymore in this loop.

I lack the experience to tell if it'd be ok to only record all edge id's which should be set not_thru here and later go through all affected tiles again to update those edges with no_thru?

Copy link
Member

Choose a reason for hiding this comment

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

You do not mean setting the DirectedEdge not_thru flag do you? That seems risky as only edges going "into" the region can be marked as not_thru. The reverse search needs to be able to get out of a not_thru region. I think a new flag is needed to identify all edges within a not_thru area.

Copy link
Member Author

Choose a reason for hiding this comment

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

You do not mean setting the DirectedEdge not_thru flag do you?

I actually do mean that @dnesbitt61 :) I couldn't see code that'd break if we do that. I looked into both bidirectional algos and they support it out-of-the-box I'm pretty sure (read my other comments in the bidir astar and bidir costmatrix). The reason is that we always set correlated edges of locations (both origin & destination) to not_thru_pruning = false so it won't be pruned if we're starting the reverse tree inside one of these regions AND all edges in the not_thru region are marked. Only once the expansion encounters its first not_thru = false edge it'll actually start pruning the path on the next expanded not_thru = true edge. Hope that made sense.

Copy link
Member Author

@nilsnolde nilsnolde Nov 18, 2022 •
edited

Choose a reason for hiding this comment

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

However, I didn't look into the potential issue @gknisely mentioned with only_* turn restrictions.

Copy link
Member

Choose a reason for hiding this comment

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

not 100% sure about this, but I think that makes sense. If the destination is in a not_thru region it would expand without not_thru culling until it encounters an edge that is !not_thru, after which it would enable not_thru culling. Seems correct, but if any one edge is incorrectly marked inside that region (e.g. missed in the marking of not_thru edges) there would be issues. Thanks for the explanation. Perhaps in the near future I can look into adding this logic to tile building. Starting in January I may have more time to devote to these efforts!

Copy link
Member Author

Choose a reason for hiding this comment

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

but if any one edge is incorrectly marked inside that region (e.g. missed in the marking of not_thru edges) there would be issues.

True, but I'd assume that wouldn't happen as it seems we can accurately determine which edges are inside such a region during the tile build (build performance could be quite affected though..).

Copy link
Member Author

@nilsnolde nilsnolde Mar 20, 2024 •
edited

Choose a reason for hiding this comment

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

However, I didn't look into the potential issue @gknisely mentioned with only_* turn restrictions.

working on #4638, I got a pretty good idea of all the pitfalls around not_thru stuff. and I know now what @gknisely was hinting at here:

                        x--------x
                        |        |
      A-1-----B---------x        |
              |         |        |
              |         x--------x
              2
              |
              C

if there's a turn restriction from AB -> BC (i.e. we could only go straight there) and we want to go from 1 -> 2, we'd have to enter that not_thru region to the right, make a loop and take the left-turn at B. I think as long as we don't turn on not_thru_pruning for correlated edge labels this should be fine (likely the reason why we currently do exactly that).

directededge.set_not_thru(true);
stats.not_thru++;
Expand Down
8 changes: 8 additions & 0 deletions src/thor/bidirectional_astar.cc
View file
Open in desktop
Expand Up @@ -319,6 +319,10 @@ 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.
// NOTE(nils): not_thru_pruning() will be false for the correlated (i.e. "first") edges as pred. if
// the current edge is also not_thru we won't prune in the next round. if the current edge is not a
// not_thru we will start pruning next round. that ensures that we can start on a not_thru edge and
// continue on other not_thru edges before pruning.
bool thru = not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false;

// Add edge label, add to the adjacency list and set edge status
Expand Down Expand Up @@ -604,6 +608,10 @@ BidirectionalAStar::GetBestPath(valhalla::Location& origin,
// on later, causing us to needlessly expand when we could have aborted sooner. However, it
// ensures that most impossible route will fail fast provided one of the locations didn't
// start from a not_thru/closed edge
// TODO(nils):
// 1. extended_search doesn't seem to do what it's documented to do; it will force both
// both directions to be eventually exhausted, no matter if the other direction
// started on a closure/not_thru
if (!extended_search_ || !pruning_disabled_at_destination_) {
return {};
}
Expand Down
7 changes: 7 additions & 0 deletions src/thor/costmatrix.cc
View file
Open in desktop
Expand Up @@ -567,6 +567,11 @@ void CostMatrix::BackwardSearch(const uint32_t index, GraphReader& graphreader)
edgestate.Update(pred.edgeid(), EdgeSet::kPermanent);

// Prune path if predecessor is not a through edge
// NOTE(nils): this will also be forward-compatible
// this is currently a problem when edges in not_thru
// areas are not marked, as expansion will just stop in this direction
// and continue in the other direction even though it wouldn't be able to
// find this tree inside the not_thru region
if (pred.not_thru() && pred.not_thru_pruning()) {
return;
}
Expand Down Expand Up @@ -777,6 +782,8 @@ void CostMatrix::SetSources(GraphReader& graphreader,

// Set the initial not_thru flag to false. There is an issue with not_thru
// flags on small loops. Set this to false here to override this for now.
// NOTE(nils): shouldn't not_thru_pruning be set to the opposite of the edge's not_thru flag?
Copy link
Member Author

Choose a reason for hiding this comment

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

this note is redundant. it doesn't matter really, constant false is as good as dynamic since we force the not_thru flag on the correlated edges anyways

// current code should also be compatible with new data
BDEdgeLabel edge_label(kInvalidLabel, edgeid, oppedge, directededge, cost, mode_, ec, d, false,
true, static_cast<bool>(flow_sources & kDefaultFlowMask),
InternalTurn::kNoTurn, -1);
Expand Down