Bug Fix for Island Edge Candidates #1835
Conversation
…er closer high reachability edge candidates
| @@ -0,0 +1,4 @@ | |||
| -j '{"locations":[{"lat":-33.4280388615334,"lon":-70.54088473320007},{"lat":-33.4268390099302,"lon":-70.53765535354614}],"costing":"pedestrian"}' | |||
There was a problem hiding this comment.
i couldnt find any good island in utrecht so i made a island routes file instead
There was a problem hiding this comment.
You won't believe it but there is a Prison Island in Utrecht: http://prisonisland.se/en/utrecht/ -- not disconnected from the road network though 😆.
Anyway I was able to trigger the bug on master with these coordinates in Utrecht with bike costing 5.011605999215277,52.114736949245156;5.020381755701578,52.10586968702347 if that helps.
| [&pp](const candidate_t& c) -> bool { | ||
| return c.sq_distance > pp.closest_external_reachable; | ||
| }); | ||
| pp.unreachable.erase(new_end, pp.unreachable.end()); |
There was a problem hiding this comment.
this is where we remove those island candidates that we should have never kept in the first place
| @@ -642,16 +650,20 @@ struct bin_handler_t { | |||
| // the last one wasnt in the radius so replace it with this one because its better or is | |||
| // in the radius | |||
| if (!last_in_radius) { | |||
| if (closer_external_reachable) | |||
| p_itr->closest_external_reachable = batch->back().sq_distance; | |||
There was a problem hiding this comment.
as we are rejecting reachable candidates we keep track of how close the closest one was
| } | ||
| } // not in radius or better and reachable and closer than closest one outside of radius | ||
| else if (closer_external_reachable) | ||
| p_itr->closest_external_reachable = c_itr->sq_distance; |
There was a problem hiding this comment.
same thing, remember how close this rejected one was for later so we can use it to cull low reachability candidates
| @@ -0,0 +1,4 @@ | |||
| -j '{"locations":[{"lat":-33.4280388615334,"lon":-70.54088473320007},{"lat":-33.4268390099302,"lon":-70.53765535354614}],"costing":"pedestrian"}' | |||
There was a problem hiding this comment.
You won't believe it but there is a Prison Island in Utrecht: http://prisonisland.se/en/utrecht/ -- not disconnected from the road network though 😆.
Anyway I was able to trigger the bug on master with these coordinates in Utrecht with bike costing 5.011605999215277,52.114736949245156;5.020381755701578,52.10586968702347 if that helps.
|
Curious if this fixes this case: #1575 (auto route). I think I remember looking into this and thinking that edge distance was a factor (there were multiple edge candidates). Strange thing was that this was auto only and not pedestrian and bike that were impacted. |
|
@danpaz that is awesome! if its in our extract i can make a unit test, either way Ill add it to the requests file for good measure! |
kevinkreiser
dismissed stale reviews from dgearhart, dnesbitt61, and danpaz
via
5ab277f
dont use low reachability edge candidates if we ended up rejecting other closer high reachability edge candidates
so we have an issue where we are sometimes using edge candidates that are on disconnected islands and the penalties in thor are not accounting for it (especially for slower modes of travel like pedestrian or bike riders).
heres the before:
the intuition to fixing this is that we shouldnt be using disconnected island edge candidates when we ended up rejecting many other connected edge candidates that were closer to the input location. so the changes in the pr, track the closest of those rejected well-connected edge candidates and then cull any island edges which are further away than those. the result is the following:
And we get the added benefit that using islands still works: