update version to 5.3.2

Conflicts:
	src/extractor/guidance/turn_lane_augmentation.cpp

.travis.yml

@@ -13,6 +13,7 @@ notifications:
 branches:
   only:
     - master
+    - "5.3"
 
 cache:
   ccache: true

CHANGELOG.md

@@ -1,3 +1,14 @@
+# 5.3.2
+  Changes from 5.3.1
+    - Bugfixes
+      - fixed a bug that occurred when trimming very short segments at the begin/end of a route (less than 1 meter)
+
+# 5.3.1
+  Changes from 5.3.1
+    - Bugfixes:
+      - Disabled broken lane handling for complex uturn/oneway combinations for now (190 intersections affected on the planet)
+      - Fixed a bug with overlaping geometries, which broke OSRM on recent Egypt extracts with data-modelling issues
+
 # 5.3.0
   Changes from 5.3.0-rc.3
     - Guidance

CMakeLists.txt

@@ -10,7 +10,7 @@ endif()
 project(OSRM C CXX)
 set(OSRM_VERSION_MAJOR 5)
 set(OSRM_VERSION_MINOR 3)
-set(OSRM_VERSION_PATCH 0)
+set(OSRM_VERSION_PATCH 2)
 
 # these two functions build up custom variables:
 #   OSRM_INCLUDE_PATHS and OSRM_DEFINES

features/guidance/trimming.feature

@@ -0,0 +1,52 @@
+@routing  @guidance @post-processing
+Feature: General Post-Processing related features
+
+    Background:
+        Given the profile "car"
+        Given a grid size of 0.1 meters
+
+    # this testcase used to crash geometry generation (at that time handled during intersection generation)
+    Scenario: Regression Test 2754 
+        Given the node map
+            | a | b | c | d | e |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   |   |   |   |   |   |
+            |   |   |   |   | f | g | h | i | j |
+
+        And the ways
+            | nodes |
+            | abcde |
+            | ef    |
+            | fghij |
+
+        When I route I should get
+            | waypoints | route |
+            | a,j       | ef,ef |

features/guidance/turn-lanes.feature

@@ -736,4 +736,113 @@ Feature: Turn Lane Guidance
             | x,d       | road,road | depart,arrive | ,     |
 
 
+    Scenario: Lane Parsing Issue #2694
+        Given the node map
+            |   | c |
+            | a | b |
+            |   | d |
 
