valhalla · nilsnolde · Apr 8, 2024 · Feb 24, 2024 · Feb 24, 2024 · Feb 24, 2024
diff --git a/.gitignore b/.gitignore
@@ -61,6 +61,7 @@ CMakeLists.txt.user
 .idea
 /.tidytmp
 vcpkg*/
+*.log
 
 # python
 .venv

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,9 @@
    * FIXED: unidirectional_astar.cc doesn't work for date_time type = 2 #4652(https://github.com/valhalla/valhalla/issues/4652)
    * FIXED: a few fixes around the routing algorithms [#4626](https://github.com/valhalla/valhalla/pull/4642)
    * FIXED: no need to search for GDAL when building data [#4651](https://github.com/valhalla/valhalla/pull/4651)
+   * 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)
@@ -85,7 +88,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: Adjustment 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)
@@ -94,21 +96,20 @@
    * 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)
    * ADDED: return isotile grid as geotiff  [#4594](https://github.com/valhalla/valhalla/pull/4594)
    * ADDED: `ignore_non_vehicular_restrictions` parameter for truck costing [#4606](https://github.com/valhalla/valhalla/pull/4606)
    * UPDATED: tz database to 2024a [#4643](https://github.com/valhalla/valhalla/pull/4643)
    * ADDED: `hgv_no_penalty` costing option to allow penalized truck access to `hgv=no` edges [#4650](https://github.com/valhalla/valhalla/pull/4650)
    * CHANGED: Significantly improve performance of graphbuilder [#4669](https://github.com/valhalla/valhalla/pull/4669)
    * UPDATED: Improved turn by turn api reference documentation [#4675](https://github.com/valhalla/valhalla/pull/4675)
+   * CHANGED: contract nodes if connecting edges have different names or speed or non-conditional access restrictions [#4613](https://github.com/valhalla/valhalla/pull/4613)
 
 ## Release Date: 2023-05-11 Valhalla 3.4.0
 * **Removed**

diff --git a/src/mjolnir/restrictionbuilder.cc b/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;
 };

diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc
@@ -29,6 +29,50 @@ using namespace valhalla::mjolnir;
 
 namespace {
 
+struct ShortcutAccessRestriction {
+  std::unordered_map<AccessType, AccessRestriction> all_restrictions;
+  // important to set the edge's attribute
+  uint64_t modes = 0;
+
+  ShortcutAccessRestriction(const std::vector<AccessRestriction>&& restrictions) {
+    for (const auto& res : restrictions) {
+      modes |= res.modes();
+      all_restrictions.emplace(res.type(), std::move(res));
+    }
+  };
+
+  // updates non-conditional restrictions if their value is lower than the current value
+  // TODO(nils): we could also contract over conditional restrictions with a bit more work:
+  //   kTimeDenied is fine to just append all restrictions of the base edges, but kTimeAllowed
+  //   will be harder, there we'll have to merge overlapping time periods
+  void update_nonconditional(const std::vector<AccessRestriction>&& other_restrictions) {
+    for (const auto& new_ar : other_restrictions) {
+      // update the modes for the edge attribute regardless
+      if (new_ar.type() == AccessType::kTimedAllowed || new_ar.type() == AccessType::kTimedDenied ||
+          new_ar.type() == AccessType::kDestinationAllowed) {
+        continue;
+      }
+      modes |= new_ar.modes();
+      auto ar_inserted = all_restrictions.emplace(new_ar.type(), new_ar);
+      if (!ar_inserted.second && new_ar.value() < ar_inserted.first->second.value()) {
+        ar_inserted.first->second = std::move(new_ar);
+      }
+    }
+  }
+};
+
+// only keeps access restrictions which can fail contraction
+void remove_nonconditional_restrictions(std::vector<AccessRestriction>& access_restrictions) {
+  access_restrictions.erase(std::remove_if(std::begin(access_restrictions),
+                                           std::end(access_restrictions),
+                                           [](const AccessRestriction& elem) {
+                                             return elem.type() != AccessType::kDestinationAllowed &&
+                                                    elem.type() != AccessType::kTimedAllowed &&
+                                                    elem.type() != AccessType::kTimedDenied;
+                                           }),
+                            std::end(access_restrictions));
+}
+
 // Simple structure to hold the 2 pair of directed edges at a node.
 // First edge in the pair is incoming and second is outgoing
 struct EdgePairs {
@@ -59,9 +103,9 @@ bool EdgesMatch(const graph_tile_ptr& tile, const DirectedEdge* edge1, const Dir
     return false;
   }
 
-  // Neither edge can be part of a complex turn restriction or differ on access restrictions
+  // Neither edge can be part of a complex turn restriction
   if (edge1->start_restriction() || edge1->end_restriction() || edge2->start_restriction() ||
-      edge2->end_restriction() || edge1->access_restriction() != edge2->access_restriction()) {
+      edge2->end_restriction()) {
     return false;
   }
 
@@ -77,20 +121,12 @@ 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()) {
+  // if there's conditional access restrictions, they must match; others we can safely contract over
+  if (edge1->access_restriction() || edge2->access_restriction()) {
     auto res1 = tile->GetAccessRestrictions(edge1 - tile->directededge(0), kVehicularAccess);
+    remove_nonconditional_restrictions(res1);
     auto res2 = tile->GetAccessRestrictions(edge2 - tile->directededge(0), kVehicularAccess);
+    remove_nonconditional_restrictions(res2);
     if (res1.size() != res2.size())
       return false;
     for (size_t i = 0; i < res1.size(); ++i) {
@@ -231,11 +267,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);
@@ -280,7 +311,8 @@ void ConnectEdges(GraphReader& reader,
                   uint32_t& restrictions,
                   float& average_density,
                   float& total_duration,
-                  float& total_truck_duration) {
+                  float& total_truck_duration,
+                  ShortcutAccessRestriction& access_restrictions) {
   // Get the tile and directed edge.
   auto tile = reader.GetGraphTile(startnode);
   const DirectedEdge* directededge = tile->directededge(edgeid);
@@ -324,6 +356,9 @@ void ConnectEdges(GraphReader& reader,
   // Add to the weighted average
   average_density += directededge->length() * directededge->density();
 
+  // Preserve the most restrictive access restrictions
+  access_restrictions.update_nonconditional(tile->GetAccessRestrictions(edgeid.id(), kAllAccess));
+
   // Update the end node
   endnode = directededge->endnode();
 }
@@ -403,22 +438,10 @@ 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()) {
-        auto restrictions = tile->GetAccessRestrictions(edge_id.id(), kAllAccess);
-        for (const auto& res : restrictions) {
-          tilebuilder.AddAccessRestriction(AccessRestriction(tilebuilder.directededges().size(),
-                                                             res.type(), res.modes(), res.value()));
-        }
-      }
+      // store all access_restrictions of the base edge: non-conditional ones will be updated while
+      // contracting, conditional ones are breaking contraction and are safe to simply copy
+      ShortcutAccessRestriction access_restrictions{
+          tile->GetAccessRestrictions(edge_id.id(), kAllAccess)};
 
       // Connect edges to the shortcut while the end node is marked as
       // contracted (contains edge pairs in the shortcut info).
@@ -454,13 +477,10 @@ std::pair<uint32_t, uint32_t> AddShortcutEdges(GraphReader& reader,
         // on the connected shortcut - need to set that so turn restrictions
         // 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);
+                     average_density, total_duration, total_truck_duration, access_restrictions);
         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 +489,14 @@ 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, {}, {}, {}, 0, forward, false);
+      ;
 
       newedge.set_edgeinfo_offset(edge_info_offset);
 
@@ -489,6 +510,18 @@ std::pair<uint32_t, uint32_t> AddShortcutEdges(GraphReader& reader,
       newedge.set_opp_local_idx(opp_local_idx);
       newedge.set_restrictions(rst);
 
+      // add new access restrictions if any and set the mask on the edge
+      if (access_restrictions.all_restrictions.size()) {
+        newedge.set_access_restriction(access_restrictions.modes);
+        for (const auto& res : access_restrictions.all_restrictions) {
+          tilebuilder.AddAccessRestriction(AccessRestriction(tilebuilder.directededges().size(),
+                                                             res.second.type(), res.second.modes(),
+                                                             res.second.value()));
+        }
+      }
+
+      // set new access mask
+
       // Update the length, curvature, and end node
       newedge.set_length(length);
       newedge.set_curvature(compute_curvature(shape));

diff --git a/test/astar.cc b/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) {

diff --git a/test/gurka/gurka.cc b/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,