Update name_id/name checks for empty IDs and empty strings #4684

daniel-j-h · 2017-11-14T14:07:43Z

Here's a failing cucumber scenario where we get a continue right when there are no name tags but the ref tag changes. My hunch is the underlying issue is #4642 and the continue type comes from:

osrm-backend/src/extractor/guidance/intersection_handler.cpp

Lines 81 to 89 in 2ae6361

    
           const auto same_name = !util::guidance::requiresNameAnnounced( 
        
               in_name, out_name, name_table, street_name_suffix_table); 
        
           if (in_name != EMPTY_NAMEID && out_name != EMPTY_NAMEID && same_name) 
        
           { 
        
               return TurnType::Continue; 
        
           } 
        
           return TurnType::Turn;

Next actions:

Fix Empty name_id checks need to be changed to empty name_id string checks #4642

… is empty

karenzshea · 2017-11-27T13:46:16Z

@daniel-j-h can you review here?

daniel-j-h · 2017-11-28T14:41:41Z

include/engine/guidance/collapse_scenario_detection.hpp

+             const RouteStepIterator step_leaving_intersection,
+             const std::string &step_prior_name,
+             const std::string &step_entering_name,
+             const std::string &step_leaving_name);


Why do we need the strings here? Aren't they in the route steps already?

oo good catch, did not see that this was already available.

daniel-j-h · 2017-11-28T14:43:14Z

src/engine/guidance/collapse_turns.cpp

@@ -371,6 +371,9 @@ RouteSteps collapseTurnInstructions(RouteSteps steps)
            break;

        const auto previous_step = findPreviousTurn(current_step);
+        const auto previous_step_name = facade.GetNameForID(previous_step->name_id).to_string();
+        const auto current_step_name = facade.GetNameForID(current_step->name_id).to_string();
+        const auto next_step_name = facade.GetNameForID(next_step->name_id).to_string();


Do we really need a string copy here or is a string view good enough?

(the .to_string() makes a copy of the internal memory used to represent a string)

this can be removed since way names are already available through RouteSteps

daniel-j-h · 2017-11-29T09:29:19Z

src/engine/guidance/collapse_scenario_detection.cpp

@@ -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)


daniel-j-h · 2017-11-29T09:29:38Z

src/engine/guidance/collapse_scenario_detection.cpp

-                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)


Why the change here?

daniel-j-h · 2017-11-29T09:31:05Z

src/engine/guidance/collapse_turns.cpp

@@ -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();


Unused, can this be removed, and then can the facade also be removed from the function signature?

daniel-j-h · 2017-11-29T09:31:54Z

src/extractor/guidance/intersection_handler.cpp

        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();


Do we have to make two string copies here? Or can we already check the string view for empty?

daniel-j-h · 2017-11-29T09:33:14Z

src/extractor/guidance/mergable_road_detector.cpp

            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())


I don't understand this - why to we

receive the name_id

use the name_id to fetch the road name

and only then check if the name_id is valid?

what will happen if the name id is invalid and we call GetNameForID on it?

daniel-j-h · 2017-11-29T09:33:58Z

src/extractor/guidance/motorway_handler.cpp

+            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();


And here - what happens if name id is invalid

daniel-j-h · 2017-11-29T09:34:10Z

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())


daniel-j-h · 2017-11-29T09:34:22Z

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();


daniel-j-h · 2017-11-29T09:34:33Z

src/extractor/guidance/sliproad_handler.cpp

@@ -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();


daniel-j-h · 2017-11-29T09:34:38Z

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();


daniel-j-h · 2017-11-30T17:19:23Z

src/extractor/guidance/intersection_handler.cpp

        node_data_container.GetAnnotation(node_based_graph.GetEdgeData(road.eid).annotation_data)
            .name_id;
+    const auto &in_name_empty = name_table.GetNameForID(in_name_id).to_string().empty();
+    const auto &out_name_empty = name_table.GetNameForID(out_name_id).to_string().empty();


GetXForID returns

http://www.boost.org/doc/libs/1_60_0/libs/utility/doc/html/string_ref.html

a zero-copy view onto the data. If you call .to_string() on it you make a copy of the data and allocate memory for that. But we only want to know if the data is empty. Check the string ref docs, there is already an empty function on the string ref.

🙇‍♀️

daniel-j-h · 2017-11-30T17:20:48Z

src/extractor/guidance/mergable_road_detector.cpp

+        if (name_id == EMPTY_NAMEID || road_name_id == EMPTY_NAMEID)
+            return true;
+        const auto road_name_empty = name_table.GetNameForID(road_name_id).to_string().empty();
+        const auto in_name_empty = name_table.GetNameForID(name_id).to_string().empty();


daniel-j-h · 2017-11-30T17:21:16Z

src/extractor/guidance/mergable_road_detector.cpp

+                return false;
+            const auto &road_name_empty = name_table.GetNameForID(road_name_id).to_string().empty();
+            const auto &required_name_empty =
+                name_table.GetNameForID(required_name_id).to_string().empty();


daniel-j-h · 2017-11-30T17:22:21Z

src/extractor/guidance/sliproad_handler.cpp

+                const auto unnamed =
+                    road_name_id == EMPTY_NAMEID
+                        ? true
+                        : name_table.GetNameForID(road_name_id).to_string().empty();


daniel-j-h · 2017-11-30T17:22:33Z

src/extractor/guidance/sliproad_handler.cpp

+                    : name_table.GetNameForID(sliproad_annotation.name_id).to_string().empty();
+            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();


daniel-j-h · 2017-11-30T17:22:45Z

src/extractor/guidance/turn_handler.cpp

+    const auto &via_name_empty =
+        via_data.name_id == EMPTY_NAMEID
+            ? true
+            : name_table.GetNameForID(via_data.name_id).to_string().empty();


