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.

Sign up for GitHub

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

Jump to bottom

Fix ferry shortest path recovery #4378

Merged
merged 16 commits into from
Nov 15, 2023
Merged

Fix ferry shortest path recovery #4378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2fdda12
Add test that demonstrates the wrong path can be recovered from the
dnesbitt61 Nov 2, 2023
095cfbe
Fix the last_label_idx - make sure it is only set when an edge with the
dnesbitt61 Nov 2, 2023
b69f490
Add to change log
dnesbitt61 Nov 2, 2023
6c07ab5
If no path is found, always add to set of ferry locations with no path.
dnesbitt61 Nov 2, 2023
8bb7f99
Add test to make sure no edges are reclassified if the target
dnesbitt61 Nov 2, 2023
1f06a9e
Looks like ReclassifyFerryConnections automatically sets the first edge
dnesbitt61 Nov 2, 2023
9bf4fc3
Formatting
dnesbitt61 Nov 3, 2023
615cd08
remove some method declarations - not needed since only used in
dnesbitt61 Nov 3, 2023
7431290
More properly identify cases where no paths to/from ferries to target
dnesbitt61 Nov 3, 2023
928d111
formatting
dnesbitt61 Nov 3, 2023
1007c6f
don't need to include unordered_set anymore.
dnesbitt61 Nov 3, 2023
a0bd237
Remove the check if node_labels.size == 0. This is redundant now that we
dnesbitt61 Nov 3, 2023
2f7bbbb
Merge branch 'master' into fix_ferry_shortestpath
dnesbitt61 Nov 7, 2023
4706189
Reclassify the first edge IF a connection to higher class roads is
dnesbitt61 Nov 7, 2023
19b65ae
Merge remote-tracking branch 'origin/fix_ferry_shortestpath' into fix…
dnesbitt61 Nov 7, 2023
6aae8c6
Modify the nothing reclassified test to remove the first edge connected
dnesbitt61 Nov 7, 2023
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
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading