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.
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
set not_thru for all edges in region #3812
Draft
nilsnolde
wants to merge
1
commit into
master
Choose a base branch
from
nn-no_thru-regional
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+18
−0
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
|
@@ -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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this note is redundant. it doesn't matter really, constant |
||
// 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); | ||
|
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.
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
?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.
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.
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.
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 firstnot_thru = false
edge it'll actually start pruning the path on the next expandednot_thru = true
edge. Hope that made sense.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.
However, I didn't look into the potential issue @gknisely mentioned with
only_*
turn restrictions.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.
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!
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.
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..).
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.
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:
if there's a turn restriction from
AB
->BC
(i.e. we could only go straight there) and we want to go from1
->2
, we'd have to enter thatnot_thru
region to the right, make a loop and take the left-turn atB
. I think as long as we don't turn onnot_thru_pruning
for correlated edge labels this should be fine (likely the reason why we currently do exactly that).