+        And the ways
+            | nodes | highway | turn:lanes:forward |
+            | ab    | primary | left;left\|right   |
+            | bc    | primary |                    |
+            | bd    | primary |                    |
+
+        When I route I should get
+            | waypoints | route     | turns                   | lanes                   |
+            | a,c       | ab,bc,bc  | depart,turn left,arrive | ,left:true right:false, |
+
+    # http://www.openstreetmap.org/#map=19/47.97685/7.82933&layers=D
+    @bug @todo
+    Scenario: Lane Parsing Issue #2706: None Assignments I
+        Given the node map
+            |   | f |   |   | j  |   |
+            |   |   |   |   |    |   |
+            | a | b | c |   | d  | e |
+            |   |   |   |   |    |   |
+            |   |   |   |   | i  |   |
+            |   | g |   |   | h  |   |
+
+        And the nodes
+            | node | highway         |
+            | a    | traffic_signals |
+            | i    | traffic_signals |
+
+        And the ways
+            | nodes | highway        | name           | oneway | turn:lanes:forward       |
+            | ab    | secondary      | Wiesentalstr   |        | through;left\|right      |
+            | bc    | secondary      | Wiesentalstr   |        | none\|left;through       |
+            | cd    | secondary      | Wiesentalstr   |        | none\|left;through       |
+            | de    | residential    | Wippertstr     |        |                          |
+            | fb    | secondary      | Merzhauser Str | yes    | through\|through\|right  |
+            | bg    | secondary      | Merzhauser Str | yes    |                          |
+            | hi    | secondary      | Merzhauser Str | yes    | left;reverse\|none\|none |
+            | ic    | secondary_link | Merzhauser Str | yes    |                          |
+            | id    | secondary      | Merzhauser Str | yes    | through;right\|none      |
+            | dj    | secondary      | Merzhauser Str | yes    |                          |
+
+        And the relations
+            | type        | way:from | way:to | node:via | restriction    |
+            | restriction | fb       | fb     | b        | no_left_turn   |
+            | restriction | ic       | cb     | c        | only_left_turn |
+            | restriction | id       | dc     | d        | no_left_turn   |
+
+        When I route I should get
+            | waypoints | route     | turns                   | lanes                  |
+            | h,a ||||
+            # Note: at the moment we don't care about routes, we care about the extract process triggering assertions
+
+    # https://www.openstreetmap.org/#map=19/47.99257/7.83276&layers=D
+    @bug @todo
+    Scenario: Lane Parsing Issue #2706: None Assignments II
+        Given the node map
+            |   | k | l |   |
+            | j | a | b | f |
+            | i | c | d | e |
+            |   | h | g |   |
+
+        And the ways
+            | nodes | highway        | name           | oneway | turn:lanes                           |
+            | ka    | secondary      | Eschholzstr    | yes    | left;reverse\|through\|through\|none |
+            | kj    | unclassified   | kj             | yes    |                                      |
+            | ac    | secondary      | Eschholzstr    | yes    | left;reverse\|none\|none\|none       |
+            | ch    | secondary      | Eschholzstr    | yes    |                                      |
+            | gd    | secondary      | Eschholzstr    | yes    | left;reverse\|through\|through\|none |
+            | db    | secondary      | Eschholzstr    | yes    | left;reverse\|through\|through\|none |
+            | bl    | secondary      | Eschholzstr    | yes    |                                      |
+            | fb    | residential    | Haslacher Str  | yes    | left;reverse\|left;through\|right    |
+            | ba    | secondary_link | Haslacher Str  | yes    | left;reverse\|left;through           |
+            | aj    | unclassified   | Haslacher Str  | yes    |                                      |
+            | ic    | unclassified   | Haslacher Str  | yes    | left;reverse\|left\|through          |
+            | cd    | secondary_link | Haslacher Str  | yes    | left;reverse\|left\|through          |
+            | de    | residential    | Haslacher Str  | yes    |                                      |
+
+        And the relations
+            | type        | way:from | way:to | node:via | restriction      |
+            | restriction | ka       | ac     | a        | only_straight_on |
+            | restriction | ic       | cd     | c        | only_straight_on |
+            | restriction | gd       | db     | d        | only_straight_on |
+
+        When I route I should get
+            | waypoints | route     | turns                   | lanes                  |
+            | i,e ||||
+            # Note: at the moment we don't care about routes, we care about the extract process triggering assertions
+
+    @bug @todo
+    Scenario: Lane Parsing Issue #2706: None Assignments III - Minimal reproduction recipe
+        Given the node map
+            |   |   | l |   |
+            |   | a | b |   |
+            |   |   | d |   |
+            |   |   |   |   |
+
+        And the ways
+            | nodes | highway        | name           | oneway | turn:lanes                           |
+            | db    | secondary      | Eschholzstr    | yes    | left;reverse\|through\|through\|none |
+            | bl    | secondary      | Eschholzstr    | yes    |                                      |
+            | ba    | secondary_link | Haslacher Str  | yes    |                                      |
+
+        When I route I should get
+            | waypoints | route     | turns                   | lanes                  |
+            | d,a ||||
+            # Note: at the moment we don't care about routes, we care about the extract process triggering assertions

src/engine/guidance/post_processing.cpp

