contract over nodes connecting edges with differing names/speed

.gitignore

@@ -61,6 +61,7 @@ CMakeLists.txt.user
 .idea
 /.tidytmp
 vcpkg*/
+*.log
 
 # python
 .venv

CHANGELOG.md

@@ -36,6 +36,9 @@
    * FIXED: fix config generator with thor.costmatrix_allow_second_pass [#4567](https://github.com/valhalla/valhalla/pull/4567)
    * FIXED: infinite loop or other random corruption in isochrones when retrieving partial shape of an edge [#4547](https://github.com/valhalla/valhalla/pull/4547)
    * FIXED: Aggregation updates: update opposing local idx after aggregating the edges, added classification check for aggregation, and shortcut length changes [#4570](https://github.com/valhalla/valhalla/pull/4570)
+   * FIXED: Fix segfault in OSRM serializer with bannerInstructions when destination is on roundabout [#4480](https://github.com/valhalla/valhalla/pull/4481)
+   * FIXED: Fix segfault in costmatrix (date_time and time zone always added). [#4530](https://github.com/valhalla/valhalla/pull/4530)
+   * FIXED: Fixed roundoff issue in Tiles Row and Col methods [#4585](https://github.com/valhalla/valhalla/pull/4585)
 * **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)
@@ -75,7 +78,6 @@
    * UPDATED: updated country access overrides [#4460](https://github.com/valhalla/valhalla/pull/4460)
    * CHANGED: date_time refactor as a preparation to return DST/timezone related offset in the response [#4365](https://github.com/valhalla/valhalla/pull/4365)
    * ADDED: find connection on backward search for bidir matrix algo [#4329](https://github.com/valhalla/valhalla/pull/4329)
-   * FIXED: Fix segfault in OSRM serializer with bannerInstructions when destination is on roundabout [#4480](https://github.com/valhalla/valhalla/pull/4481)
    * CHANGED: Adujustment of walk speed when walking on slight downhill [#4302](https://github.com/valhalla/valhalla/pull/4302)
    * CHANGED: Do not reclassify ferry connections when no hierarchies are to be generated [#4487](https://github.com/valhalla/valhalla/pull/4487)
    * ADDED: Added a config option to sort nodes spatially during graph building [#4455](https://github.com/valhalla/valhalla/pull/4455)
@@ -84,15 +86,14 @@
    * CHANGED: use pkg-config to find spatialite & geos and remove our cmake modules; upgraded conan's boost to 1.83.0 in the process [#4253](https://github.com/valhalla/valhalla/pull/4253)
    * ADDED: Added aggregation logic to filter stage of tile building [#4512](https://github.com/valhalla/valhalla/pull/4512)
    * UPDATED: tz to 2023d [#4519](https://github.com/valhalla/valhalla/pull/4519)
-   * FIXED: Fix segfault in costmatrix (date_time and time zone always added). [#4530](https://github.com/valhalla/valhalla/pull/4530)
    * CHANGED: libvalhalla.pc generation to have finer controls; install third_party public headers; overhaul lots of CMake; remove conan support [#4516](https://github.com/valhalla/valhalla/pull/4516)
    * CHANGED: refactored matrix code to include a base class for all matrix algorithms to prepare for second passes on matrix [#4535](https://github.com/valhalla/valhalla/pull/4535)
    * ADDED: matrix second pass for connections not found in the first pass, analogous to /route [#4536](https://github.com/valhalla/valhalla/pull/4536)
    * UPDATED: cxxopts to 3.1.1 [#4541](https://github.com/valhalla/valhalla/pull/4541)
    * CHANGED: make use of vendored libraries optional (other than libraries which are not commonly in package managers or only used for testing) [#4544](https://github.com/valhalla/valhalla/pull/4544)
    * ADDED: Improved instructions for blind users [#3694](https://github.com/valhalla/valhalla/pull/3694)
-   * FIXED: Fixed roundoff issue in Tiles Row and Col methods [#4585](https://github.com/valhalla/valhalla/pull/4585)
    * ADDED: isochrone proper polygon support & pbf output for isochrone [#4575](https://github.com/valhalla/valhalla/pull/4575)
+   * CHANGED: contract nodes if connecting edges have different names or speed [#4608](https://github.com/valhalla/valhalla/pull/4608)
 
 ## Release Date: 2023-05-11 Valhalla 3.4.0
 * **Removed**

src/mjolnir/restrictionbuilder.cc

@@ -270,8 +270,8 @@ ComplexRestrictionBuilder CreateComplexRestriction(const OSMRestriction& restric
 };
 
 struct Result {
-  uint32_t forward_restrictions_count;
-  uint32_t reverse_restrictions_count;
+  uint32_t forward_restrictions_count = 0;
+  uint32_t reverse_restrictions_count = 0;
   std::vector<ComplexRestrictionBuilder> restrictions;
   std::unordered_set<GraphId> part_of_restriction;
 };

src/mjolnir/shortcutbuilder.cc

@@ -77,16 +77,6 @@ bool EdgesMatch(const graph_tile_ptr& tile, const DirectedEdge* edge1, const Dir
     return false;
   }
 
-  // Names must match
-  // TODO - this allows matches in any order. Do we need to maintain order?
-  // TODO - should allow near matches?
-  std::vector<std::string> edge1names = tile->GetNames(edge1);
-  std::vector<std::string> edge2names = tile->GetNames(edge2);
-  std::sort(edge1names.begin(), edge1names.end());
-  std::sort(edge2names.begin(), edge2names.end());
-  if (edge1names != edge2names)
-    return false;
-
   // if they have access restrictions those must match (for modes that use shortcuts)
   if (edge1->access_restriction()) {
     auto res1 = tile->GetAccessRestrictions(edge1 - tile->directededge(0), kVehicularAccess);
@@ -231,11 +221,6 @@ bool CanContract(GraphReader& reader,
     return false;
   }
 
-  // Can not have different speeds in the same direction
-  if ((oppdiredge1->speed() != edge2->speed()) || (oppdiredge2->speed() != edge1->speed())) {
-    return false;
-  }
-
   // ISO country codes at the end nodes must equal this node
   std::string iso = tile->admininfo(nodeinfo->admin_index()).country_iso();
   std::string e1_iso = EndNodeIso(edge1, reader);
@@ -403,13 +388,6 @@ std::pair<uint32_t, uint32_t> AddShortcutEdges(GraphReader& reader,
         std::reverse(shape.begin(), shape.end());
       }
 
-      // Get names - they apply over all edges of the shortcut
-      auto names = edgeinfo.GetNames();
-      auto tagged_values = edgeinfo.GetTaggedValues();
-      auto linguistics = edgeinfo.GetLinguisticTaggedValues();
-
-      auto types = edgeinfo.GetTypes();
-
       // Add any access restriction records. We don't contract if they differ, so if
       // there's any, they're the same for all involved edges
       if (newedge.access_restriction()) {
@@ -458,9 +436,6 @@ std::pair<uint32_t, uint32_t> AddShortcutEdges(GraphReader& reader,
         total_edge_count++;
       }
 
-      // Names can be different in the forward and backward direction
-      bool diff_names = tilebuilder.OpposingEdgeInfoDiffers(tile, directededge);
-
       // Get the length from the shape. This prevents roundoff issues when forming
       // elevation.
       uint32_t length = valhalla::midgard::length(shape);
@@ -469,13 +444,13 @@ std::pair<uint32_t, uint32_t> AddShortcutEdges(GraphReader& reader,
       // edge in case multiple shortcut edges exist between the 2 nodes.
       // Test whether this shape is forward or reverse (in case an existing
       // edge exists). Shortcuts use way Id = 0.Set mean elevation to 0 as a placeholder,
-      // set it later if adding elevation to this dataset.
+      // set it later if adding elevation to this dataset. No need for names etc, shortcuts
+      // aren't used in guidance
       bool forward = true;
       uint32_t idx = ((length & 0xfffff) | ((shape.size() & 0xfff) << 20));
       uint32_t edge_info_offset =
           tilebuilder.AddEdgeInfo(idx, start_node, end_node, 0, 0, edgeinfo.bike_network(),
-                                  edgeinfo.speed_limit(), shape, names, tagged_values, linguistics,
-                                  types, forward, diff_names);
+                                  edgeinfo.speed_limit(), shape, {}, {}, {}, 0U, forward, false);
 
       newedge.set_edgeinfo_offset(edge_info_offset);
 

test/astar.cc

@@ -1571,7 +1571,7 @@
   )";
   const gurka::ways ways = {
       // make ABC to be a shortcut
-      {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}}},
+      {"ABC", {{"highway", "motorway"}, {"maxspeed", "80"}}},
       // make CDE to be a shortcut
       {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}}},
       {"1A", {{"highway", "secondary"}}},
@@ -1580,8 +1580,8 @@
       {"D5", {{"highway", "secondary"}}},
       // set speeds less than on ABCDE path to force the algorithm
       // to go through ABCDE nodes instead of AXY
-      {"AX", {{"highway", "primary"}, {"maxspeed", "70"}}},
-      {"XY", {{"highway", "primary"}, {"maxspeed", "70"}}},
+      {"AX", {{"highway", "motorway"}, {"maxspeed", "70"}}},
+      {"XY", {{"highway", "trunk"}, {"maxspeed", "70"}}},
       {"YE", {{"highway", "primary"}, {"maxspeed", "80"}}},
       {"E2", {{"highway", "secondary"}}},
   };
@@ -1679,6 +1679,81 @@
   }
 }
 
+// TODO(nils): this test fails currently, because bidir A* has a problem with 2 shortcuts between the
+// same nodes: https://github.com/valhalla/valhalla/issues/4609
+TEST(BiDiAstar, DISABLED_test_recost_path_failing) {
+  const std::string ascii_map = R"(
+           X-----------Y
+          /             \
+    1----A               E---2
+          \             /
+           B--C--------D
+  )";
+  const gurka::ways ways = {
+      // make ABCDE to be a shortcut
+      {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}}},
+      {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}}},
+      {"1A", {{"highway", "secondary"}}},
+      // set speeds less than on ABCDE path to force the algorithm
+      // to go through ABCDE nodes instead of AXY
+      {"AX", {{"highway", "primary"}, {"maxspeed", "70"}}},
+      {"XY", {{"highway", "primary"}, {"maxspeed", "70"}}},
+      {"YE", {{"highway", "primary"}, {"maxspeed", "80"}}},
+      {"E2", {{"highway", "secondary"}}},
+  };
+
+  auto nodes = gurka::detail::map_to_coordinates(ascii_map, 500);
+
+  const std::string test_dir = "test/data/astar_shortcuts_recosting";
+  const auto map = gurka::buildtiles(nodes, ways, {}, {}, test_dir);
+
+  vb::GraphReader graphreader(map.config.get_child("mjolnir"));
+
+  // before continue check that ABC is actually a shortcut
+  const auto ABCDE = gurka::findEdgeByNodes(graphreader, nodes, "A", "E");
+  ASSERT_TRUE(std::get<1>(ABCDE)->is_shortcut()) << "Expected ABCDE to be a shortcut";
+
+  Options options;
+  create_costing_options(options, Costing::auto_);
+  vs::TravelMode travel_mode = vs::TravelMode::kDrive;
+  const auto mode_costing = vs::CostFactory().CreateModeCosting(options, travel_mode);
+
+  std::vector<vb::Location> locations;
+  // set origin location
+  locations.push_back({nodes["1"]});
+  // set destination location
+  locations.push_back({nodes["2"]});
+  auto pbf_locations = ToPBFLocations(locations, graphreader, mode_costing[int(travel_mode)]);
+
+  vt::BidirectionalAStar astar;
+
+  // hack hierarchy limits to allow to go through the shortcut
+  {
+    auto& hierarchy_limits =
+        mode_costing[int(travel_mode)]->GetHierarchyLimits(); // access mutable limits
+    for (auto& hierarchy : hierarchy_limits) {
+      hierarchy.Relax(0.f, 0.f);
+    }
+  }
+  const auto path =
+      astar.GetBestPath(pbf_locations[0], pbf_locations[1], graphreader, mode_costing, travel_mode)
+          .front();
+
+  // collect names of base edges
+  std::vector<std::string> expected_names = {"1A", "AB", "BC", "CD", "DE", "E2"};
+  std::vector<std::string> actual_names;
+  for (const auto& info : path) {
+    const auto* edge = graphreader.directededge(info.edgeid);
+    ASSERT_FALSE(edge->is_shortcut()) << "Final path shouldn't contain shortcuts";
+
+    const auto name = graphreader.edgeinfo(info.edgeid).GetNames()[0];
+    actual_names.emplace_back(name);
+  }
+  // TODO(nils): it gets the wrong path! bidir A* has a problem with 2 shortcuts between the same
+  // nodes
+  EXPECT_EQ(actual_names, expected_names);
+}
+
 class BiAstarTest : public thor::BidirectionalAStar {
 public:
   explicit BiAstarTest(const boost::property_tree::ptree& config = {}) : BidirectionalAStar(config) {

test/gurka/gurka.cc

@@ -488,6 +488,7 @@ map buildtiles(const nodelayout& layout,
  * @param end_node the node that should be the target of the directed edge you want
  * @param tile_id optional tile_id to limit the search to
  * @param way_id optional way_id to limit the search to
+ * @param is_shortcut whether we want a shortcut returned
  * @return the directed edge that matches, or nullptr if there was no match
  */
 std::tuple<const baldr::GraphId,

test/gurka/test_64bit_wayid.cc

@@ -7,13 +7,17 @@
   // check all edges have correct edge info
   for (const auto& tile_id : reader.GetTileSet()) {
     auto tile = reader.GetGraphTile(tile_id);
-    for (auto edge = tile_id; edge.id() < tile->header()->directededgecount(); ++edge) {
-      // we should find every way id in the tile set
-      auto info = tile->edgeinfo(tile->directededge(edge));
+    for (auto edge_id = tile_id; edge_id.id() < tile->header()->directededgecount(); ++edge_id) {
+      // we should find every way id in the tile set, unless it's a shortcut, no way id there
+      auto* edge = tile->directededge(edge_id);
+      if (edge->is_shortcut()) {
+        continue;
+      }
+      auto info = tile->edgeinfo(edge);
       auto id = info.wayid();
       auto found = osm_way_ids.find(id);
       if (found == osm_way_ids.cend()) {
-        EXPECT_FALSE(found == osm_way_ids.cend()) << " couldnt find " << info.wayid();
+        ASSERT_FALSE(found == osm_way_ids.cend()) << " couldnt find " << info.wayid();
         return;
       }
       osm_way_ids.erase(found);

test/gurka/test_avoids.cc

@@ -1,4 +1,5 @@
 #include "gurka.h"
+#include <boost/format.hpp>
 #include <boost/geometry.hpp>
 #include <boost/geometry/geometries/register/point.hpp>
 #include <boost/geometry/multi/geometries/register/multi_polygon.hpp>

test/gurka/test_barrier_uturns.cc

@@ -1,4 +1,5 @@
 #include "gurka.h"
+#include <boost/format.hpp>
 #include <gtest/gtest.h>
 
 using namespace valhalla;

test/gurka/test_gtfs.cc

@@ -847,7 +847,7 @@ TEST(GtfsExample, MakeTile) {
   }
 
   EXPECT_EQ(transit_nodes, 15);
-  EXPECT_EQ(uses[Use::kRoad], 12);
+  EXPECT_EQ(uses[Use::kRoad], 14); // + 2 shortcuts
   EXPECT_EQ(uses[Use::kTransitConnection], 20);
   EXPECT_EQ(uses[Use::kPlatformConnection], 10);
   EXPECT_EQ(uses[Use::kEgressConnection], 10);

test/gurka/test_landmarks.cc

@@ -143,6 +143,9 @@ void CheckLandmarksInTiles(GraphReader& reader, const GraphId& graphid) {
 
   auto tile = reader.GetGraphTile(graphid);
   for (const auto& e : tile->GetDirectedEdges()) {
+    if (e.is_shortcut()) {
+      continue;
+    }
     auto ei = tile->edgeinfo(&e);
     auto tagged_values = ei.GetTags();
 
@@ -620,7 +623,7 @@ TEST(LandmarkTest, TestLandmarksInManeuvers) {
           "Checking landmarks in maneuver failed: cannot find the maneuver in the expected result!");
     }
     ASSERT_EQ(result_landmarks.size(), expected->second.size());
-    for (auto i = 0; i < result_landmarks.size(); ++i) {
+    for (size_t i = 0; i < result_landmarks.size(); ++i) {
       EXPECT_EQ(result_landmarks[i], expected->second[i]);
     }
   }

test/gurka/test_shortcut.cc

@@ -67,38 +67,6 @@ TEST(Shortcuts, LoopWithoutShortcut) {
   EXPECT_FALSE(shortcut.Is_Valid()) << "Shortcuts found. Check the map.";
 }
 
-// Here no shortcuts are created. There could be one from A to C with speed 80 but in the opposite
-// direction speeds differ which blocks CA creation.
-TEST(Shortcuts, CreateInvalid) {
-  constexpr double gridsize = 50;
-
-  const std::string ascii_map = R"(
-      A--B--C
-  )";
-
-  const gurka::ways ways = {
-      {"AB",
-       {{"highway", "primary"},
-        {"name", "Independence Avenue"},
-        {"maxspeed:forward", "80"},
-        {"maxspeed:backward", "80"}}},
-      {"BC",
-       {{"highway", "primary"},
-        {"name", "Independence Avenue"},
-        {"maxspeed:forward", "80"},
-        {"maxspeed:backward", "90"}}},
-  };
-
-  const auto layout = gurka::detail::map_to_coordinates(ascii_map, gridsize);
-  auto map = gurka::buildtiles(layout, ways, {}, {}, "test/data/gurka_openlrjoiner_shortcut_speed");
-
-  baldr::GraphReader graph_reader(map.config.get_child("mjolnir"));
-
-  // check that there are no shortcut edges
-  EXPECT_ANY_THROW(gurka::findEdgeByNodes(graph_reader, layout, "A", "C"));
-  EXPECT_ANY_THROW(gurka::findEdgeByNodes(graph_reader, layout, "C", "A"));
-}
-
 TEST(Shortcuts, ShortcutSpeed) {
   // At C node turn duration is present. As a result the speed for AE shortcut is decreased
   // from 100 kph to 93 kph and for EA shortcut - from 100 kph to 98 kph in the test case below.
@@ -363,13 +331,15 @@ TEST(Shortcuts, ShortcutsInBins) {
 }
 
 TEST(Shortcuts, ShortcutRestrictions) {
+  using node_pairs = std::vector<std::pair<std::string, std::string>>;
+
+  // the first line should produce only one shortcut, the second two
   const std::string ascii_map = R"(
   A--B--C--D--E--F
 
   G--H--I--J--K--L
   )";
 
-  // the first line should produce only one shortcut, the second two
   gurka::ways ways = {
       {"AB", {{"highway", "motorway"}, {"name", "highway"}}},
       {"BC", {{"highway", "motorway"}, {"name", "highway"}}},
@@ -389,13 +359,11 @@ TEST(Shortcuts, ShortcutRestrictions) {
   baldr::GraphReader reader(map.config.get_child("mjolnir"));
 
   // test we got the right shortcuts edges, implicitly means they were broken properly
-  for (const auto& end_node : {"C", "J", "L"}) {
-    const auto shortcut =
-        gurka::findEdge(reader, layout, "highway", end_node, baldr::GraphId{}, 0, true);
+  for (const auto& pair :
+       node_pairs{{"A", "C"}, {"C", "A"}, {"H", "J"}, {"J", "H"}, {"J", "L"}, {"L", "J"}}) {
+    const auto shortcut = gurka::findEdgeByNodes(reader, layout, pair.first, pair.second);
     EXPECT_TRUE(std::get<1>(shortcut)->is_shortcut());
-    EXPECT_TRUE(std::get<3>(shortcut)->is_shortcut());
     EXPECT_NEAR(std::get<1>(shortcut)->length(), 3000, 1);
-    EXPECT_NEAR(std::get<3>(shortcut)->length(), 3000, 1);
   }
 
   // test the right edges are really superseded by a shortcut
@@ -430,12 +398,9 @@ TEST(Shortcuts, ShortcutRestrictions) {
   }
 
   // we did build the long shorcuts across all edges
-  for (const auto& end_node : {"F", "L"}) {
-    const auto shortcut =
-        gurka::findEdge(reader2, layout, "highway", end_node, baldr::GraphId{}, 0, true);
+  for (const auto& pair : node_pairs{{"A", "F"}, {"F", "A"}, {"G", "L"}, {"L", "G"}}) {
+    const auto shortcut = gurka::findEdgeByNodes(reader2, layout, pair.first, pair.second);
     EXPECT_TRUE(std::get<1>(shortcut)->is_shortcut());
-    EXPECT_TRUE(std::get<3>(shortcut)->is_shortcut());
     EXPECT_NEAR(std::get<1>(shortcut)->length(), 7500, 1);
-    EXPECT_NEAR(std::get<3>(shortcut)->length(), 7500, 1);
   }
 }