Skip to content

Commit

Permalink
contract edges with differing non-conditional access restrictions (#4613
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@nilsnolde
nilsnolde committed Apr 8, 2024
1 parent 2b438ee commit fca9135
Show file tree
Hide file tree
Showing 12 changed files with 243 additions and 125 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Expand Up @@ -61,6 +61,7 @@ CMakeLists.txt.user
.idea
/.tidytmp
vcpkg*/
*.log

# python
.venv
Expand Down
7 changes: 4 additions & 3 deletions CHANGELOG.md
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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**
Expand Down
4 changes: 2 additions & 2 deletions src/mjolnir/restrictionbuilder.cc
View file
Expand Up @@ -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;
};
Expand Down
119 changes: 76 additions & 43 deletions src/mjolnir/shortcutbuilder.cc
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand All @@ -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));
Expand Down
81 changes: 78 additions & 3 deletions test/astar.cc
View file
Expand Up @@ -1571,7 +1571,7 @@ TEST(BiDiAstar, test_recost_path) {
)";
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"}}},
Expand All @@ -1580,8 +1580,8 @@ TEST(BiDiAstar, test_recost_path) {
{"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"}}},
};
Expand Down Expand Up @@ -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: 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) {
Expand Down
1 change: 1 addition & 0 deletions test/gurka/gurka.cc
View file
Expand Up @@ -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,
Expand Down

0 comments on commit fca9135

Please sign in to comment.