@@ -878,25 +878,34 @@ void trimShortSegments(std::vector<RouteStep> &steps, LegGeometry &geometry)
                                            geometry.locations[0], geometry.locations[1]) <= 1;
     if (zero_length_step || duplicated_coordinate)
     {
-        // fixup the coordinate
-        geometry.locations.erase(geometry.locations.begin());
-        geometry.annotations.erase(geometry.annotations.begin());
-        geometry.osm_node_ids.erase(geometry.osm_node_ids.begin());
 
         // remove the initial distance value
         geometry.segment_distances.erase(geometry.segment_distances.begin());
 
+        const auto offset = zero_length_step ? geometry.segment_offsets[1] : 1;
+        if (offset > 0)
+        {
+            // fixup the coordinates/annotations/ids
+            geometry.locations.erase(geometry.locations.begin(),
+                                     geometry.locations.begin() + offset);
+            geometry.annotations.erase(geometry.annotations.begin(),
+                                       geometry.annotations.begin() + offset);
+            geometry.osm_node_ids.erase(geometry.osm_node_ids.begin(),
+                                        geometry.osm_node_ids.begin() + offset);
+        }
+
         // We have to adjust the first step both for its name and the bearings
         if (zero_length_step)
         {
+            // since we are not only checking for epsilon but for a full meter, we can have multiple
+            // coordinates here.
             // move offsets to front
-            BOOST_ASSERT(geometry.segment_offsets[1] == 1);
             // geometry offsets have to be adjusted. Move all offsets to the front and reduce by
             // one. (This is an inplace forward one and reduce by one)
             std::transform(geometry.segment_offsets.begin() + 1,
                            geometry.segment_offsets.end(),
                            geometry.segment_offsets.begin(),
-                           [](const std::size_t val) { return val - 1; });
+                           [offset](const std::size_t val) { return val - offset; });
 
             geometry.segment_offsets.pop_back();
             const auto &current_depart = steps.front();
@@ -937,9 +946,9 @@ void trimShortSegments(std::vector<RouteStep> &steps, LegGeometry &geometry)
         }
 
         // and update the leg geometry indices for the removed entry