daniel-j-h

Do you have stats from the statistics handler? Would be great to have a feeling for what this changeset changes. And maybe create a map and sample some locations.

daniel-j-h · 2017-12-01T15:30:43Z

src/extractor/guidance/motorway_handler.cpp

+                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();


Copy needed here?

daniel-j-h · 2017-12-01T15:31:28Z

src/extractor/guidance/roundabout_handler.cpp

                {
+                    const auto &edge_name = name_table.GetNameForID(edge_data.name_id).to_string();
+                    if (!edge_name.empty())


Copy needed here for the empty check?

daniel-j-h · 2017-12-01T15:33:25Z

src/extractor/guidance/sliproad_handler.cpp

+            const auto main_road_name_empty =
+                main_road_name_id == EMPTY_NAMEID
+                    ? true
+                    : name_table.GetNameForID(main_road_name_id).empty();


Here and below: you can simplify to

const auto empty = name_id == EMPTY || GetName(name_id).empty()

daniel-j-h · 2017-12-01T15:33:35Z

src/extractor/guidance/turn_handler.cpp

@@ -199,7 +199,9 @@ 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_empty =
+        via_data.name_id == EMPTY_NAMEID ? true : name_table.GetNameForID(via_data.name_id).empty();


karenzshea · 2017-12-05T11:48:20Z

Cool! I used https://github.com/mapbox/osrm-guidance-differ with a few modifications to compare current master against this branch and I saw some good changes from continue to turn maneuver types. On master, we sometimes announce a continue onto streets that are missing names, and now we announce a turn even if the street is unnamed. Mostly saw this on sliproads.

Previously we announced a maneuver onto the unnamed sliproad as a continue slight right, is now a turn slight right

Same here,

Map linked here

…d checks

Adds cucumber test for continue not taking ref into account when name…

2ae6361

… is empty

daniel-j-h added the Guidance label Nov 14, 2017

karenzshea added the Work In Progress label Nov 22, 2017

karenzshea force-pushed the name-ref-change branch from e305a73 to 2ae6361 Compare November 27, 2017 15:21

check empty name string in findBasicTurnType

1d62d5d

karenzshea force-pushed the name-ref-change branch from 1e3e3ff to 1d62d5d Compare November 27, 2017 16:12

karenzshea added 6 commits November 27, 2017 17:56

check empty string name in mergable road detector

abbc2d8

check empty name string in motorway handler

178f270

check empty name string in sliproad handler

4c68866

check empty name string in turn handler

b981db2

check empty name string in turn collapsing

6ead257

check empty name string in roundabout handler

84d294b

karenzshea added the Review label Nov 28, 2017

daniel-j-h commented Nov 28, 2017

View reviewed changes

karenzshea self-assigned this Nov 28, 2017

access way names through RouteStep in post processing

00f79f3

karenzshea force-pushed the name-ref-change branch from b6f095e to 00f79f3 Compare November 28, 2017 16:59

daniel-j-h commented Nov 29, 2017

View reviewed changes

check for empty name_id before getting data

d58414d

karenzshea force-pushed the name-ref-change branch from 3197736 to d58414d Compare November 30, 2017 12:27

daniel-j-h commented Nov 30, 2017

View reviewed changes

TheMarex removed the Work In Progress label Dec 1, 2017

karenzshea changed the title ~~Adds cucumber test for continue not taking ref into account when name is empty~~ Update name_id/name checks for empty IDs and empty strings Dec 1, 2017

daniel-j-h commented Dec 1, 2017

View reviewed changes

karenzshea force-pushed the name-ref-change branch from cde9582 to 93e3204 Compare December 4, 2017 17:07

check for empty on string_ref, rather than string itself

aad3b28

karenzshea force-pushed the name-ref-change branch from 93e3204 to aad3b28 Compare December 4, 2017 17:23

TheMarex added the Review - In feedback label Dec 5, 2017

TheMarex removed the Review label Dec 5, 2017

add assert for untested sliproad cases, removed redundant empty_namei…

faa9ac0

…d checks

TheMarex merged commit 9a8ed30 into master Dec 11, 2017

TheMarex deleted the name-ref-change branch December 11, 2017 15:38

TheMarex added this to the 5.14.2 milestone Dec 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update name_id/name checks for empty IDs and empty strings #4684

Update name_id/name checks for empty IDs and empty strings #4684

daniel-j-h commented Nov 14, 2017

karenzshea commented Nov 27, 2017

daniel-j-h Nov 28, 2017

karenzshea Nov 28, 2017

daniel-j-h Nov 28, 2017

karenzshea Nov 28, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

karenzshea Nov 30, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 29, 2017

daniel-j-h Nov 30, 2017

karenzshea Dec 1, 2017

daniel-j-h Nov 30, 2017

daniel-j-h Nov 30, 2017

daniel-j-h Nov 30, 2017

daniel-j-h Nov 30, 2017

daniel-j-h Nov 30, 2017

daniel-j-h left a comment

daniel-j-h Dec 1, 2017

daniel-j-h Dec 1, 2017

daniel-j-h Dec 1, 2017

daniel-j-h Dec 1, 2017

karenzshea commented Dec 5, 2017

	const auto same_name = !util::guidance::requiresNameAnnounced(
	in_name, out_name, name_table, street_name_suffix_table);

	if (in_name != EMPTY_NAMEID && out_name != EMPTY_NAMEID && same_name)
	{
	return TurnType::Continue;
	}

	return TurnType::Turn;

Update name_id/name checks for empty IDs and empty strings #4684

Update name_id/name checks for empty IDs and empty strings #4684

Conversation

daniel-j-h commented Nov 14, 2017

karenzshea commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-j-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenzshea commented Dec 5, 2017