Fix Broken Trivial Unidirectional Routes #3299
Conversation
| } | ||
| } | ||
| return false; | ||
| return p != destinations_percent_along_.end() && edge.percent_along() == p->second; |
There was a problem hiding this comment.
this here was the fix, the comparison of percent along. see, we need to confirm for the super trivial case that the edge candidates are indeed snapped to the exact same side (node) of the same edge. we can compare floats here because when an edge gets node snapped its percent_along is not the result of arithmetic/trig/etc its forced to either 0 or 1 depending on the direction of the edge.
There was a problem hiding this comment.
Awesome find & fix @kevinkreiser!
kevinkreiser
requested review from
dnesbitt61,
kdiluca,
danpat and
nilsnolde
|
... and indeed we do remove node snap edge candidates that are superfluous unless its possible that we need them to find the node snapped destination edge candidate to complete what im now calling a "super trivial route". that is a route which is just routing from a node in the graph to that same node in the graph. it turned out our check for this case casted too wide of a net and gave false positives, specifically, in the forward direction if you had a node snapped origin that was on the same edge (regular trivial route) of a NOT node snapped destination, it would see the edges were the same (trivial) but not that it wasnt actually both snapped to the same node (missed the super part). the same phenomenon happened in the reverse astar. |
|
@kevinkreiser Looks like |
|
@danpat i tried the same failure scenario with bss and it doesnt fail because for some reason the expansion happens in a different order, though they of course have different costings (ped vs auto). in both of them they start with the wrong edge, the one we should have rejected because it arrives at the origin with 0 length. but then after that unidirectional astar would keep expanding that path from the useless edge onto the correct one while bss actually starts a new path from the correct one. seems to me that somehow unidirectional astar sorts the uturn path lower than the correct origin edge. i'll write another issue about it. regarding maintainance i would say that all who are willing are maintainers of the algorithm but @xlqian is the main author |
While I was lending a hand over in #3274 i added a simple test that relied on the results of unidirecitonal path finding (in the forward direction). The route in question is a single edge starting at the beginning of the edge and going 2/3rds along it. The algorithm currently (for this specific ordering of the input data) returns a two edge route starting at the end of the opposing edge and then making its way to the correct edge and ending 2/3rds along. at least from my quick debug session that is what looked like happened.
i thought we removed node snap edge candidates that were the full distance along...