-        std::for_each(steps.begin(), steps.end(), [](RouteStep &step) {
+        std::for_each(steps.begin(), steps.end(), [offset](RouteStep &step) {
-            --step.geometry_begin;
+            step.geometry_begin -= offset;
-            --step.geometry_end;
+            step.geometry_end -= offset;
         });
 
         auto &first_step = steps.front();
@@ -971,10 +980,12 @@ void trimShortSegments(std::vector<RouteStep> &steps, LegGeometry &geometry)
     // all zero-length instructions
     if (next_to_last_step.distance <= 1 && steps.size() > 2)
     {
-        geometry.locations.pop_back();
-        geometry.annotations.pop_back();
-        geometry.osm_node_ids.pop_back();
         geometry.segment_offsets.pop_back();
+        // remove all the last coordinates from the geometry
+        geometry.locations.resize(geometry.segment_offsets.back() + 1);
+        geometry.annotations.resize(geometry.segment_offsets.back() + 1);
+        geometry.osm_node_ids.resize(geometry.segment_offsets.back() + 1);
+
         BOOST_ASSERT(geometry.segment_distances.back() <= 1);
         geometry.segment_distances.pop_back();
 
@@ -983,6 +994,7 @@ void trimShortSegments(std::vector<RouteStep> &steps, LegGeometry &geometry)
         next_to_last_step.maneuver.bearing_after = 0;
         next_to_last_step.intersections.front().lanes = util::guidance::LaneTupel();
         next_to_last_step.intersections.front().lane_description.clear();
+        next_to_last_step.geometry_end = next_to_last_step.geometry_begin + 1;
         BOOST_ASSERT(next_to_last_step.intersections.size() == 1);
         auto &last_intersection = next_to_last_step.intersections.back();
         last_intersection.bearings = {last_intersection.bearings[last_intersection.in]};
@@ -1030,6 +1042,8 @@ void trimShortSegments(std::vector<RouteStep> &steps, LegGeometry &geometry)
         last_step.intersections.front().bearings.front() = util::bearing::reverseBearing(bearing);
     }
 
+    BOOST_ASSERT(steps.back().geometry_end == geometry.locations.size());
+
     BOOST_ASSERT(steps.front().intersections.size() >= 1);
     BOOST_ASSERT(steps.front().intersections.front().bearings.size() == 1);
     BOOST_ASSERT(steps.front().intersections.front().entry.size() == 1);

src/extractor/extractor_callbacks.cpp

@@ -193,7 +193,8 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
             for (auto token_itr = inner_tokens.begin(); token_itr != inner_tokens.end();
                  ++token_itr)
             {
-                auto position = std::find(osm_lane_strings, osm_lane_strings + num_osm_tags, *token_itr);
+                auto position =
+                    std::find(osm_lane_strings, osm_lane_strings + num_osm_tags, *token_itr);
                 const auto translated_mask =
                     masks_by_osm_string[std::distance(osm_lane_strings, position)];
                 if (translated_mask == TurnLaneType::empty)
@@ -203,7 +204,10 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
                                                          << *token_itr << "\"";
                     return {};
                 }
-                BOOST_ASSERT((lane_mask & translated_mask) == 0); // make sure the mask is valid
+
+                // In case of multiple times the same lane indicators withn a lane, as in
+                // "left;left|.."  or-ing the masks generates a single "left" enum.
+                // Which is fine since this is data issue and we can't represent it anyway.
                 lane_mask |= translated_mask;
             }
             // add the lane to the description
@@ -265,8 +269,8 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
         // name_offsets already has an offset of a new name, take the offset index as the name id
         name_id = external_memory.name_offsets.size() - 1;
 
-        external_memory.name_char_data.reserve(external_memory.name_char_data.size() + name_length
+        external_memory.name_char_data.reserve(external_memory.name_char_data.size() + name_length +
-                                               + destinations_length + pronunciation_length);
+                                               destinations_length + pronunciation_length);
 
         std::copy(parsed_way.name.c_str(),
                   parsed_way.name.c_str() + name_length,
@@ -306,7 +310,9 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
     std::transform(input_way.nodes().begin(),
                    input_way.nodes().end(),
                    std::back_inserter(external_memory.used_node_id_list),
-                   [](const osmium::NodeRef &ref) { return OSMNodeID{static_cast<std::uint64_t>(ref.ref())}; });
+                   [](const osmium::NodeRef &ref) {
+                       return OSMNodeID{static_cast<std::uint64_t>(ref.ref())};
+                   });
 
     const bool is_opposite_way = TRAVEL_MODE_INACCESSIBLE == parsed_way.forward_travel_mode;
 
@@ -338,7 +344,8 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
         external_memory.way_start_end_id_list.push_back(
             {OSMWayID{static_cast<std::uint32_t>(input_way.id())},
              OSMNodeID{static_cast<std::uint64_t>(input_way.nodes().back().ref())},
-             OSMNodeID{static_cast<std::uint64_t>(input_way.nodes()[input_way.nodes().size() - 2].ref())},
+             OSMNodeID{
+                 static_cast<std::uint64_t>(input_way.nodes()[input_way.nodes().size() - 2].ref())},
              OSMNodeID{static_cast<std::uint64_t>(input_way.nodes()[1].ref())},
              OSMNodeID{static_cast<std::uint64_t>(input_way.nodes()[0].ref())}});
     }
@@ -372,27 +379,28 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
                 input_way.nodes().cbegin(),
                 input_way.nodes().cend(),
                 [&](const osmium::NodeRef &first_node, const osmium::NodeRef &last_node) {
-                    external_memory.all_edges_list.push_back(
+                    external_memory.all_edges_list.push_back(InternalExtractorEdge(
-                        InternalExtractorEdge(OSMNodeID{static_cast<std::uint64_t>(first_node.ref())},
+                        OSMNodeID{static_cast<std::uint64_t>(first_node.ref())},
-                                              OSMNodeID{static_cast<std::uint64_t>(last_node.ref())},
+                        OSMNodeID{static_cast<std::uint64_t>(last_node.ref())},
-                                              name_id,
+                        name_id,
-                                              backward_weight_data,
+                        backward_weight_data,
-                                              false,
+                        false,
-                                              true,
+                        true,
-                                              parsed_way.roundabout,
+                        parsed_way.roundabout,
-                                              parsed_way.is_access_restricted,
+                        parsed_way.is_access_restricted,
-                                              parsed_way.is_startpoint,
+                        parsed_way.is_startpoint,
-                                              parsed_way.backward_travel_mode,
+                        parsed_way.backward_travel_mode,
-                                              true,
+                        true,
-                                              turn_lane_id_backward,
+                        turn_lane_id_backward,
-                                              road_classification));
+                        road_classification));
                 });
         }
 
         external_memory.way_start_end_id_list.push_back(
             {OSMWayID{static_cast<std::uint32_t>(input_way.id())},
              OSMNodeID{static_cast<std::uint64_t>(input_way.nodes().back().ref())},
-             OSMNodeID{static_cast<std::uint64_t>(input_way.nodes()[input_way.nodes().size() - 2].ref())},
+             OSMNodeID{
+                 static_cast<std::uint64_t>(input_way.nodes()[input_way.nodes().size() - 2].ref())},
              OSMNodeID{static_cast<std::uint64_t>(input_way.nodes()[1].ref())},
              OSMNodeID{static_cast<std::uint64_t>(input_way.nodes()[0].ref())}});
     }

