From 6e1b34a59df8a2d5998677f8b032737998be4a44 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sat, 24 Feb 2024 16:28:33 +0100 Subject: [PATCH 01/18] make shortcut logging more useful --- src/mjolnir/shortcutbuilder.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index d9d36fbec0..6c3216cb1c 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -29,6 +29,9 @@ using namespace valhalla::mjolnir; namespace { +// total_shortcut_count and avg_edge_per_shortcut +using shortcut_stats = std::pair; + // 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 { @@ -427,6 +430,7 @@ std::pair AddShortcutEdges(GraphReader& reader, uint32_t opp_local_idx = directededge->opp_local_idx(); GraphId next_edge_id = edge_id; while (true) { + total_edge_count++; EdgePairs edgepairs; graph_tile_ptr tile = reader.GetGraphTile(end_node); if (last_edge(tile, end_node, edgepairs)) { From 0b36329aaa0ec1acfe733cd9009f1a575685024e Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sat, 24 Feb 2024 16:30:11 +0100 Subject: [PATCH 02/18] comment --- src/mjolnir/shortcutbuilder.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 6c3216cb1c..d69c0a3d1a 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -29,7 +29,7 @@ using namespace valhalla::mjolnir; namespace { -// total_shortcut_count and avg_edge_per_shortcut +// total_shortcut_count and total_superseded_edges using shortcut_stats = std::pair; // Simple structure to hold the 2 pair of directed edges at a node. @@ -752,7 +752,3 @@ void ShortcutBuilder::Build(const boost::property_tree::ptree& pt) { std::to_string(stats.second) + " edges, average ~" + std::to_string(avg) + " edges per shortcut"); } -} - -} // namespace mjolnir -} // namespace valhalla From 7fd1f390a081b14a3a794e0600736b2296f528cb Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sat, 24 Feb 2024 18:45:02 +0100 Subject: [PATCH 03/18] misplaced count --- src/mjolnir/shortcutbuilder.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index d69c0a3d1a..8ca02ccb00 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -430,7 +430,6 @@ std::pair AddShortcutEdges(GraphReader& reader, uint32_t opp_local_idx = directededge->opp_local_idx(); GraphId next_edge_id = edge_id; while (true) { - total_edge_count++; EdgePairs edgepairs; graph_tile_ptr tile = reader.GetGraphTile(end_node); if (last_edge(tile, end_node, edgepairs)) { From 38ddabadf7a3c1160b510e281732d0d4651ad922 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sat, 24 Feb 2024 20:35:37 +0100 Subject: [PATCH 04/18] address review --- src/mjolnir/shortcutbuilder.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 8ca02ccb00..e2400a3b23 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -29,9 +29,6 @@ using namespace valhalla::mjolnir; namespace { -// total_shortcut_count and total_superseded_edges -using shortcut_stats = std::pair; - // 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 { From f8d7e19006d29e6d215db4d833effd03b3715bdd Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sat, 24 Feb 2024 22:01:06 +0100 Subject: [PATCH 05/18] [skip ci] remove names and speed requirements from node contraction; even remove most of edgeinfo stuff for shortcuts, no need to store name, pronunciations etc; adapt all tests which were relying on the fact that names/speed break up shortcuts --- .gitignore | 1 + src/mjolnir/shortcutbuilder.cc | 31 ++----------- test/CMakeLists.txt | 4 +- test/astar.cc | 15 ++---- test/gurka/gurka.cc | 1 + test/gurka/test_64bit_wayid.cc | 12 +++-- test/gurka/test_gtfs.cc | 2 +- test/gurka/test_landmarks.cc | 3 ++ test/gurka/test_shortcut.cc | 51 ++++----------------- test/scripts/test_valhalla_build_extract.py | 6 +-- 10 files changed, 35 insertions(+), 91 deletions(-) diff --git a/.gitignore b/.gitignore index 036d2a39c6..a3b8b63caf 100644 --- a/.gitignore +++ b/.gitignore @@ -61,6 +61,7 @@ CMakeLists.txt.user .idea /.tidytmp vcpkg*/ +*.log # python .venv diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index e2400a3b23..51c74e1f82 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/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 edge1names = tile->GetNames(edge1); - std::vector 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 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 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 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); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 490d10d404..24bde26429 100644 --- a/test/CMakeLists.txt +++ b/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) @@ -360,7 +360,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) diff --git a/test/astar.cc b/test/astar.cc index f2868007fc..61f6b7a9f8 100644 --- a/test/astar.cc +++ b/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,7 +1585,7 @@ 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); @@ -1594,11 +1593,8 @@ TEST(BiDiAstar, test_recost_path) { 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& 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(); diff --git a/test/gurka/gurka.cc b/test/gurka/gurka.cc index 3a07c947f0..6e185b2b02 100644 --- 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 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); diff --git a/test/gurka/test_gtfs.cc b/test/gurka/test_gtfs.cc index aa43999789..7ad51bac8c 100644 --- a/test/gurka/test_gtfs.cc +++ b/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); diff --git a/test/gurka/test_landmarks.cc b/test/gurka/test_landmarks.cc index 53eece209c..c395b39d9a 100644 --- a/test/gurka/test_landmarks.cc +++ b/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(); diff --git a/test/gurka/test_shortcut.cc b/test/gurka/test_shortcut.cc index b16f357fc9..220f6a94e9 100644 --- a/test/gurka/test_shortcut.cc +++ b/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>; + + // 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); } } diff --git a/test/scripts/test_valhalla_build_extract.py b/test/scripts/test_valhalla_build_extract.py index a671a10199..8feacfeb8a 100644 --- a/test/scripts/test_valhalla_build_extract.py +++ b/test/scripts/test_valhalla_build_extract.py @@ -151,15 +151,15 @@ def test_create_extracts(self): tile_count = len(tile_resolver.matched_paths) # test that the index has the right offsets/sizes - exp_tuples = ((2560, 25568, 302472), (306688, 410441, 676208), (984576, 6549282, 6137768)) + exp_tuples = ((2560, 25568, 296680), (301056, 410441, 663208), (966144, 6549282, 6136952)) self.check_tar(EXTRACT_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE) # same for traffic.tar - exp_tuples = ((1536, 25568, 26416), (28672, 410441, 65312), (94720, 6549282, 604608)) + exp_tuples = ((1536, 25568, 25856), (28160, 410441, 64160), (93184, 6549282, 604608)) self.check_tar(TRAFFIC_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE) # tests the implementation using the tile_dir new_tile_extract = TILE_PATH.joinpath("tiles2.tar") - exp_tuples = ((2560, 25568, 302472), (306688, 410441, 676208), (984576, 6549282, 6137768)) + exp_tuples = ((2560, 25568, 296680), (301056, 410441, 663208), (966144, 6549282, 6136952)) tile_resolver = TileResolver(EXTRACT_PATH) tile_resolver.matched_paths = tile_resolver.normalized_tile_paths valhalla_build_extract.create_extracts(config, True, tile_resolver, new_tile_extract) From 50762160ca112c48648d3c29c7caf40462e232cd Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sat, 24 Feb 2024 22:28:29 +0100 Subject: [PATCH 06/18] changelog --- CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16af4e7693..e603148cbb 100644 --- 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: 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) @@ -94,18 +96,17 @@ * 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) + * 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** From a413d9a57460794d0d464fd4ccbdcc9252dd6e09 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sun, 25 Feb 2024 01:06:21 +0100 Subject: [PATCH 07/18] bidir astar is having a problem with 2 shortcuts between the same nodes: seems like it's randomly selecting one of the 2, no matter what the costing says.. --- src/mjolnir/restrictionbuilder.cc | 4 ++-- test/CMakeLists.txt | 4 ++-- test/astar.cc | 25 +++++++++++++++---------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/mjolnir/restrictionbuilder.cc b/src/mjolnir/restrictionbuilder.cc index 6c8c050a81..706c488a1c 100644 --- 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 restrictions; std::unordered_set part_of_restriction; }; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 24bde26429..490d10d404 100644 --- a/test/CMakeLists.txt +++ b/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_bikeshare complexrestriction countryaccess edgeinfobuilder graphbuilder graphparser + list(APPEND tests astar 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) @@ -360,7 +360,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) diff --git a/test/astar.cc b/test/astar.cc index 61f6b7a9f8..973d9138f9 100644 --- a/test/astar.cc +++ b/test/astar.cc @@ -1570,22 +1570,23 @@ TEST(BiDiAstar, test_recost_path) { 3 4 5 )"; const gurka::ways ways = { - // make ABCDE to be a shortcut - {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}}}, - {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}}}, + // make ABC to be a shortcut + {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}, {"maxweight", "3.5"}}}, + // make CDE to be a shortcut + {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}, {"maxweight", "7"}}}, {"1A", {{"highway", "secondary"}}}, {"B3", {{"highway", "secondary"}}}, {"C4", {{"highway", "secondary"}}}, {"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"}}}, - {"YE", {{"highway", "primary"}, {"maxspeed", "80"}}}, + {"AX", {{"highway", "primary"}, {"maxspeed", "70"}, {"maxweight", "3.5"}}}, + {"XY", {{"highway", "primary"}, {"maxspeed", "70"}, {"maxweight", "7"}}}, + {"YE", {{"highway", "primary"}, {"maxspeed", "80"}, {"maxweight", "12"}}}, {"E2", {{"highway", "secondary"}}}, }; - auto nodes = gurka::detail::map_to_coordinates(ascii_map, 5); + auto nodes = gurka::detail::map_to_coordinates(ascii_map, 1000); const std::string test_dir = "test/data/astar_shortcuts_recosting"; const auto map = gurka::buildtiles(nodes, ways, {}, {}, test_dir); @@ -1593,8 +1594,11 @@ TEST(BiDiAstar, test_recost_path) { 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"; + 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"; auto const set_constrained_speed = [&graphreader, &test_dir](const std::vector& edge_ids) { for (const auto& edgeid : edge_ids) { @@ -1614,7 +1618,8 @@ 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>(ABCDE))); + set_constrained_speed(graphreader.RecoverShortcut(std::get<0>(ABC))); + set_constrained_speed(graphreader.RecoverShortcut(std::get<0>(CDE))); // reset cache to see updated speeds graphreader.Clear(); From 09126a5e74ca0d05a6e2f2d3ddd829a9e391c2bd Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sun, 25 Feb 2024 01:40:21 +0100 Subject: [PATCH 08/18] add failing test but put it disabled --- test/astar.cc | 87 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/test/astar.cc b/test/astar.cc index 973d9138f9..fead28a980 100644 --- a/test/astar.cc +++ b/test/astar.cc @@ -1571,22 +1571,22 @@ TEST(BiDiAstar, test_recost_path) { )"; const gurka::ways ways = { // make ABC to be a shortcut - {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}, {"maxweight", "3.5"}}}, + {"ABC", {{"highway", "motorway"}, {"maxspeed", "80"}}}, // make CDE to be a shortcut - {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}, {"maxweight", "7"}}}, + {"CDE", {{"highway", "primary"}, {"maxspeed", "80"}}}, {"1A", {{"highway", "secondary"}}}, {"B3", {{"highway", "secondary"}}}, {"C4", {{"highway", "secondary"}}}, {"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"}, {"maxweight", "3.5"}}}, - {"XY", {{"highway", "primary"}, {"maxspeed", "70"}, {"maxweight", "7"}}}, - {"YE", {{"highway", "primary"}, {"maxspeed", "80"}, {"maxweight", "12"}}}, + {"AX", {{"highway", "motorway"}, {"maxspeed", "70"}}}, + {"XY", {{"highway", "trunk"}, {"maxspeed", "70"}}}, + {"YE", {{"highway", "primary"}, {"maxspeed", "80"}}}, {"E2", {{"highway", "secondary"}}}, }; - auto nodes = gurka::detail::map_to_coordinates(ascii_map, 1000); + 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); @@ -1679,6 +1679,81 @@ TEST(BiDiAstar, test_recost_path) { } } +// TODO(nils): this test fails currently, because bidir A* has a problem with 2 shortcuts between the +// same nodes +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 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 expected_names = {"1A", "AB", "BC", "CD", "DE", "E2"}; + std::vector 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) { From 4042e15c7dc505ee0c80203a510d43e02cd3359c Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sun, 25 Feb 2024 01:47:22 +0100 Subject: [PATCH 09/18] comment --- test/astar.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/astar.cc b/test/astar.cc index fead28a980..ba1f6a2097 100644 --- a/test/astar.cc +++ b/test/astar.cc @@ -1680,7 +1680,7 @@ TEST(BiDiAstar, test_recost_path) { } // TODO(nils): this test fails currently, because bidir A* has a problem with 2 shortcuts between the -// same nodes +// same nodes: https://github.com/valhalla/valhalla/issues/4609 TEST(BiDiAstar, DISABLED_test_recost_path_failing) { const std::string ascii_map = R"( X-----------Y From 39b9f30d54b9fb0a00366e423fc2ce96f14bd88f Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sun, 25 Feb 2024 02:41:14 +0100 Subject: [PATCH 10/18] roll back changes to name/pronunciation etc --- src/mjolnir/shortcutbuilder.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 51c74e1f82..21127d2391 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -388,6 +388,13 @@ std::pair 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()) { @@ -436,6 +443,9 @@ std::pair 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); @@ -450,7 +460,9 @@ std::pair AddShortcutEdges(GraphReader& reader, 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, {}, {}, {}, 0U, forward, false); + edgeinfo.speed_limit(), shape, names, tagged_values, linguistics, + types, forward, diff_names); + ; newedge.set_edgeinfo_offset(edge_info_offset); From 98293695f47ce73d8ccb84efb2323b68ed25f0b0 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Sun, 25 Feb 2024 14:54:26 +0100 Subject: [PATCH 11/18] remove names/pronunciations etc from shortcuts again --- src/mjolnir/shortcutbuilder.cc | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 21127d2391..530b293850 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -388,13 +388,6 @@ std::pair 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()) { @@ -443,9 +436,6 @@ std::pair 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); @@ -460,8 +450,7 @@ std::pair AddShortcutEdges(GraphReader& reader, 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); From e7843d8d8bf8cde701f10437396b6e9bd7632455 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Tue, 27 Feb 2024 14:00:24 +0100 Subject: [PATCH 12/18] extend shortcut creation further by contracting over vast majority of access restrictions --- CHANGELOG.md | 1 + src/mjolnir/shortcutbuilder.cc | 81 ++++++++++++++++++++++++++------ test/gurka/test_shortcut.cc | 85 ++++++++++++++++++++++++---------- 3 files changed, 127 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e603148cbb..d9bd789272 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -107,6 +107,7 @@ * 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) * CHANGED: contract nodes if connecting edges have different names or speed [#4608](https://github.com/valhalla/valhalla/pull/4608) + * CHANGED: contract nodes if connecting edges have 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/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 530b293850..4f9391ee5a 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -29,6 +29,45 @@ using namespace valhalla::mjolnir; namespace { +struct ShortcutAccessRestriction { + std::unordered_map all_restrictions; + + ShortcutAccessRestriction(const std::vector&& restrictions) { + for (const auto& res : restrictions) { + 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&& other_restrictions) { + for (const auto& new_ar : other_restrictions) { + if (new_ar.type() == AccessType::kTimedAllowed || new_ar.type() == AccessType::kTimedDenied || + new_ar.type() == AccessType::kDestinationAllowed) { + continue; + } + 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& 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 +98,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,10 +116,12 @@ bool EdgesMatch(const graph_tile_ptr& tile, const DirectedEdge* edge1, const Dir 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) { @@ -265,7 +306,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); @@ -309,6 +351,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(); } @@ -382,21 +427,20 @@ std::pair AddShortcutEdges(GraphReader& reader, // forward - reverse the shape so the edge info stored is forward for // the first added edge info auto edgeinfo = tile->edgeinfo(directededge); + std::string name; + auto names = edgeinfo.GetNames(); + if (names.size()) + name = names[0]; std::list shape = valhalla::midgard::decode7>(edgeinfo.encoded_shape()); if (!directededge->forward()) { std::reverse(shape.begin(), shape.end()); } - // 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). @@ -432,7 +476,7 @@ std::pair 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++; } @@ -465,6 +509,13 @@ std::pair AddShortcutEdges(GraphReader& reader, newedge.set_opp_local_idx(opp_local_idx); newedge.set_restrictions(rst); + // add new access restrictions + for (const auto& res : access_restrictions.all_restrictions) { + tilebuilder.AddAccessRestriction(AccessRestriction(tilebuilder.directededges().size(), + res.second.type(), res.second.modes(), + res.second.value())); + } + // Update the length, curvature, and end node newedge.set_length(length); newedge.set_curvature(compute_curvature(shape)); diff --git a/test/gurka/test_shortcut.cc b/test/gurka/test_shortcut.cc index 220f6a94e9..028fb6e1b5 100644 --- a/test/gurka/test_shortcut.cc +++ b/test/gurka/test_shortcut.cc @@ -333,58 +333,93 @@ TEST(Shortcuts, ShortcutsInBins) { TEST(Shortcuts, ShortcutRestrictions) { using node_pairs = std::vector>; - // the first line should produce only one shortcut, the second two + // the first line should produce only one HUGE shortcut, the second line one small one const std::string ascii_map = R"( A--B--C--D--E--F G--H--I--J--K--L )"; + std::map high_access_res = {{"highway", "motorway"}, {"hazmat", "yes"}, + {"maxweight", "30"}, {"maxheight", "6"}, + {"maxlength", "10"}, {"maxaxles", "10"}}; + std::map low_access_res = {{"highway", "motorway"}, {"hazmat", "no"}, + {"maxweight", "3"}, {"maxheight", "3"}, + {"maxlength", "4"}, {"maxaxles", "4"}}; + gurka::ways ways = { - {"AB", {{"highway", "motorway"}, {"name", "highway"}}}, - {"BC", {{"highway", "motorway"}, {"name", "highway"}}}, - {"CD", {{"highway", "motorway"}, {"name", "highway"}, {"maxweight", "3.5"}}}, - {"DE", {{"highway", "motorway"}, {"name", "highway"}, {"maxweight", "8"}}}, - {"EF", {{"highway", "motorway"}, {"name", "highway"}}}, - - {"GH", {{"highway", "motorway"}, {"name", "highway"}}}, - {"HI", {{"highway", "motorway"}, {"name", "highway"}, {"hazmat", "yes"}}}, - {"IJ", {{"highway", "motorway"}, {"name", "highway"}, {"hazmat", "yes"}}}, - {"JK", {{"highway", "motorway"}, {"name", "highway"}}}, - {"KL", {{"highway", "motorway"}, {"name", "highway"}}}, + {"AB", {{"highway", "motorway"}}}, + {"BC", {{"highway", "motorway"}}}, + {"CD", high_access_res}, + {"DE", low_access_res}, + {"EF", {{"highway", "motorway"}}}, + + {"GH", {{"highway", "motorway"}}}, + {"HI", {{"highway", "motorway"}, {"motorcar:conditional", "yes @ 00:00-07:00"}}}, + {"IJ", {{"highway", "motorway"}, {"motorcar:conditional", "no @ 00:00-07:00"}}}, + {"JK", {{"highway", "motorway"}}}, + {"KL", {{"highway", "motorway"}}}, }; const auto layout = gurka::detail::map_to_coordinates(ascii_map, 500); auto map = gurka::buildtiles(layout, ways, {}, {}, VALHALLA_BUILD_DIR "test/data/gurka_shortcut_restrictions"); baldr::GraphReader reader(map.config.get_child("mjolnir")); - // test we got the right shortcuts edges, implicitly means they were broken properly - for (const auto& pair : - node_pairs{{"A", "C"}, {"C", "A"}, {"H", "J"}, {"J", "H"}, {"J", "L"}, {"L", "J"}}) { + // test we got the right shortcuts edges for the second line of the map + // implicitly means they were broken properly + for (const auto& pair : node_pairs{{"A", "F"}, {"F", "A"}, {"J", "L"}, {"L", "J"}}) { const auto shortcut = gurka::findEdgeByNodes(reader, layout, pair.first, pair.second); EXPECT_TRUE(std::get<1>(shortcut)->is_shortcut()); - EXPECT_NEAR(std::get<1>(shortcut)->length(), 3000, 1); + } + + // test that the long shortcut has the strictest non-conditional access restrictions + const auto AF = gurka::findEdgeByNodes(reader, layout, "A", "F"); + const auto AF_res = + reader.GetGraphTile(std::get<0>(AF))->GetAccessRestrictions(std::get<0>(AF).id(), kAllAccess); + EXPECT_EQ(AF_res.size(), 5); + for (const auto& res : AF_res) { + uint64_t expected_value = 0; + switch (res.type()) { + case AccessType::kHazmat: + // should be false/0 + break; + case AccessType::kMaxWeight: + expected_value = strtoull(low_access_res["maxweight"].c_str(), nullptr, 10) * 100; + break; + case AccessType::kMaxHeight: + expected_value = strtoull(low_access_res["maxheight"].c_str(), nullptr, 10) * 100; + break; + case AccessType::kMaxLength: + expected_value = strtoull(low_access_res["maxlength"].c_str(), nullptr, 10) * 100; + break; + case AccessType::kMaxAxles: + expected_value = strtoull(low_access_res["maxaxles"].c_str(), nullptr, 10); + break; + default: + break; + } + EXPECT_EQ(res.value(), expected_value); } // test the right edges are really superseded by a shortcut // forward - for (const auto& end_node : {"B", "I", "K"}) { - const auto edge = gurka::findEdge(reader, layout, "highway", end_node); + for (const auto& pair : node_pairs{{"A", "B"}, {"J", "K"}}) { + const auto edge = gurka::findEdgeByNodes(reader, layout, pair.first, pair.second); EXPECT_NE(std::get<1>(edge)->superseded(), 0); } // reverse - for (const auto& end_node : {"C", "J", "L"}) { - const auto edge = gurka::findEdge(reader, layout, "highway", end_node); - EXPECT_NE(std::get<3>(edge)->superseded(), 0); + for (const auto& pair : node_pairs{{"F", "E"}, {"L", "K"}}) { + const auto edge = gurka::findEdgeByNodes(reader, layout, pair.first, pair.second); + EXPECT_NE(std::get<1>(edge)->superseded(), 0); } // test that without those restrictions we're still building all shortcuts // remove those access restrictions - ways["CD"].erase("maxweight"); - ways["DE"].erase("maxweight"); - ways["HI"].erase("hazmat"); - ways["IJ"].erase("hazmat"); + ways["CD"] = {{"highway", "motorway"}}; + ways["DE"] = {{"highway", "motorway"}}; + ways["HI"] = {{"highway", "motorway"}}; + ways["IJ"] = {{"highway", "motorway"}}; auto map2 = gurka::buildtiles(layout, ways, {}, {}, VALHALLA_BUILD_DIR "test/data/gurka_shortcut_without_restrictions"); baldr::GraphReader reader2(map2.config.get_child("mjolnir")); From d830e9255d69a3f46789625d007d291bab551994 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Wed, 28 Feb 2024 18:55:55 +0100 Subject: [PATCH 13/18] fix test for tile sizes --- test/scripts/test_valhalla_build_extract.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/scripts/test_valhalla_build_extract.py b/test/scripts/test_valhalla_build_extract.py index 8feacfeb8a..ae62f3d903 100644 --- a/test/scripts/test_valhalla_build_extract.py +++ b/test/scripts/test_valhalla_build_extract.py @@ -151,15 +151,15 @@ def test_create_extracts(self): tile_count = len(tile_resolver.matched_paths) # test that the index has the right offsets/sizes - exp_tuples = ((2560, 25568, 296680), (301056, 410441, 663208), (966144, 6549282, 6136952)) + exp_tuples = ((2560, 25568, 296768), (301056, 410441, 665624), (968704, 6549282, 6137080)) self.check_tar(EXTRACT_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE) # same for traffic.tar - exp_tuples = ((1536, 25568, 25856), (28160, 410441, 64160), (93184, 6549282, 604608)) + exp_tuples = ((1536, 25568, 25856), (28160, 410441, 64400), (93184, 6549282, 604608)) self.check_tar(TRAFFIC_PATH, exp_tuples, tile_count * INDEX_BIN_SIZE) # tests the implementation using the tile_dir new_tile_extract = TILE_PATH.joinpath("tiles2.tar") - exp_tuples = ((2560, 25568, 296680), (301056, 410441, 663208), (966144, 6549282, 6136952)) + exp_tuples = ((2560, 25568, 296768), (301056, 410441, 665624), (968704, 6549282, 6137080)) tile_resolver = TileResolver(EXTRACT_PATH) tile_resolver.matched_paths = tile_resolver.normalized_tile_paths valhalla_build_extract.create_extracts(config, True, tile_resolver, new_tile_extract) From cfac7e7a0b28e4e764098ebd8c0b4f8c7fd1e059 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Thu, 29 Feb 2024 13:42:04 +0100 Subject: [PATCH 14/18] add modes mask to the new shortcuts --- src/mjolnir/shortcutbuilder.cc | 23 ++++++++++++++++++----- valhalla/baldr/accessrestriction.h | 2 +- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 4f9391ee5a..45d70c7494 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -31,9 +31,12 @@ namespace { struct ShortcutAccessRestriction { std::unordered_map all_restrictions; + // important to set the edge's attribute + uint64_t modes = 0; ShortcutAccessRestriction(const std::vector&& restrictions) { for (const auto& res : restrictions) { + modes |= res.modes(); all_restrictions.emplace(res.type(), std::move(res)); } }; @@ -44,6 +47,8 @@ struct ShortcutAccessRestriction { // will be harder, there we'll have to merge overlapping time periods void update_nonconditional(const std::vector&& other_restrictions) { for (const auto& new_ar : other_restrictions) { + // update the modes for the edge attribute regardless + modes |= new_ar.modes(); if (new_ar.type() == AccessType::kTimedAllowed || new_ar.type() == AccessType::kTimedDenied || new_ar.type() == AccessType::kDestinationAllowed) { continue; @@ -442,6 +447,9 @@ std::pair AddShortcutEdges(GraphReader& reader, ShortcutAccessRestriction access_restrictions{ tile->GetAccessRestrictions(edge_id.id(), kAllAccess)}; + // access mask will be only the modes which are on all base edges + auto access_mask = directededge->forwardaccess(); + // Connect edges to the shortcut while the end node is marked as // contracted (contains edge pairs in the shortcut info). uint32_t rst = 0; @@ -509,13 +517,18 @@ std::pair AddShortcutEdges(GraphReader& reader, newedge.set_opp_local_idx(opp_local_idx); newedge.set_restrictions(rst); - // add new access restrictions - for (const auto& res : access_restrictions.all_restrictions) { - tilebuilder.AddAccessRestriction(AccessRestriction(tilebuilder.directededges().size(), - res.second.type(), res.second.modes(), - res.second.value())); + // 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/valhalla/baldr/accessrestriction.h b/valhalla/baldr/accessrestriction.h index 624a0b03f8..672bfe77e3 100644 --- a/valhalla/baldr/accessrestriction.h +++ b/valhalla/baldr/accessrestriction.h @@ -39,7 +39,7 @@ class AccessRestriction { /** * Get the modes impacted by access restriction. - * @return Returns a bit field of affected modes. + * @return Returns a bit mask of affected modes. */ uint32_t modes() const; From 7fe45cfeddd9a574c9b30d869854be93c8778d50 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Thu, 29 Feb 2024 13:47:57 +0100 Subject: [PATCH 15/18] forgot a variable --- src/mjolnir/shortcutbuilder.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 45d70c7494..29deb4f5b9 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -447,9 +447,6 @@ std::pair AddShortcutEdges(GraphReader& reader, ShortcutAccessRestriction access_restrictions{ tile->GetAccessRestrictions(edge_id.id(), kAllAccess)}; - // access mask will be only the modes which are on all base edges - auto access_mask = directededge->forwardaccess(); - // Connect edges to the shortcut while the end node is marked as // contracted (contains edge pairs in the shortcut info). uint32_t rst = 0; From 6f17a57d83de55b5ad990bb813e9d2d8ed31695e Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Mon, 4 Mar 2024 15:26:55 +0100 Subject: [PATCH 16/18] modes should be added only for non-conditional restrictions --- src/mjolnir/shortcutbuilder.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index 29deb4f5b9..e17062c054 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -48,11 +48,11 @@ struct ShortcutAccessRestriction { void update_nonconditional(const std::vector&& other_restrictions) { for (const auto& new_ar : other_restrictions) { // update the modes for the edge attribute regardless - modes |= new_ar.modes(); 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); From 33a76fdcebc4f3626a7603852ed62bb88361ad7b Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Mon, 25 Mar 2024 17:01:13 +0100 Subject: [PATCH 17/18] remove comment and update changelog --- CHANGELOG.md | 3 +-- src/mjolnir/shortcutbuilder.cc | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9bd789272..215ed3fb85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,8 +106,7 @@ * 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) - * CHANGED: contract nodes if connecting edges have different names or speed [#4608](https://github.com/valhalla/valhalla/pull/4608) - * CHANGED: contract nodes if connecting edges have non-conditional access restrictions [#4613](https://github.com/valhalla/valhalla/pull/4613) + * 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/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index e17062c054..ca702d0dc3 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -432,10 +432,6 @@ std::pair AddShortcutEdges(GraphReader& reader, // forward - reverse the shape so the edge info stored is forward for // the first added edge info auto edgeinfo = tile->edgeinfo(directededge); - std::string name; - auto names = edgeinfo.GetNames(); - if (names.size()) - name = names[0]; std::list shape = valhalla::midgard::decode7>(edgeinfo.encoded_shape()); if (!directededge->forward()) { From 4a61b008819564ca1664c9725cfeed0ec6a69157 Mon Sep 17 00:00:00 2001 From: nilsnolde Date: Mon, 25 Mar 2024 17:21:48 +0100 Subject: [PATCH 18/18] what happened there.. I think the rebasing tool tends to mess things up.. --- src/mjolnir/shortcutbuilder.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/mjolnir/shortcutbuilder.cc b/src/mjolnir/shortcutbuilder.cc index ca702d0dc3..7f9a9edaa3 100644 --- a/src/mjolnir/shortcutbuilder.cc +++ b/src/mjolnir/shortcutbuilder.cc @@ -781,3 +781,7 @@ void ShortcutBuilder::Build(const boost::property_tree::ptree& pt) { std::to_string(stats.second) + " edges, average ~" + std::to_string(avg) + " edges per shortcut"); } +} + +} // namespace mjolnir +} // namespace valhalla