contract over nodes connecting edges with differing names/speed

.gitignore

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

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);
@@ -337,18 +322,18 @@ bool IsEnteringEdgeOfContractedNode(GraphReader& reader, const GraphId& nodeid,
 }
 
 // Add shortcut edges (if they should exist) from the specified node
-uint32_t AddShortcutEdges(GraphReader& reader,
-                          const graph_tile_ptr& tile,
-                          GraphTileBuilder& tilebuilder,
-                          const GraphId& start_node,
-                          const uint32_t edge_index,
-                          const uint32_t edge_count,
-                          std::unordered_map<uint32_t, uint32_t>& shortcuts) {
+std::pair<uint32_t, uint32_t> AddShortcutEdges(GraphReader& reader,
+                                               const graph_tile_ptr& tile,
+                                               GraphTileBuilder& tilebuilder,
+                                               const GraphId& start_node,
+                                               const uint32_t edge_index,
+                                               const uint32_t edge_count,
+                                               std::unordered_map<uint32_t, uint32_t>& shortcuts) {
   // Shortcut edges have to start at a node that is not contracted - return if
   // this node can be contracted.
   EdgePairs edgepairs;
   if (CanContract(reader, tile, start_node, edgepairs)) {
-    return 0;
+    return {uint32_t(0), uint32_t(0)};
   }
 
   // Check if this is the last edge in a shortcut (if the endnode cannot be contracted).
@@ -359,6 +344,7 @@ uint32_t AddShortcutEdges(GraphReader& reader,
   // Iterate through directed edges of the base node
   uint32_t shortcut = 0;
   uint32_t shortcut_count = 0;
+  uint32_t total_edge_count = 0;
   GraphId edge_id(start_node.tileid(), start_node.level(), edge_index);
   for (uint32_t i = 0; i < edge_count; i++, ++edge_id) {
     // Skip transit connection edges.
@@ -376,6 +362,7 @@ uint32_t AddShortcutEdges(GraphReader& reader,
     // edge of the contracted node.
     GraphId end_node = directededge->endnode();
     if (IsEnteringEdgeOfContractedNode(reader, end_node, edge_id)) {
+      total_edge_count++;
       // Form a shortcut edge.
       DirectedEdge newedge = *directededge;
 
@@ -401,13 +388,6 @@ 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()) {
@@ -453,11 +433,9 @@ uint32_t AddShortcutEdges(GraphReader& reader,
         // off of shortcuts work properly
         ConnectEdges(reader, end_node, next_edge_id, shape, end_node, opp_local_idx, rst,
                      average_density, total_duration, total_truck_duration);
+        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);
@@ -466,13 +444,13 @@ 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);
 
@@ -554,16 +532,17 @@ uint32_t AddShortcutEdges(GraphReader& reader,
     LOG_WARN("Exceeding max shortcut edges from a node at LL = " + std::to_string(ll.lat()) + "," +
              std::to_string(ll.lng()));
   }
-  return shortcut_count;
+  return {shortcut_count, total_edge_count};
 }
 
 // Form shortcuts for tiles in this level.
-uint32_t FormShortcuts(GraphReader& reader, const TileLevel& level) {
+std::pair<uint32_t, uint32_t> FormShortcuts(GraphReader& reader, const TileLevel& level) {
   // Iterate through the tiles at this level (TODO - can we mark the tiles
   // the tiles that shortcuts end within?)
   reader.Clear();
   bool added = false;
   uint32_t shortcut_count = 0;
+  uint32_t total_edge_count = 0;
   uint32_t ntiles = level.tiles.TileCount();
   uint32_t tile_level = (uint32_t)level.level;
   graph_tile_ptr tile;
@@ -605,8 +584,10 @@ uint32_t FormShortcuts(GraphReader& reader, const TileLevel& level) {
 
       // Add shortcut edges first.
       std::unordered_map<uint32_t, uint32_t> shortcuts;
-      shortcut_count += AddShortcutEdges(reader, tile, tilebuilder, node_id, old_edge_index,
-                                         old_edge_count, shortcuts);
+      auto stats = AddShortcutEdges(reader, tile, tilebuilder, node_id, old_edge_index,
+                                    old_edge_count, shortcuts);
+      shortcut_count += stats.first;
+      total_edge_count += stats.second;
 
       // Copy the rest of the directed edges from this node
       GraphId edgeid(tileid, tile_level, old_edge_index);
@@ -711,7 +692,7 @@ uint32_t FormShortcuts(GraphReader& reader, const TileLevel& level) {
       reader.Trim();
     }
   }
-  return shortcut_count;
+  return {shortcut_count, total_edge_count};
 }
 
 } // namespace
@@ -736,8 +717,11 @@ void ShortcutBuilder::Build(const boost::property_tree::ptree& pt) {
   for (; tile_level != TileHierarchy::levels().rend(); ++tile_level) {
     // Create shortcuts on this level
     LOG_INFO("Creating shortcuts on level " + std::to_string(tile_level->level));
-    [[maybe_unused]] uint32_t count = FormShortcuts(reader, *tile_level);
-    LOG_INFO("Finished with " + std::to_string(count) + " shortcuts");
+    [[maybe_unused]] auto stats = FormShortcuts(reader, *tile_level);
+    [[maybe_unused]] uint32_t avg = stats.first ? (stats.second / stats.first) : 0;
+    LOG_INFO("Finished with " + std::to_string(stats.first) + " shortcuts superseding " +
+             std::to_string(stats.second) + " edges, average ~" + std::to_string(avg) +
+             " edges per shortcut");
   }
 }
 

test/CMakeLists.txt

@@ -42,7 +42,7 @@ set(tests aabb2 access_restriction actor admin attributes_controller configurati
   incident_loading worker_nullptr_tiles curl_tilegetter)
 
 if(ENABLE_DATA_TOOLS)
-  list(APPEND tests astar astar_bikeshare complexrestriction countryaccess edgeinfobuilder graphbuilder graphparser
+  list(APPEND tests astar_bikeshare complexrestriction countryaccess edgeinfobuilder graphbuilder graphparser
     graphtilebuilder graphreader isochrone predictive_traffic idtable mapmatch matrix matrix_bss minbb multipoint_routes
     names node_search reach recover_shortcut refs search servicedays shape_attributes signinfo summary urban tar_index
     thor_worker timedep_paths timeparsing trivial_paths uniquenames util_mjolnir utrecht lua alternates)
@@ -367,7 +367,7 @@ if(ENABLE_DATA_TOOLS)
   add_dependencies(run-recover_shortcut utrecht_tiles)
   add_dependencies(run-minbb utrecht_tiles)
   add_dependencies(run-astar_bikeshare paris_bss_tiles)
-  add_dependencies(run-astar whitelion_tiles roma_tiles reversed_whitelion_tiles bayfront_singapore_tiles ny_ar_tiles pa_ar_tiles nh_ar_tiles melborne_tiles utrecht_tiles)
+  #add_dependencies(run-astar whitelion_tiles roma_tiles reversed_whitelion_tiles bayfront_singapore_tiles ny_ar_tiles pa_ar_tiles nh_ar_tiles melborne_tiles utrecht_tiles)
   add_dependencies(run-alternates utrecht_tiles)
   add_dependencies(run-tar_index utrecht_tiles)
   add_dependencies(run-graphbuilder build_timezones)

test/astar.cc

@@ -1570,9 +1570,8 @@ TEST(BiDiAstar, test_recost_path) {
            3  4        5
   )";
   const gurka::ways ways = {
-      // make ABC to be a shortcut
+      // make ABCDE to be a shortcut
       {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}}},
-      // make CDE to be a shortcut
       {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}}},
       {"1A", {{"highway", "secondary"}}},
       {"B3", {{"highway", "secondary"}}},
@@ -1586,19 +1585,16 @@ TEST(BiDiAstar, test_recost_path) {
       {"E2", {{"highway", "secondary"}}},
   };
 
-  auto nodes = gurka::detail::map_to_coordinates(ascii_map, 500);
+  auto nodes = gurka::detail::map_to_coordinates(ascii_map, 5);
 
   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 ABC = gurka::findEdgeByNodes(graphreader, nodes, "A", "C");
-  ASSERT_TRUE(std::get<1>(ABC)->is_shortcut()) << "Expected ABC to be a shortcut";
-  // before continue check that CDE is actually a shortcut
-  const auto CDE = gurka::findEdgeByNodes(graphreader, nodes, "C", "E");
-  ASSERT_TRUE(std::get<1>(CDE)->is_shortcut()) << "Expected CDE to be 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";
 
   auto const set_constrained_speed = [&graphreader, &test_dir](const std::vector<GraphId>& edge_ids) {
     for (const auto& edgeid : edge_ids) {
@@ -1618,8 +1614,7 @@ TEST(BiDiAstar, test_recost_path) {
   };
   // set constrained speed for all superseded edges;
   // this speed will be used for them in costing model
-  set_constrained_speed(graphreader.RecoverShortcut(std::get<0>(ABC)));
-  set_constrained_speed(graphreader.RecoverShortcut(std::get<0>(CDE)));
+  set_constrained_speed(graphreader.RecoverShortcut(std::get<0>(ABCDE)));
   // reset cache to see updated speeds
   graphreader.Clear();
 

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 @@ void find_ids(baldr::GraphReader& reader, std::multiset<uint64_t> osm_way_ids) {
   // 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_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);
   }
 }