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.

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

Fix NoRoute for waypoints with low reachability edges #2914

Merged
merged 18 commits into from
Mar 15, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* FIXED: Ensure route summaries are unique among all returned route/legs [#2874](https://github.com/valhalla/valhalla/pull/2874)
* FIXED: Fix compilation errors when boost < 1.68 and libprotobuf < 3.6 [#2878](https://github.com/valhalla/valhalla/pull/2878)
* FIXED: Allow u-turns at no-access barriers when forced by heading [#2875](https://github.com/valhalla/valhalla/pull/2875)
* FIXED: Fixed "No route found" error in case of multipoint request with locations near low reachability edges [#2914](https://github.com/valhalla/valhalla/pull/2914)
* FIXED: Python bindings installation [#2751](https://github.com/valhalla/valhalla/issues/2751)
* FIXED: Skip bindings if there's no Python development version [#2893](https://github.com/valhalla/valhalla/pull/2878)

Expand Down
4 changes: 4 additions & 0 deletions src/baldr/pathlocation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,9 @@ bool PathLocation::shares_edges(const PathLocation& other) const {
return true;
}

bool PathLocation::is_high_reachable(const PathEdge& edge) const {
Copy link
Member

Choose a reason for hiding this comment

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

nit: should be is_highly_reachable or has_high_reach

return edge.inbound_reach >= min_inbound_reach_ && edge.outbound_reach >= min_outbound_reach_;
}

} // namespace baldr
} // namespace valhalla
2 changes: 1 addition & 1 deletion src/loki/search.cc
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ struct bin_handler_t {
auto opposing_edge_id = reader.GetOpposingEdgeId(candidate.edge_id, other_edge, other_tile);

if (other_edge && costing->Allowed(other_edge, other_tile)) {
auto reach = get_reach(opposing_edge_id, other_edge);
Copy link
Member

Choose a reason for hiding this comment

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

hahaha oops! thanks for removing the shadowing!

reach = get_reach(opposing_edge_id, other_edge);
PathLocation::PathEdge other_path_edge{opposing_edge_id, 1 - length_ratio, candidate.point,
distance, flip_side(side), reach.outbound,
reach.inbound};
Expand Down
220 changes: 161 additions & 59 deletions src/thor/route_action.cc

Large diffs are not rendered by default.

51 changes: 51 additions & 0 deletions test/gurka/test_reachability.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#include "gurka.h"
Copy link
Member

Choose a reason for hiding this comment

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

nit: there is already a test_reach and a test_route gurka test, maybe this should just go in one of those?

#include <gtest/gtest.h>

using namespace valhalla;

TEST(Reachability, dont_snap_waypoints_to_deadends) {
const std::string ascii_map = R"(
A--1-B-2--C
/ \
D E)";

// BD and BE are oneways that lead away, and toward the main road A-B-C
const gurka::ways ways = {{"AB", {{"highway", "primary"}}},
{"BC", {{"highway", "primary"}}},
{"BD", {{"highway", "secondary"}, {"oneway", "yes"}}},
{"BE", {{"highway", "secondary"}, {"oneway", "yes"}}}};
const auto layout = gurka::detail::map_to_coordinates(ascii_map, 25);

std::unordered_map<std::string, std::string> config_opts = {
{"mjolnir.concurrency", "1"},
{"loki.service_defaults.minimum_reachability", "3"},
};

auto map = gurka::buildtiles(layout, ways, {}, {}, "test/data/reachability", config_opts);

{
// Verify simple waypoint routing works
auto result1 = gurka::do_action(valhalla::Options::route, map, {"A", "1", "C"}, "auto", {}, {},
nullptr, "break_through");
gurka::assert::raw::expect_path(result1, {"AB", "AB", "AB", "BC"});
auto result2 = gurka::do_action(valhalla::Options::route, map, {"A", "2", "C"}, "auto", {}, {},
nullptr, "break_through");
gurka::assert::raw::expect_path(result2, {"AB", "BC", "BC"});
}
{
// BD is a oneway leading away from ABC with no escape
// input coordinate at D should get snapped to the ABC way
// Should give the same result as A->1->C
auto result = gurka::do_action(valhalla::Options::route, map, {"A", "D", "C"}, "auto", {}, {},
nullptr, "break_through");
gurka::assert::raw::expect_path(result, {"AB", "AB", "AB", "BC"});
Copy link
Member

Choose a reason for hiding this comment

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

why are there three AB edges? im sure there is some detail i just cant remember at the moment... i get why there is one from A to the snap point at or near 1 and that then there is another for the next leg from that point to B but i cant figure out where the 3rd one is coming from at the moment. can you jog my memory?

Copy link
Contributor Author

@genadz genadz Mar 12, 2021

Choose a reason for hiding this comment

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

ha, great question!) actually, the first AB edge is reversed. I think there is a small bug in TimeDepForward algorithm. I debugged it, there is a combination of conditions in TimeDepForward::SetOrigin method that led to this:

  1. looks like we should remove all inbound edges in case of the origin was snapped to the node, but we didn't do ti because of this https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L432 : origin and destination is on the same edge;
  2. when we got forward path edge AB we found that the destination also on this edge, and added forward AB edge to the priority queue with cost equal to A -> 1, BUT didn't settle this edge because of this comment https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L530 ;
  3. last step, we have edges ABr and ABf in the queue and sortcost for ABr edge is less than sortcost for ABf. So, pop edge ABr from the queue, start expansion from the node A and found destination edge ABf. The end)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, this should be fixed in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

interesting, we have a bit of code that is supposed to do this if i can remember where it is. it should say somethign like if its node snapped and its an origin no point in using the edge that is snapped to its end node...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

the purpose is to ignore end node snaps for origins of the route unless that is all we have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L432 - we also use this condition that just checks that origin and destination on the same edge. so , probably it's needless

}
{
// BE is a oneway leading towards ABC with no way in
// input coordinate at E should get snapped to the ABC way
// Should give the same result as A->2->C
auto result = gurka::do_action(valhalla::Options::route, map, {"A", "E", "C"}, "auto", {}, {},
nullptr, "break_through");
gurka::assert::raw::expect_path(result, {"AB", "BC", "BC"});
}
}
3 changes: 3 additions & 0 deletions test_requests/through_routes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@
-j '{"costing":"pedestrian","locations":[{"lon":-122.42256,"heading":81,"lat":37.75679,"type":"break"},{"lat":37.75863,"lon":-122.41907,"heading":175,"type":"through"},{"lat":37.75543,"lon":-122.41877,"type":"break"}]}'
-j '{"costing":"auto","locations":[{"lon":-122.42256,"heading":81,"lat":37.75679,"type":"break"},{"lat":37.75863,"lon":-122.41907,"heading":175,"type":"through"},{"lat":37.75543,"lon":-122.41877,"type":"break"}]}'
-j '{"costing":"auto","locations":[{"lon":-122.42256,"heading":81,"lat":37.75679,"type":"break"},{"lat":37.75863,"lon":-122.41907,"heading":174,"heading_tolerance":60,"type":"through"},{"lat":37.75543,"lon":-122.41877,"type":"break"}]}'
-j '{"costing":"auto","locations":[{"lat":38.245509,"lon":-85.75095,"type":"break"},{"lat":38.251,"lon":-85.763,"type":"through"},{"lat":38.258075,"lon":-85.744407,"type":"break"}]}'
-j '{"costing":"auto","locations":[{"lat":52.027886,"lon":5.077448,"type":"break"},{"lat":52.030559,"lon":5.078592,"type":"through"},{"lat":52.030178,"lon":5.080847,"type":"through"},{"lat":52.025977,"lon":5.08614,"type":"break"}]}'
-j '{"costing":"auto","locations":[{"lat":39.821226,"lon":-84.317642,"type":"break"},{"lat":39.828069,"lon":-84.314242,"type":"through"},{"lat":39.832658,"lon":-84.318762,"type":"through"},{"lat":39.828843,"lon":-84.323137,"type":"break"}]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test routes that fail right now

7 changes: 7 additions & 0 deletions valhalla/baldr/pathlocation.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ struct PathLocation : public Location {
*/
bool shares_edges(const PathLocation& other) const;

/**
* Check if path edge has high reachability for this location.
* @param edge PathEdge that should be checked
* @return true if the edge is high reachable
*/
bool is_high_reachable(const PathEdge& edge) const;
Copy link
Member

Choose a reason for hiding this comment

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

seems this method is actually unused. we instead use the free function inside thor route_action

Copy link
Contributor Author

@genadz genadz Mar 11, 2021

Choose a reason for hiding this comment

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

will remove, thanks


static void toPBF(const PathLocation& pl, valhalla::Location* l, baldr::GraphReader& reader) {
l->mutable_ll()->set_lng(pl.latlng_.first);
l->mutable_ll()->set_lat(pl.latlng_.second);
Expand Down
2 changes: 1 addition & 1 deletion valhalla/loki/search.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace loki {
* @param locations the positions which need to be correlated to the route network
* @param reader and object used to access tiled route data TODO: switch this out for a
* proper cache
* @param edge_filter a costing object by which we can determine which portions of the graph are
* @param costing a costing object by which we can determine which portions of the graph are
* accessable and therefor potential candidates
* @return pathLocations the correlated data with in the tile that matches the inputs. If a
* projection is not found, it will not have any entry in the returned value.
Expand Down