Skip to content

Commit

Permalink
Fix ferry shortest path recovery (#4378)
Browse files Browse the repository at this point in the history 
* Add test that demonstrates the wrong path can be recovered from the
ShortestPath method in ferry_connections.cc

* Fix the last_label_idx - make sure it is only set when an edge with the
target classification (or better) is present at the node being expanded.
Identify cases where no reclassification occurs (e.g,. islands with no
edges that meet the target classification) and log the locations.

* Add to change log

* If no path is found, always add to set of ferry locations with no path.

* Add test to make sure no edges are reclassified if the target
classification is not found.

* Looks like ReclassifyFerryConnections automatically sets the first edge
connecting to the ferry to primary. I added a test to make
sure no edges are reclassified if no edges with the target
classification are encountered. However, the initial ferry connection
edge is set to primary even if no edges with the target classification
are encountered in ShortestPath.

* Formatting

* remove some method declarations - not needed since only used in
ferry_connections.cc.

* More properly identify cases where no paths to/from ferries to target
classification roadways exist.

* formatting

* don't need to include unordered_set anymore.

* Remove the check if node_labels.size == 0. This is redundant now that we
check for a valid last_label_idx being set.

* Reclassify the first edge IF a connection to higher class roads is
found.

* Modify the nothing reclassified test to remove the first edge connected
to the ferry. No longer upclassed if no path to higher class edges is
found.
  • Loading branch information
@dnesbitt61
dnesbitt61 committed Nov 15, 2023
1 parent 83f1c95 commit 274a3e7
Show file tree
Hide file tree
Showing 4 changed files with 201 additions and 65 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* FIXED: arm builds. tons of errors due to floating point issues mostly [#4213](https://github.com/valhalla/valhalla/pull/4213)
* FIXED: respond with correlated edges for format=valhalla and matrix [#4335](https://github.com/valhalla/valhalla/pull/4335)
* FIXED: recover proper shortest path to ferry connections (when multiple edges exist between node pair) [#4361](https://github.com/valhalla/valhalla/pull/4361)
* FIXED: recover proper shortest path to ferry connections (make sure correct label index is used) [#4378](https://github.com/valhalla/valhalla/pull/4378)
* **Enhancement**
* UPDATED: French translations, thanks to @xlqian [#4159](https://github.com/valhalla/valhalla/pull/4159)
* CHANGED: -j flag for multithreaded executables to override mjolnir.concurrency [#4168](https://github.com/valhalla/valhalla/pull/4168)
Expand Down
109 changes: 77 additions & 32 deletions src/mjolnir/ferry_connections.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
namespace valhalla {
namespace mjolnir {

// Invalid index
const uint32_t kInvalidIndex = std::numeric_limits<uint32_t>::max();

// Get the best classification for any drivable non-ferry and non-link
// edges from a node. Skip any reclassified ferry edges
uint32_t GetBestNonFerryClass(const std::map<Edge, size_t>& edges) {
Expand Down Expand Up @@ -35,14 +38,14 @@ class CompareCost {

// Form the shortest path from the start node until a node that
// touches the specified road classification.
uint32_t ShortestPath(const uint32_t start_node_idx,
const uint32_t node_idx,
sequence<OSMWay>& ways,
sequence<OSMWayNode>& way_nodes,
sequence<Edge>& edges,
sequence<Node>& nodes,
const bool inbound,
const bool first_edge_destonly) {
std::pair<uint32_t, bool> ShortestPath(const uint32_t start_node_idx,
const uint32_t node_idx,
sequence<OSMWay>& ways,
sequence<OSMWayNode>& way_nodes,
sequence<Edge>& edges,
sequence<Node>& nodes,
const bool inbound,
const bool first_edge_destonly) {
// Method to get the shape for an edge - since LL is stored as a pair of
// floats we need to change into PointLL to get length of an edge
const auto EdgeShape = [&way_nodes](size_t idx, const size_t count) {
Expand Down Expand Up @@ -91,7 +94,7 @@ uint32_t ShortestPath(const uint32_t start_node_idx,
// is reached
uint32_t n = 0;
uint32_t label_idx = 0;
uint32_t last_label_idx = 0;
uint32_t last_label_idx = kInvalidIndex;
while (!adjset.empty()) {
// Get the next node from the adjacency list/priority queue. Gets its
// current cost and index
Expand All @@ -116,8 +119,15 @@ uint32_t ShortestPath(const uint32_t start_node_idx,
// Have seen cases where the immediate connections are high class roads
// but then there are service roads (lanes) immediately after (like
// Twawwassen Terminal near Vancouver,BC)
if (n > 400 && GetBestNonFerryClass(expanded_bundle.node_edges) <= kFerryUpClass) {
break;
if (GetBestNonFerryClass(expanded_bundle.node_edges) <= kFerryUpClass) {
// Set the last label index - shortest path is recovered backwards from this
// label to the ferry start
last_label_idx = label_idx;

// TODO - better termination criteria?!
if (n > 400) {
break;
}
}
n++;

Expand Down Expand Up @@ -183,9 +193,6 @@ uint32_t ShortestPath(const uint32_t start_node_idx,
}
}

// Get the last label index to form the path
last_label_idx = nodelabel_index;

// Add to the node labels and adjacency set. Skip if this is a loop.
node_labels.emplace_back(cost, endnode, expand_node_idx, edge.wayindex_,
w.destination_only());
Expand All @@ -195,11 +202,9 @@ uint32_t ShortestPath(const uint32_t start_node_idx,
}
}

// If only one label we have immediately found an edge with proper
// classification - or we cannot expand due to driveability
if (node_labels.size() == 1) {
LOG_DEBUG("Only 1 edge reclassified");
return 0;
// Path not found to edge with proper classification
if (last_label_idx == kInvalidIndex) {
return std::make_pair(0, false);
}

// Trace shortest path backwards and upgrade edge classifications
Expand Down Expand Up @@ -246,7 +251,7 @@ uint32_t ShortestPath(const uint32_t start_node_idx,
overall_access_after |= path_access;
}

return edge_count;
return std::make_pair(edge_count, true);
}

// Check if the ferry included in this node bundle is short. Must be
Expand Down Expand Up @@ -329,6 +334,10 @@ void ReclassifyFerryConnections(const std::string& ways_file,
if (bundle.node.ferry_edge_ && bundle.node.non_ferry_edge_ &&
GetBestNonFerryClass(bundle.node_edges) > kFerryUpClass &&
!ShortFerry(node_itr.position(), bundle, edges, nodes, way_nodes)) {
bool inbound_path_found = false;
bool outbound_path_found = false;
PointLL ll = (*nodes[node_itr.position()]).node.latlng();

// Form shortest path from node along each edge connected to the ferry,
// track until the specified RC is reached
for (const auto& edge : bundle.node_edges) {
Expand Down Expand Up @@ -357,33 +366,69 @@ void ReclassifyFerryConnections(const std::string& ways_file,
if (edge_fwd_access == edge_rev_access) {
// drivable in both directions - get an inbound path and an
// outbound path.
total_count += ShortestPath(node_itr.position(), end_node_idx, ways, way_nodes, edges,
nodes, true, remove_destonly);
total_count += ShortestPath(node_itr.position(), end_node_idx, ways, way_nodes, edges,
nodes, false, remove_destonly);
auto ret1 = ShortestPath(node_itr.position(), end_node_idx, ways, way_nodes, edges, nodes,
true, remove_destonly);
total_count += ret1.first;
if (ret1.second) {
inbound_path_found = true;
}
auto ret2 = ShortestPath(node_itr.position(), end_node_idx, ways, way_nodes, edges, nodes,
false, remove_destonly);
total_count += ret2.first;
if (ret2.second) {
outbound_path_found = true;
}
} else {
// Check if oneway inbound to the ferry
bool inbound =
(edge.first.sourcenode_ == node_itr.position()) ? edge_rev_access : edge_fwd_access;
total_count += ShortestPath(node_itr.position(), end_node_idx, ways, way_nodes, edges,
nodes, inbound, remove_destonly);
auto ret = ShortestPath(node_itr.position(), end_node_idx, ways, way_nodes, edges, nodes,
inbound, remove_destonly);
total_count += ret.first;
if (ret.second > 0) {
if (inbound) {
inbound_path_found = true;
} else {
outbound_path_found = true;
}
}
}
ferry_endpoint_count++;

// Reclassify the first/start edge. Do this AFTER finding shortest path so
// we do not immediately determine we hit the specified classification
// Reclassify the first/start edge if a connection to higher class roads
// is found. Do this AFTER finding shortest path so we do not immediately
// determine we hit the specified classification
sequence<Edge>::iterator element = edges[edge.second];
auto update_edge = *element;
update_edge.attributes.importance = kFerryUpClass;
update_edge.attributes.reclass_ferry = remove_destonly;
element = update_edge;
total_count++;
if (inbound_path_found && outbound_path_found &&
update_edge.attributes.importance > kFerryUpClass) {
update_edge.attributes.importance = kFerryUpClass;
update_edge.attributes.reclass_ferry = remove_destonly;
element = update_edge;
total_count++;
}
}

// Log cases where reclassification fails
if (!inbound_path_found && !outbound_path_found) {
LOG_INFO("Reclassification fails both directions to ferry at LL =" +
std::to_string(ll.lat()) + "," + std::to_string(ll.lng()));
} else {
if (!inbound_path_found) {
LOG_INFO("Reclassification fails inbound to ferry at LL =" + std::to_string(ll.lat()) +
"," + std::to_string(ll.lng()));
}
if (!outbound_path_found) {
LOG_INFO("Reclassification fails outbound from ferry at LL =" + std::to_string(ll.lat()) +
"," + std::to_string(ll.lng()));
}
}
}

// Go to the next node
node_itr += bundle.node_count;
}

LOG_INFO("Finished ReclassifyFerryEdges: ferry_endpoint_count = " +
std::to_string(ferry_endpoint_count) + ", " + std::to_string(total_count) +
" edges reclassified.");
Expand Down
123 changes: 123 additions & 0 deletions test/gurka/test_ferry_connections.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,129 @@ TEST(Standalone, ReclassifyFerryNodePair) {
valhalla::baldr::RoadClass::kResidential);
}

TEST(Standalone, ReclassifyCorrectPath) {
// Test to validate that the shortest path between 2 edges between a node
// pair takes the shorter, higher class road (not the longer, lower class)

const std::string ascii_map = R"(
A--Z--B--C--D------E
| |
F--G J--K
| |
I--H L--M
)";

std::map<std::string, std::string> trunk = {{"highway", "trunk"}};
std::map<std::string, std::string> secondary = {{"highway", "secondary"}};
std::map<std::string, std::string> residential = {{"highway", "residential"}};
std::map<std::string, std::string> driveway = {{"highway", "service"}, {"service", "driveway"}};

const gurka::ways ways = {
{"AZ", trunk},
{"ZB", secondary},
{"BC", secondary},
{"CD", secondary},
{"BG", residential},
{"GH", residential},
{"GF", driveway},
{"HI", driveway},
{"CJ", residential},
{"JL", residential},
{"JK", driveway},
{"LM", driveway},
{"DE",
{{"motor_vehicle", "yes"},
{"motorcar", "yes"},
{"bicycle", "yes"},
{"moped", "yes"},
{"bus", "yes"},
{"hov", "yes"},
{"taxi", "yes"},
{"motorcycle", "yes"},
{"route", "ferry"}}},
};

const auto layout = gurka::detail::map_to_coordinates(ascii_map, 500);
auto map = gurka::buildtiles(layout, ways, {}, {}, "test/data/gurka_reclassify_correct_path");
baldr::GraphReader reader(map.config.get_child("mjolnir"));

// make sure ZB, BC and CD are upclassed
auto upclassed1 = gurka::findEdge(reader, layout, "ZB", "B");
EXPECT_TRUE(std::get<1>(upclassed1)->classification() == valhalla::baldr::RoadClass::kPrimary);
auto upclassed2 = gurka::findEdge(reader, layout, "BC", "C");
EXPECT_TRUE(std::get<1>(upclassed2)->classification() == valhalla::baldr::RoadClass::kPrimary);
auto upclassed3 = gurka::findEdge(reader, layout, "CD", "D");
EXPECT_TRUE(std::get<1>(upclassed3)->classification() == valhalla::baldr::RoadClass::kPrimary);

// make sure other edges are not upclassed
auto not_upclassed1 = gurka::findEdge(reader, layout, "BG", "G");
EXPECT_TRUE(std::get<1>(not_upclassed1)->classification() ==
valhalla::baldr::RoadClass::kResidential);
auto not_upclassed2 = gurka::findEdge(reader, layout, "GH", "H");
EXPECT_TRUE(std::get<1>(not_upclassed2)->classification() ==
valhalla::baldr::RoadClass::kResidential);
auto not_upclassed3 = gurka::findEdge(reader, layout, "CJ", "J");
EXPECT_TRUE(std::get<1>(not_upclassed3)->classification() ==
valhalla::baldr::RoadClass::kResidential);
auto not_upclassed4 = gurka::findEdge(reader, layout, "JL", "L");
EXPECT_TRUE(std::get<1>(not_upclassed4)->classification() ==
valhalla::baldr::RoadClass::kResidential);
auto not_upclassed5 = gurka::findEdge(reader, layout, "GF", "F");
EXPECT_TRUE(std::get<1>(not_upclassed5)->classification() ==
valhalla::baldr::RoadClass::kServiceOther);
auto not_upclassed6 = gurka::findEdge(reader, layout, "HI", "I");
EXPECT_TRUE(std::get<1>(not_upclassed6)->classification() ==
valhalla::baldr::RoadClass::kServiceOther);
auto not_upclassed7 = gurka::findEdge(reader, layout, "JK", "K");
EXPECT_TRUE(std::get<1>(not_upclassed7)->classification() ==
valhalla::baldr::RoadClass::kServiceOther);
auto not_upclassed8 = gurka::findEdge(reader, layout, "LM", "M");
EXPECT_TRUE(std::get<1>(not_upclassed8)->classification() ==
valhalla::baldr::RoadClass::kServiceOther);
}

TEST(Standalone, ReclassifyNothingReclassified) {
// Test to validate that if no edges are found with the target classification
// nothing gets reclassified.

const std::string ascii_map = R"(
A--B--C--D------E
)";

std::map<std::string, std::string> secondary = {{"highway", "secondary"}};
std::map<std::string, std::string> tertiary = {{"highway", "tertiary"}};

const gurka::ways ways = {
{"AB", secondary},
{"BC", secondary},
{"CD", tertiary},
{"DE",
{{"motor_vehicle", "yes"},
{"motorcar", "yes"},
{"bicycle", "yes"},
{"moped", "yes"},
{"bus", "yes"},
{"hov", "yes"},
{"taxi", "yes"},
{"motorcycle", "yes"},
{"route", "ferry"}}},
};

const auto layout = gurka::detail::map_to_coordinates(ascii_map, 500);
auto map = gurka::buildtiles(layout, ways, {}, {}, "test/data/gurka_reclassify_nothing");
baldr::GraphReader reader(map.config.get_child("mjolnir"));

// make sure no edges are upclassed
auto not_upclassed1 = gurka::findEdge(reader, layout, "AB", "B");
EXPECT_TRUE(std::get<1>(not_upclassed1)->classification() ==
valhalla::baldr::RoadClass::kSecondary);
auto not_upclassed2 = gurka::findEdge(reader, layout, "BC", "C");
EXPECT_TRUE(std::get<1>(not_upclassed2)->classification() ==
valhalla::baldr::RoadClass::kSecondary);
auto not_upclassed3 = gurka::findEdge(reader, layout, "CD", "D");
EXPECT_TRUE(std::get<1>(not_upclassed3)->classification() == valhalla::baldr::RoadClass::kTertiary);
}

INSTANTIATE_TEST_SUITE_P(FerryConnectionTest,
FerryTest,
::testing::Values("motorcar", "hgv", "moped", "motorcycle", "taxi", "bus"));
33 changes: 0 additions & 33 deletions valhalla/mjolnir/ferry_connections.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,39 +57,6 @@ struct NodeStatusInfo {
}
};

/**
* Get the best classification for any drivable non-ferry and non-link
* edges from a node. Skip any reclassified ferry edges
* @param edges The file backed list of edges in the graph.
* @return Returns the best (most important) classification
*/
uint32_t GetBestNonFerryClass(const std::map<Edge, size_t>& edges);

/**
* Form the shortest path from the start node until a node that
* touches the specified road classification.
*/
uint32_t ShortestPath(const uint32_t start_node_idx,
const uint32_t node_idx,
sequence<OSMWay>& ways,
sequence<OSMWayNode>& way_nodes,
sequence<Edge>& edges,
sequence<Node>& nodes,
const bool inbound,
const bool remove_dest_only);

/**
* Check if the ferry included in this node bundle is short. Must be
* just one edge and length < 2 km. This prevents forming connections
* to what are most likely river crossing ferries.
*/
bool ShortFerry(const uint32_t node_index,
node_bundle& bundle,
sequence<Edge>& edges,
sequence<Node>& nodes,
sequence<OSMWay>& ways,
sequence<OSMWayNode>& way_nodes);

/**
* Reclassify edges from a ferry along the shortest path to the
* specified road classification.
Expand Down

0 comments on commit 274a3e7

Please sign in to comment.