From 884ce4025b170de57b3edf92d68b4ab433eb836b Mon Sep 17 00:00:00 2001 From: Moritz Kobitzsch Date: Thu, 17 Aug 2017 15:31:13 +0200 Subject: [PATCH] fix detection of suffix/prefix changes for name-changes --- CHANGELOG.md | 2 + features/guidance/continue.feature | 34 +++++ features/guidance/new-name.feature | 8 +- include/util/guidance/name_announcements.hpp | 138 ++++++++++++------- profiles/car.lua | 4 +- 5 files changed, 130 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd4c11ba1..6c424df24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ - Guidance - Fixed some cases of sliproads pre-processing (https://github.com/Project-OSRM/osrm-backend/issues/4348) - don't suppress name announcements via sliproad handler + - Bugfixes + - Fixed a bug that would result in unnecessary instructions, due to problems in suffix/prefix detection # 5.12.0 - Changes from 5.11: diff --git a/features/guidance/continue.feature b/features/guidance/continue.feature index 249a34aad..86ac2c3fe 100644 --- a/features/guidance/continue.feature +++ b/features/guidance/continue.feature @@ -22,6 +22,40 @@ Feature: Continue Instructions | a,c | abc,abc,abc | depart,continue left,arrive | | a,d | abc,bd,bd | depart,turn straight,arrive | + Scenario: Road turning left, Suffix changes + Given the node map + """ + c + a - b-d + """ + + And the ways + | nodes | highway | name | + | ab | primary | North Capitol Northeast | + | bc | primary | North Capitol Northwest | + | bd | primary | some random street | + + When I route I should get + | waypoints | route | turns | + | a,c | North Capitol Northeast,North Capitol Northwest,North Capitol Northwest | depart,continue left,arrive | + + Scenario: Road turning left, Suffix changes, no-spaces + Given the node map + """ + c + a - b-d + """ + + And the ways + | nodes | highway | name | + | ab | primary | North CapitolNortheast | + | bc | primary | North CapitolNorthwest | + | bd | primary | some random street | + + When I route I should get + | waypoints | route | turns | + | a,c | North CapitolNortheast,North CapitolNorthwest,North CapitolNorthwest | depart,continue left,arrive | + Scenario: Road turning left and straight Given the node map """ diff --git a/features/guidance/new-name.feature b/features/guidance/new-name.feature index 65aa38690..9ab342eef 100644 --- a/features/guidance/new-name.feature +++ b/features/guidance/new-name.feature @@ -274,8 +274,8 @@ Feature: New-Name Instructions | bc | Central Expressway | US 75 | motorway | When I route I should get - | waypoints | route | turns | - | a,c | North Central Expressway,Central Expressway,Central Expressway | depart,new name straight,arrive | + | waypoints | route | turns | + | a,c | North Central Expressway,Central Expressway | depart,arrive | Scenario: Prefix Change Given the node map @@ -289,8 +289,8 @@ Feature: New-Name Instructions | cb | Central Expressway | US 75 | motorway | When I route I should get - | waypoints | route | turns | - | c,a | Central Expressway,North Central Expressway,North Central Expressway | depart,new name straight,arrive | + | waypoints | route | turns | + | c,a | Central Expressway,North Central Expressway | depart,arrive | Scenario: No Name, Same Reference Given the node map diff --git a/include/util/guidance/name_announcements.hpp b/include/util/guidance/name_announcements.hpp index 15c738e2a..2c3c3338a 100644 --- a/include/util/guidance/name_announcements.hpp +++ b/include/util/guidance/name_announcements.hpp @@ -10,6 +10,7 @@ #include #include +#include #include #include @@ -25,17 +26,81 @@ namespace guidance // Name Change Logic // Used both during Extraction as well as during Post-Processing -inline std::pair getPrefixAndSuffix(const std::string &data) +inline std::string longest_common_substring(const std::string &lhs, const std::string &rhs) { - const auto suffix_pos = data.find_last_of(' '); - if (suffix_pos == std::string::npos) - return {}; + if (lhs.empty() || rhs.empty()) + return ""; - const auto prefix_pos = data.find_first_of(' '); - auto result = std::make_pair(data.substr(0, prefix_pos), data.substr(suffix_pos + 1)); - boost::to_lower(result.first); - boost::to_lower(result.second); - return result; + // array for dynamic programming + std::vector> dp(lhs.size(), + std::vector(rhs.size(), 0)); + + // to remember the best location + std::uint32_t best = 0; + std::uint32_t best_pos = 0; + + for (std::uint32_t i = 0; i < lhs.size(); ++i) + { + for (std::uint32_t j = 0; j < rhs.size(); ++j) + { + if (lhs[i] == rhs[j]) + { + dp[i][j] = (i == 0 || j == 0) ? 1 : (dp[i - 1][j - 1] + 1); + if (dp[i][j] > best) + { + best = dp[i][j]; + best_pos = i + 1; + } + } + } + } + // the best position marks the end of the string + return lhs.substr(best_pos - best, best); +} + +inline auto decompose(const std::string &lhs, const std::string &rhs) +{ + auto const lcs = longest_common_substring(lhs, rhs); + + // trim spaces, transform to lower + const auto trim = [](auto str) { + boost::to_lower(str); + auto front = str.find_first_not_of(" "); + + if (front == std::string::npos) + return str; + + auto back = str.find_last_not_of(" "); + return str.substr(front, back - front + 1); + }; + + if (lcs.empty()) + { + std::string empty = ""; + return std::make_tuple(trim(lhs), trim(rhs), empty, empty); + } + + // find the common substring in both + auto lhs_pos = lhs.find(lcs); + auto rhs_pos = rhs.find(lcs); + + BOOST_ASSERT(lhs_pos + lcs.size() <= lhs.size()); + BOOST_ASSERT(rhs_pos + lcs.size() <= rhs.size()); + + // prefixes + std::string lhs_prefix = (lhs_pos > 0) ? lhs.substr(0, lhs_pos) : ""; + std::string rhs_prefix = (rhs_pos > 0) ? rhs.substr(0, rhs_pos) : ""; + + // suffices + std::string lhs_suffix = lhs.substr(lhs_pos + lcs.size()); + std::string rhs_suffix = rhs.substr(rhs_pos + lcs.size()); + + lhs_prefix = trim(std::move(lhs_prefix)); + lhs_suffix = trim(std::move(lhs_suffix)); + rhs_prefix = trim(std::move(rhs_prefix)); + rhs_suffix = trim(std::move(rhs_suffix)); + + return std::make_tuple(lhs_prefix, lhs_suffix, rhs_prefix, rhs_suffix); } // Note: there is an overload without suffix checking below. @@ -64,51 +129,24 @@ inline bool requiresNameAnnounced(const std::string &from_name, const auto name_is_contained = boost::starts_with(from_name, to_name) || boost::starts_with(to_name, from_name); - const auto checkForPrefixOrSuffixChange = []( - const std::string &first, const std::string &second, const SuffixTable &suffix_table) { + const auto checkForPrefixOrSuffixChange = + [](const std::string &first, const std::string &second, const SuffixTable &suffix_table) { + std::string first_prefix, first_suffix, second_prefix, second_suffix; + std::tie(first_prefix, first_suffix, second_prefix, second_suffix) = + decompose(first, second); - const auto first_prefix_and_suffixes = getPrefixAndSuffix(first); - const auto second_prefix_and_suffixes = getPrefixAndSuffix(second); + const auto checkTable = [&](const std::string &str) { + // workaround for cucumber tests: + if (str.length() == 1 && (first.length() == 2 || second.length() == 2)) + return false; - // reverse strings, get suffices and reverse them to get prefixes - const auto checkTable = [&](const std::string &str) { - return str.empty() || suffix_table.isSuffix(str); + return str.empty() || suffix_table.isSuffix(str); + }; + + return checkTable(first_prefix) && checkTable(first_suffix) && + checkTable(second_prefix) && checkTable(second_suffix); }; - const auto getOffset = [](const std::string &str) -> std::size_t { - if (str.empty()) - return 0; - else - return str.length() + 1; - }; - - const bool is_prefix_change = [&]() -> bool { - if (!checkTable(first_prefix_and_suffixes.first)) - return false; - if (!checkTable(second_prefix_and_suffixes.first)) - return false; - return !first.compare(getOffset(first_prefix_and_suffixes.first), - std::string::npos, - second, - getOffset(second_prefix_and_suffixes.first), - std::string::npos); - }(); - - const bool is_suffix_change = [&]() -> bool { - if (!checkTable(first_prefix_and_suffixes.second)) - return false; - if (!checkTable(second_prefix_and_suffixes.second)) - return false; - return !first.compare(0, - first.length() - getOffset(first_prefix_and_suffixes.second), - second, - 0, - second.length() - getOffset(second_prefix_and_suffixes.second)); - }(); - - return is_prefix_change || is_suffix_change; - }; - const auto is_suffix_change = checkForPrefixOrSuffixChange(from_name, to_name, suffix_table); const auto names_are_equal = from_name == to_name || name_is_contained || is_suffix_change; const auto name_is_removed = !from_name.empty() && to_name.empty(); diff --git a/profiles/car.lua b/profiles/car.lua index d10afae0d..d9ba3edfe 100644 --- a/profiles/car.lua +++ b/profiles/car.lua @@ -34,9 +34,9 @@ function setup() speed_reduction = 0.8, turn_bias = 1.075, - -- a list of suffixes to suppress in name change instructions + -- a list of suffixes to suppress in name change instructions. The suffixes also include common substrings of each other suffix_list = { - 'N', 'NE', 'E', 'SE', 'S', 'SW', 'W', 'NW', 'North', 'South', 'West', 'East' + 'N', 'NE', 'E', 'SE', 'S', 'SW', 'W', 'NW', 'North', 'South', 'West', 'East', 'Nor', 'Sou', 'We', 'Ea' }, barrier_whitelist = Set {