Update name_id/name checks for empty IDs and empty strings

features/guidance/turn.feature

@@ -1372,3 +1372,64 @@ Feature: Simple Turns
             | waypoints | route        | turns                           |
             | a,d       | ab,bcd,bcd   | depart,fork slight right,arrive |
             | a,g       | ab,befg,befg | depart,fork slight left,arrive  |
+
+    # https://www.openstreetmap.org/#map=18/52.25130/10.42545
+    Scenario: Turn for roads with no name, ref changes
+        Given the node map
+            """
+              d
+              .
+              .
+            e c . . f
+              .
+              .
+              b
+              .
+              .
+              a
+            """
+
+        And the ways
+            | nodes | highway     | ref  | name          |
+            | abc   | tertiary    | K 57 |               |
+            | cd    | tertiary    | K 56 |               |
+            | cf    | tertiary    | K 56 |               |
+            | ce    | residential |      | Heinrichshöhe |
+
+       When I route I should get
+            | waypoints | route     | turns                    |
+            | a,f       | ,,        | depart,turn right,arrive |
+
+    # https://www.openstreetmap.org/#map=18/52.24071/10.29066
+    Scenario: Turn for roads with no name, ref changes
+        Given the node map
+            """
+              x
+              .
+              .
+              d
+             . .
+            .   .
+           .     .
+     e. . t . c . p. .f
+           .     .
+            .   .
+             . .
+              b
+              .
+              .
+              a
+            """
+
+        And the ways
+            | nodes | highway     | ref  | name          | oneway |
+            | abp   | tertiary    | K 23 |               | yes    |
+            | pdx   | tertiary    | K 23 |               | yes    |
+            | xdt   | tertiary    | K 23 |               | yes    |
+            | tba   | tertiary    | K 23 |               | yes    |
+            | etcpf | primary     | B 1  |               | no     |
+
+       When I route I should get
+            | waypoints | route     | turns                                    |
+            | e,x       | ,,,       | depart,turn sharp left,turn right,arrive |
+            | f,a       | ,,        | depart,turn left,arrive                  |

include/engine/api/route_api.hpp

@@ -173,7 +173,7 @@ class RouteAPI : public BaseAPI
 
                 guidance::trimShortSegments(steps, leg_geometry);
                 leg.steps = guidance::handleRoundabouts(std::move(steps));
-                leg.steps = guidance::collapseTurnInstructions(std::move(leg.steps));
+                leg.steps = guidance::collapseTurnInstructions(std::move(leg.steps), facade);
                 leg.steps = guidance::anticipateLaneChange(std::move(leg.steps));
                 leg.steps = guidance::buildIntersections(std::move(leg.steps));
                 leg.steps = guidance::suppressShortNameSegments(std::move(leg.steps));

include/engine/guidance/collapse_turns.hpp

@@ -1,5 +1,6 @@
 #ifndef OSRM_ENGINE_GUIDANCE_COLLAPSE_HPP
 
+#include "engine/datafacade/datafacade_base.hpp"
 #include "engine/guidance/route_step.hpp"
 #include "util/attributes.hpp"
 
@@ -19,7 +20,8 @@ namespace guidance
 // Collapsing such turns into a single turn instruction, we give a clearer
 // set of instructionst that is not cluttered by unnecessary turns/name changes.
 OSRM_ATTR_WARN_UNUSED
-std::vector<RouteStep> collapseTurnInstructions(std::vector<RouteStep> steps);
+std::vector<RouteStep> collapseTurnInstructions(std::vector<RouteStep> steps,
+                                                const datafacade::BaseDataFacade &facade);
 
 // A combined turn is a set of two instructions that actually form a single turn, as far as we
 // perceive it. A u-turn consisting of two left turns is one such example. But there are also lots

src/engine/guidance/collapse_scenario_detection.cpp

@@ -41,15 +41,14 @@ bool noIntermediaryIntersections(const RouteStep &step)
 }
 
 // Link roads, as far as we are concerned, are short unnamed segments between to named segments.
