From 6e77d5394656ae1afc80033d3aa7a9f78f74255f Mon Sep 17 00:00:00 2001 From: Monday <51102038+GitBenjamin@users.noreply.github.com> Date: Sun, 17 Mar 2024 19:32:10 +0800 Subject: [PATCH] Correctly handle compressed traffic signals (#6724) Unidirectional traffic signal segments are currently not compressed. This means traffic signals which are not on turns can be missed and not applied the correct penalty. This commit changes this behaviour to correctly handle the graph compression. Additional tests are added to ensure there is no regression for other cases (turns, restrictions). Co-authored-by: Michael Bell --- CHANGELOG.md | 1 + features/car/traffic_light_penalties.feature | 204 ++++++++++++++---- include/extractor/graph_compressor.hpp | 2 +- .../extractor/node_based_graph_factory.hpp | 4 +- include/extractor/traffic_signals.hpp | 15 ++ src/extractor/edge_based_graph_factory.cpp | 33 +-- src/extractor/graph_compressor.cpp | 5 +- src/extractor/node_based_graph_factory.cpp | 4 +- 8 files changed, 204 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4f432653..aec4f0d66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ - FIXED: Bicycle and foot profiles now don't route on proposed ways [#6615](https://github.com/Project-OSRM/osrm-backend/pull/6615) - Routing: - FIXED: Fix adding traffic signal penalties during compression [#6419](https://github.com/Project-OSRM/osrm-backend/pull/6419) + - FIXED: Correctly handle compressed traffic signals. [#6724](https://github.com/Project-OSRM/osrm-backend/pull/6724) # 5.27.1 - Changes from 5.27.0 - Misc: diff --git a/features/car/traffic_light_penalties.feature b/features/car/traffic_light_penalties.feature index 048c25f4f..2a5ded5f0 100644 --- a/features/car/traffic_light_penalties.feature +++ b/features/car/traffic_light_penalties.feature @@ -32,14 +32,14 @@ Feature: Car - Handle traffic lights | l | traffic_signals | When I route I should get - | from | to | time | # | + | from | to | time | # | | 1 | 2 | 11.1s | no turn with no traffic light | | 3 | 4 | 13.1s | no turn with traffic light | | g | j | 18.7s | turn with no traffic light | | k | n | 20.7s | turn with traffic light | - Scenario: Car - Traffic signal direction + Scenario: Car - Traffic signal direction straight Given the node map """ a-1-b-2-c @@ -112,14 +112,14 @@ Feature: Car - Handle traffic lights - Scenario: Car - Encounters a traffic light + Scenario: Car - Encounters a traffic light direction Given the node map """ - a f k - | | | - b-c-d h-g-i l-m-n - | | | - e j o + a f k p + | | | | + b-c-d h-g-i l-m-n q-r-s + | | | | + e j o t """ @@ -131,53 +131,70 @@ Feature: Car - Handle traffic lights | fgj | primary | | lmn | primary | | kmo | primary | + | qrs | primary | + | prt | primary | And the nodes | node | highway | traffic_signals:direction | - | g | traffic_signals | forward | - | m | traffic_signals | backward | + | g | traffic_signals | | + | m | traffic_signals | forward | + | r | traffic_signals | backward | When I route I should get + # Base case | from | to | time | # | - | a | d | 21.9s | no turn with no traffic light | - | a | e | 22.2s | no turn with traffic light | | a | b | 18.7s | turn with no traffic light | - | e | b | 21.9s | no turn with no traffic light | - | e | a | 22.2s | no turn with traffic light | + | a | e | 22.2s | no turn with no traffic light | + | a | d | 21.9s | turn with no traffic light | + | e | b | 21.9s | turn with no traffic light | + | e | a | 22.2s | no turn with no traffic light | | e | d | 18.7s | turn with no traffic light | - | d | e | 21.9s | no turn with no traffic light | - | d | b | 11s | no turn with traffic light | + | d | e | 21.9s | turn with no traffic light | + | d | b | 11s | no turn with no traffic light | | d | a | 18.7s | turn with no traffic light | - | b | a | 21.9s | no turn with no traffic light | - | b | d | 11s | no turn with traffic light | + | b | a | 21.9s | turn with no traffic light | + | b | d | 11s | no turn with no traffic light | | b | e | 18.7s | turn with no traffic light | - - | f | i | 23.9s | no turn with no traffic light | + # All have traffic lights - 2s penalty + | f | h | 20.7s | turn with traffic light | | f | j | 24.2s | no turn with traffic light | - | f | h | 20.7s | turn with no traffic light | - | j | h | 21.9s | no turn with no traffic light | - | j | f | 22.2s | no turn with traffic light | - | j | i | 18.7s | turn with no traffic light | - | i | j | 21.9s | no turn with no traffic light | - | i | h | 11s | no turn with traffic light | - | i | f | 18.7s | turn with no traffic light | - | h | f | 23.9s | no turn with no traffic light | + | f | i | 23.9s | turn with traffic light | + | j | h | 23.9s | turn with traffic light | + | j | f | 24.2s | no turn with traffic light | + | j | i | 20.7s | turn with traffic light | + | i | j | 23.9s | turn with traffic light | + | i | h | 13s | no turn with traffic light | + | i | f | 20.7s | turn with traffic light | + | h | f | 23.9s | turn with traffic light | | h | i | 13s | no turn with traffic light | - | h | j | 20.7s | turn with no traffic light | - - | k | n | 21.9s | no turn with no traffic light | - | k | o | 22.2s | no turn with traffic light | - | k | l | 18.7s | turn with no traffic light | - | o | l | 23.9s | no turn with no traffic light | - | o | k | 24.2s | no turn with traffic light | - | o | n | 20.7s | turn with no traffic light | - | n | o | 23.9s | no turn with no traffic light | - | n | l | 13s | no turn with traffic light | - | n | k | 20.7s | turn with no traffic light | - | l | k | 21.9s | no turn with no traffic light | - | l | n | 11s | no turn with traffic light | - | l | o | 18.7s | turn with no traffic light | + | h | j | 20.7s | turn with traffic light | + # Front direction have traffic lights - 2s penalty + | k | l | 20.7s | turn with traffic light | + | k | o | 24.2s | no turn with traffic light | + | k | n | 23.9s | turn with traffic light | + | o | l | 21.9s | turn with no traffic light | + | o | k | 22.2s | no turn with no traffic light | + | o | n | 18.7s | turn with no traffic light | + | n | o | 21.9s | turn with no traffic light | + | n | l | 11s | no turn with no traffic light | + | n | k | 18.7s | turn with no traffic light | + | l | k | 23.9s | turn with traffic light | + | l | n | 13s | no turn with traffic light | + | l | o | 20.7s | turn with traffic light | + # Reverse direction have traffic lights - 2s penalty + | p | q | 18.7s | turn with no traffic light | + | p | t | 22.2s | no turn with no traffic light | + | p | s | 21.9s | turn with no traffic light | + | t | q | 23.9s | turn with traffic light | + | t | p | 24.2s | no turn with traffic light | + | t | s | 20.7s | turn with traffic light | + | s | t | 23.9s | turn with traffic light | + | s | q | 13s | no turn with traffic light | + | s | p | 20.7s | turn with traffic light | + | q | p | 21.9s | turn with no traffic light | + | q | s | 11s | no turn with no traffic light | + | q | t | 18.7s | turn with no traffic light | Scenario: Traffic Signal Geometry @@ -343,3 +360,106 @@ Feature: Car - Handle traffic lights | from | to | route | speed | weights | time | distances | a:datasources | a:nodes | a:speed | a:duration | a:weight | | a | c | abc,abc | 65 km/h | 22.2,0 | 22.2s | 400m,0m | 1:0 | 1:2:3 | 18:18 | 11.1:11.1 | 11.1:11.1 | | c | a | abc,abc | 60 km/h | 24.2,0 | 24.2s | 400m,0m | 0:1 | 3:2:1 | 18:18 | 11.1:11.1 | 11.1:11.1 | + + + Scenario: Car - Traffic signal straight direction with edge compression + Given the node map + """ + a-1-b - c - d-2-e + + """ + + And the ways + | nodes | highway | + | abcde | primary | + + And the nodes + | node | highway | traffic_signals:direction | + | c | traffic_signals | forward | + + When I route I should get + | from | to | time | weight | # | + | 1 | 2 | 35.3s | 35.3 | no turn with traffic light | + | 2 | 1 | 33.3s | 33.3 | no turn with no traffic light | + + + Scenario: Car - Traffic signal turn direction with edge compression + Given the node map + """ + d + | + 2 + | + a-1-b - c - f + | + e + + j + | + 4 + | + g-3-h - i - k + | + l + + """ + + And the ways + | nodes | highway | + | abc | primary | + | cf | primary | + | fd | primary | + | fe | primary | + | ghi | primary | + | ik | primary | + | kj | primary | + | kl | primary | + + And the nodes + | node | highway | traffic_signals:direction | + | k | traffic_signals | forward | + + When I route I should get + | from | to | time | weight | # | + | 1 | 2 | 44.2s | 44.2 | turn with no traffic light | + | 2 | 1 | 41s | 41 | turn with no traffic light | + | 3 | 4 | 46.2s | 46.2 | turn with traffic light | + | 4 | 3 | 41s | 41 | turn with no traffic light | + + + Scenario: Car - Traffic signal turn direction with turn restriction + Given the node map + """ + d + | + 2 + | + a-1-b - c - f + | + e + + """ + + And the ways + | nodes | highway | + | abc | primary | + | cf | primary | + | fd | primary | + | fe | primary | + + And the nodes + | node | highway | traffic_signals:direction | + | f | traffic_signals | forward | + + And the relations + | type | way:from | way:to | way:via | restriction | + | restriction | abc | fe | cf | no_right_turn | + + And the relations + | type | way:from | way:to | node:via | restriction | + | restriction | df | fc | f | right_turn_only | + + When I route I should get + | from | to | time | weight | # | + | 1 | 2 | 46.2s | 46.2 | turn with traffic light | + | 2 | 1 | 41s | 41 | turn with no traffic light | diff --git a/include/extractor/graph_compressor.hpp b/include/extractor/graph_compressor.hpp index ba518e210..960bd64ff 100644 --- a/include/extractor/graph_compressor.hpp +++ b/include/extractor/graph_compressor.hpp @@ -24,7 +24,7 @@ class GraphCompressor public: void Compress(const std::unordered_set &barrier_nodes, - const TrafficSignals &traffic_signals, + TrafficSignals &traffic_signals, ScriptingEnvironment &scripting_environment, std::vector &turn_restrictions, std::vector &maneuver_overrides, diff --git a/include/extractor/node_based_graph_factory.hpp b/include/extractor/node_based_graph_factory.hpp index 29cc25ac4..c69842d3c 100644 --- a/include/extractor/node_based_graph_factory.hpp +++ b/include/extractor/node_based_graph_factory.hpp @@ -39,7 +39,7 @@ class NodeBasedGraphFactory NodeBasedGraphFactory(ScriptingEnvironment &scripting_environment, std::vector &turn_restrictions, std::vector &maneuver_overrides, - const TrafficSignals &traffic_signals, + TrafficSignals &traffic_signals, std::unordered_set &&barriers, std::vector &&coordinates, extractor::PackedOSMIDs &&osm_node_ids, @@ -71,7 +71,7 @@ class NodeBasedGraphFactory void Compress(ScriptingEnvironment &scripting_environment, std::vector &turn_restrictions, std::vector &maneuver_overrides, - const TrafficSignals &traffic_signals); + TrafficSignals &traffic_signals); // Most ways are bidirectional, making the geometry in forward and backward direction the same, // except for reversal. We make use of this fact by keeping only one representation of the diff --git a/include/extractor/traffic_signals.hpp b/include/extractor/traffic_signals.hpp index a70d67d54..739b57bcb 100644 --- a/include/extractor/traffic_signals.hpp +++ b/include/extractor/traffic_signals.hpp @@ -19,6 +19,21 @@ struct TrafficSignals { return bidirectional_nodes.count(to) > 0 || unidirectional_segments.count({from, to}) > 0; } + + void Compress(NodeID from, NodeID via, NodeID to) + { + bidirectional_nodes.erase(via); + if (unidirectional_segments.count({via, to})) + { + unidirectional_segments.erase({via, to}); + unidirectional_segments.insert({from, to}); + } + if (unidirectional_segments.count({via, from})) + { + unidirectional_segments.erase({via, from}); + unidirectional_segments.insert({to, from}); + } + } }; } // namespace osrm::extractor diff --git a/src/extractor/edge_based_graph_factory.cpp b/src/extractor/edge_based_graph_factory.cpp index 0e44276f2..cfb29e6c1 100644 --- a/src/extractor/edge_based_graph_factory.cpp +++ b/src/extractor/edge_based_graph_factory.cpp @@ -608,26 +608,12 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges( BOOST_ASSERT(!edge_data1.reversed); BOOST_ASSERT(!edge_data2.reversed); - // We write out the mapping between the edge-expanded edges and the original nodes. - // Since each edge represents a possible maneuver, external programs can use this to - // quickly perform updates to edge weights in order to penalize certain turns. - - // If this edge is 'trivial' -- where the compressed edge corresponds exactly to an - // original OSM segment -- we can pull the turn's preceding node ID directly with - // `node_along_road_entering`; - // otherwise, we need to look up the node immediately preceding the turn from the - // compressed edge container. - const bool isTrivial = m_compressed_edge_container.IsTrivial(node_based_edge_from); - - const auto &from_node = - isTrivial ? node_along_road_entering - : m_compressed_edge_container.GetLastEdgeSourceID(node_based_edge_from); - // compute weight and duration penalties // In theory we shouldn't get a directed traffic light on a turn, as it indicates that // the traffic signal direction was potentially ambiguously annotated on the junction // node But we'll check anyway. - const auto is_traffic_light = m_traffic_signals.HasSignal(from_node, intersection_node); + const auto is_traffic_light = + m_traffic_signals.HasSignal(node_along_road_entering, intersection_node); const auto is_uturn = guidance::getTurnDirection(turn_angle) == guidance::DirectionModifier::UTurn; @@ -694,6 +680,21 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges( true, false}; + // We write out the mapping between the edge-expanded edges and the original nodes. + // Since each edge represents a possible maneuver, external programs can use this to + // quickly perform updates to edge weights in order to penalize certain turns. + + // If this edge is 'trivial' -- where the compressed edge corresponds exactly to an + // original OSM segment -- we can pull the turn's preceding node ID directly with + // `node_along_road_entering`; + // otherwise, we need to look up the node immediately preceding the turn from the + // compressed edge container. + const bool isTrivial = m_compressed_edge_container.IsTrivial(node_based_edge_from); + + const auto &from_node = + isTrivial ? node_along_road_entering + : m_compressed_edge_container.GetLastEdgeSourceID(node_based_edge_from); + const auto &to_node = m_compressed_edge_container.GetFirstEdgeTargetID(node_based_edge_to); diff --git a/src/extractor/graph_compressor.cpp b/src/extractor/graph_compressor.cpp index 9c933ef4f..cee5afe92 100644 --- a/src/extractor/graph_compressor.cpp +++ b/src/extractor/graph_compressor.cpp @@ -20,7 +20,7 @@ namespace osrm::extractor static constexpr int SECOND_TO_DECISECOND = 10; void GraphCompressor::Compress(const std::unordered_set &barrier_nodes, - const TrafficSignals &traffic_signals, + TrafficSignals &traffic_signals, ScriptingEnvironment &scripting_environment, std::vector &turn_restrictions, std::vector &maneuver_overrides, @@ -338,6 +338,9 @@ void GraphCompressor::Compress(const std::unordered_set &barrier_nodes, // update any involved turn relations turn_path_compressor.Compress(node_u, node_v, node_w); + // Update traffic signal paths containing compressed node. + traffic_signals.Compress(node_u, node_v, node_w); + // Forward and reversed compressed edge lengths need to match. // Set a dummy empty penalty weight if opposite value exists. auto set_dummy_penalty = [](EdgeWeight &weight_penalty, diff --git a/src/extractor/node_based_graph_factory.cpp b/src/extractor/node_based_graph_factory.cpp index 9819a9778..045e49543 100644 --- a/src/extractor/node_based_graph_factory.cpp +++ b/src/extractor/node_based_graph_factory.cpp @@ -17,7 +17,7 @@ NodeBasedGraphFactory::NodeBasedGraphFactory( ScriptingEnvironment &scripting_environment, std::vector &turn_restrictions, std::vector &maneuver_overrides, - const TrafficSignals &traffic_signals, + TrafficSignals &traffic_signals, std::unordered_set &&barriers, std::vector &&coordinates, extractor::PackedOSMIDs &&osm_node_ids, @@ -72,7 +72,7 @@ void NodeBasedGraphFactory::BuildCompressedOutputGraph(const std::vector &turn_restrictions, std::vector &maneuver_overrides, - const TrafficSignals &traffic_signals) + TrafficSignals &traffic_signals) { GraphCompressor graph_compressor; graph_compressor.Compress(barriers,