set not_thru for all edges in region

[nilsnolde]

fixes #3807

Will see another day if it'll work, just leaving some comments now after looking over the bidirectional ones.

[nilsnolde]

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

[nilsnolde]

apparently there's some complication around only_* complex restrictions and not_thru greg just made us aware of:

https://github.com/valhalla/valhalla/blob/a0af4ccfd1aee48646c6d8421591b82f2f458ced/test/gurka/test_not_thru_pruning.cc

[nilsnolde]

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?

[dnesbitt61]

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.

[nilsnolde]

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.

[nilsnolde]

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

[dnesbitt61]

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!

[nilsnolde]

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..).

[nilsnolde]

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).