-bool isLinkroad(const RouteStep &pre_link_step,
-                const RouteStep &link_step,
-                const RouteStep &post_link_step)
+bool isLinkRoad(const RouteStep &link_step,
+                const std::string &pre_link_step_name,
+                const std::string &post_link_step_name)
 {
     const constexpr double MAX_LINK_ROAD_LENGTH = 2 * MAX_COLLAPSE_DISTANCE;
     const auto is_short = link_step.distance <= MAX_LINK_ROAD_LENGTH;
-    const auto unnamed = link_step.name_id == EMPTY_NAMEID;
-    const auto between_named =
-        (pre_link_step.name_id != EMPTY_NAMEID) && (post_link_step.name_id != EMPTY_NAMEID);
+    const auto unnamed = link_step.name.empty();
+    const auto between_named = !pre_link_step_name.empty() && !post_link_step_name.empty();
 
     return is_short && unnamed && between_named && noIntermediaryIntersections(link_step);
 }
@@ -164,6 +163,9 @@ bool isStaggeredIntersection(const RouteStepIterator step_prior_to_intersection,
 bool isUTurn(const RouteStepIterator step_prior_to_intersection,
              const RouteStepIterator step_entering_intersection,
              const RouteStepIterator step_leaving_intersection)
+//             const std::string &step_prior_name,
+//             const std::string &step_entering_name,
+//             const std::string &step_leaving_name)
 {
     if (!basicCollapsePreconditions(
             step_prior_to_intersection, step_entering_intersection, step_leaving_intersection))
@@ -196,9 +198,9 @@ bool isUTurn(const RouteStepIterator step_prior_to_intersection,
     const auto only_allowed_turn = (numberOfAllowedTurns(*step_leaving_intersection) == 1) &&
                                    noIntermediaryIntersections(*step_entering_intersection);
 
-    return collapsable || isLinkroad(*step_prior_to_intersection,
-                                     *step_entering_intersection,
-                                     *step_leaving_intersection) ||
+    return collapsable || isLinkRoad(*step_entering_intersection,
+                                     step_prior_to_intersection->name,
+                                     step_leaving_intersection->name) ||
            only_allowed_turn;
 }
 

src/engine/guidance/collapse_turns.cpp

@@ -326,7 +326,7 @@ void suppressStep(RouteStep &step_at_turn_location, RouteStep &step_after_turn_l
 
 // OTHER IMPLEMENTATIONS
 OSRM_ATTR_WARN_UNUSED
-RouteSteps collapseTurnInstructions(RouteSteps steps)
+RouteSteps collapseTurnInstructions(RouteSteps steps, const datafacade::BaseDataFacade &facade)
 {
     // make sure we can safely iterate over all steps (has depart/arrive with TurnType::NoTurn)
     BOOST_ASSERT(!hasTurnType(steps.front()) && !hasTurnType(steps.back()));
@@ -463,6 +463,7 @@ RouteSteps collapseTurnInstructions(RouteSteps steps)
         if (!hasWaypointType(*previous_step))
         {
             const auto far_back_step = findPreviousTurn(previous_step);
+            const auto far_back_step_name = facade.GetNameForID(far_back_step->name_id).to_string();
             // due to name changes, we can find u-turns a bit late. Thats why we check far back as
             // well
             if (isUTurn(far_back_step, previous_step, current_step))

src/extractor/guidance/intersection_handler.cpp

@@ -71,17 +71,19 @@ TurnType::Enum IntersectionHandler::findBasicTurnType(const EdgeID via_edge,
     if (!on_ramp && onto_ramp)
         return TurnType::OnRamp;
 
-    const auto &in_name =
+    const auto &in_name_id =
         node_data_container.GetAnnotation(node_based_graph.GetEdgeData(via_edge).annotation_data)
             .name_id;
-    const auto &out_name =
+    const auto &out_name_id =
         node_data_container.GetAnnotation(node_based_graph.GetEdgeData(road.eid).annotation_data)
             .name_id;
+    const auto &in_name = name_table.GetNameForID(in_name_id).to_string();
+    const auto &out_name = name_table.GetNameForID(out_name_id).to_string();
 
     const auto same_name = !util::guidance::requiresNameAnnounced(
-        in_name, out_name, name_table, street_name_suffix_table);
+        in_name_id, out_name_id, name_table, street_name_suffix_table);
 
-    if (in_name != EMPTY_NAMEID && out_name != EMPTY_NAMEID && same_name)
+    if (!in_name.empty() && !out_name.empty() && same_name)
     {
         return TurnType::Continue;
     }

src/extractor/guidance/mergable_road_detector.cpp

@@ -32,15 +32,17 @@ inline auto makeCheckRoadForName(const NameID name_id,
     return [name_id, &node_based_graph, &node_data_container, &name_table, &suffix_table](
         const MergableRoadDetector::MergableRoadData &road) {
         // since we filter here, we don't want any other name than the one we are looking for
-        const auto road_name =
+        const auto road_name_id =
             node_data_container
                 .GetAnnotation(node_based_graph.GetEdgeData(road.eid).annotation_data)
                 .name_id;
-        if (name_id == EMPTY_NAMEID || road_name == EMPTY_NAMEID)
+        const auto road_name = name_table.GetNameForID(road_name_id).to_string();
+        if (name_id == EMPTY_NAMEID || road_name.empty())
             return true;
         const auto requires_announcement =
-            util::guidance::requiresNameAnnounced(name_id, road_name, name_table, suffix_table) ||
-            util::guidance::requiresNameAnnounced(road_name, name_id, name_table, suffix_table);
+            util::guidance::requiresNameAnnounced(
+                name_id, road_name_id, name_table, suffix_table) ||
+            util::guidance::requiresNameAnnounced(road_name_id, name_id, name_table, suffix_table);
 
         return requires_announcement;
     };
@@ -465,16 +467,19 @@ bool MergableRoadDetector::IsTrafficIsland(const NodeID intersection_node,
                 .name_id;
 
         const auto has_required_name = [this, required_name_id](const auto edge_id) {
-            const auto road_name =
+            const auto road_name_id =
                 node_data_container
                     .GetAnnotation(node_based_graph.GetEdgeData(edge_id).annotation_data)
                     .name_id;
-            if (required_name_id == EMPTY_NAMEID || road_name == EMPTY_NAMEID)
+            const auto &road_name = name_table.GetNameForID(road_name_id).to_string();
+            const auto &required_name = name_table.GetNameForID(required_name_id).to_string();
+            if (required_name_id == EMPTY_NAMEID || road_name_id == EMPTY_NAMEID ||
+                (required_name.empty() && road_name.empty()))
                 return false;
             return !util::guidance::requiresNameAnnounced(
-                       required_name_id, road_name, name_table, street_name_suffix_table) ||
+                       required_name_id, road_name_id, name_table, street_name_suffix_table) ||
                    !util::guidance::requiresNameAnnounced(
-                       road_name, required_name_id, name_table, street_name_suffix_table);
+                       road_name_id, required_name_id, name_table, street_name_suffix_table);
         };
 
         /* the beautiful way would be:

src/extractor/guidance/motorway_handler.cpp

@@ -379,11 +379,15 @@ Intersection MotorwayHandler::fromRamp(const EdgeID via_eid, Intersection inters
             //
             //     7         1
             //          0
+            const auto &first_intersection_name =
+                name_table.GetNameForID(first_intersection_data.name_id).to_string();
+            const auto &second_intersection_name =
+                name_table.GetNameForID(second_intersection_data.name_id).to_string();
             if (intersection[1].entry_allowed)
             {
                 if (isMotorwayClass(intersection[1].eid, node_based_graph) &&
-                    second_intersection_data.name_id != EMPTY_NAMEID &&
-                    first_intersection_data.name_id != EMPTY_NAMEID && first_second_same_name)
+                    !second_intersection_name.empty() && !first_intersection_name.empty() &&
+                    first_second_same_name)
                 {
                     // circular order indicates a merge to the left (0-3 onto 4
                     if (angularDeviation(intersection[1].angle, STRAIGHT_ANGLE) <
@@ -407,8 +411,8 @@ Intersection MotorwayHandler::fromRamp(const EdgeID via_eid, Intersection inters
             {
                 BOOST_ASSERT(intersection[2].entry_allowed);
                 if (isMotorwayClass(intersection[2].eid, node_based_graph) &&
-                    second_intersection_data.name_id != EMPTY_NAMEID &&
-                    first_intersection_data.name_id != EMPTY_NAMEID && first_second_same_name)
+                    !second_intersection_name.empty() && !first_intersection_name.empty() &&
+                    first_second_same_name)
                 {
                     // circular order (5-0) onto 4
                     if (angularDeviation(intersection[2].angle, STRAIGHT_ANGLE) <

src/extractor/guidance/roundabout_handler.cpp

@@ -295,7 +295,8 @@ RoundaboutType RoundaboutHandler::getRoundaboutType(const NodeID nid) const
                     return SPECIAL_EDGEID;
                 }
 
-                if (EMPTY_NAMEID != edge_data.name_id)
+                const auto &edge_name = name_table.GetNameForID(edge_data.name_id).to_string();
+                if (!edge_name.empty())
                 {
 
                     const auto announce = [&](unsigned id) {

src/extractor/guidance/sliproad_handler.cpp

@@ -474,7 +474,8 @@ operator()(const NodeID /*nid*/, const EdgeID source_edge_id, Intersection inter
 
             // Name mismatch: check roads at `c` and `d` for same name
             const auto name_mismatch = [&](const NameID road_name_id) {
-                const auto unnamed = road_name_id == EMPTY_NAMEID;
+                const auto &road_name = name_table.GetNameForID(road_name_id).to_string();
+                const auto unnamed = road_name.empty();
 
                 return unnamed ||
                        util::guidance::requiresNameAnnounced(road_name_id,              //
@@ -501,9 +502,13 @@ operator()(const NodeID /*nid*/, const EdgeID source_edge_id, Intersection inter
                     .name_id;
             const auto &sliproad_annotation =
                 node_data_container.GetAnnotation(sliproad_edge_data.annotation_data);
+            const auto &sliproad_name =
+                name_table.GetNameForID(sliproad_annotation.name_id).to_string();
+            const auto &main_road_name = name_table.GetNameForID(main_road_name_id).to_string();
+            const auto &candidate_road_name =
+                name_table.GetNameForID(candidate_data.name_id).to_string();
             if (!sliproad_edge_data.flags.road_classification.IsLinkClass() &&
-                sliproad_annotation.name_id != EMPTY_NAMEID && main_road_name_id != EMPTY_NAMEID &&
-                candidate_data.name_id != EMPTY_NAMEID &&
+                !sliproad_name.empty() && !main_road_name.empty() && !candidate_road_name.empty() &&
                 util::guidance::requiresNameAnnounced(main_road_name_id,
                                                       sliproad_annotation.name_id,
                                                       name_table,

src/extractor/guidance/turn_handler.cpp

@@ -199,7 +199,8 @@ bool TurnHandler::isObviousOfTwo(const EdgeID via_edge,
 
     const bool turn_is_perfectly_straight =
         angularDeviation(road.angle, STRAIGHT_ANGLE) < std::numeric_limits<double>::epsilon();
-    if (via_data.name_id != EMPTY_NAMEID)
+    const auto &via_name = name_table.GetNameForID(via_data.name_id).to_string();
+    if (!via_name.empty())
     {
         const auto same_name = !util::guidance::requiresNameAnnounced(
             via_data.name_id, road_data.name_id, name_table, street_name_suffix_table);