src/extractor/guidance/intersection_generator.cpp

@@ -143,7 +143,20 @@ Intersection IntersectionGenerator::getConnectedRoads(const NodeID from_node,
     const auto valid_count =
         boost::count_if(intersection, [](const ConnectedRoad &road) { return road.entry_allowed; });
     if (0 == valid_count && uturn_could_be_valid)
-        intersection[0].entry_allowed = true;
+    {
+        // after intersections sorting by angles, find the u-turn with (from_node == to_node)
+        // that was inserted together with setting uturn_could_be_valid flag
+        std::size_t self_u_turn = 0;
+        while (self_u_turn < intersection.size()
+               && intersection[self_u_turn].turn.angle < std::numeric_limits<double>::epsilon()
+               && from_node != node_based_graph.GetTarget(intersection[self_u_turn].turn.eid))
+        {
+            ++self_u_turn;
+        }
+
+        BOOST_ASSERT(from_node == node_based_graph.GetTarget(intersection[self_u_turn].turn.eid));
+        intersection[self_u_turn].entry_allowed = true;
+    }
 
     return mergeSegregatedRoads(std::move(intersection));
 }

src/extractor/guidance/turn_lane_augmentation.cpp

@@ -282,7 +282,14 @@ LaneDataVector handleNoneValueAtSimpleTurn(LaneDataVector lane_data,
         // a pgerequisite is simple turns. Larger differences should not end up here
         // an additional line at the side is only reasonable if it is targeting public
         // service vehicles. Otherwise, we should not have it
-        BOOST_ASSERT(connection_count + 1 == lane_data.size());
+        //
+        // TODO(mokob): #2730 have a look please
+        // BOOST_ASSERT(connection_count + 1 == lane_data.size());
+        //
+        if (connection_count + 1 != lane_data.size())
+        {
+            goto these_intersections_are_clearly_broken_at_the_moment;
+        }
 
         lane_data = mergeNoneTag(none_index, std::move(lane_data));
     }
@@ -292,6 +299,9 @@ LaneDataVector handleNoneValueAtSimpleTurn(LaneDataVector lane_data,
     {
         lane_data = handleRenamingSituations(none_index, std::move(lane_data), intersection);
     }
+
+these_intersections_are_clearly_broken_at_the_moment:
+
     // finally make sure we are still sorted
     std::sort(lane_data.begin(), lane_data.end());
     return lane_data;

src/extractor/guidance/turn_lane_handler.cpp

@@ -351,8 +351,14 @@ bool TurnLaneHandler::isSimpleIntersection(const LaneDataVector &lane_data,
             if (lane_data.back().tag == TurnLaneType::uturn)
                 return findBestMatchForReverse(lane_data[lane_data.size() - 2].tag, intersection);
 
-            BOOST_ASSERT(lane_data.front().tag == TurnLaneType::uturn);
+            // TODO(mokob): #2730 have a look please
-            return findBestMatchForReverse(lane_data[1].tag, intersection);
+            // BOOST_ASSERT(lane_data.front().tag == TurnLaneType::uturn);
+            // return findBestMatchForReverse(lane_data[1].tag, intersection);
+            //
+            if (lane_data.front().tag == TurnLaneType::uturn)
+                return findBestMatchForReverse(lane_data[1].tag, intersection);
+
+            return findBestMatch(data.tag, intersection);
         }();
         std::size_t match_index = std::distance(intersection.begin(), best_match);
         all_simple &= (matched_indices.count(match_index) == 0);