Take stop and give way signs into account during routing #6426

SiarheiFedartsou · 2022-10-27T19:39:45Z

Issue

Done mainly using #6153, d1b9dff and #4828 (which also handles stop=all which I don't do here) as a reference

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-10-27T19:49:35Z

include/extractor/traffic_signals.hpp

+
+struct TrafficSignals final : public RoadObjects {};
+
+// TODO: better naming ?


It feels like we can try to generalise the whole this logic to any kind of objects which follows the same "idea" as traffic lights, i.e. some nodes which can be directed and give additional penalty in routing.

Maybe TrafficFlowControl?

If we view them as node penalties applicable by traversal direction, then I can see them being part of the same logical grouping.

SiarheiFedartsou · 2022-10-27T19:54:22Z

features/car/stop_sign_penalties.feature

@@ -0,0 +1,39 @@
+@routing @car @stop_sign


Hi @mjjbell !

May I ask you to take a look here?
What I actually did is took our existing traffic lights logic and completely repeated it for stop signs. There is a lot of things to be done here(at least we need to get rid of copy-paste), but before starting doing it, would be great to have your opinion here. Don't you see any fundamental problems in this approach? I am not very familiar with OSRM internals yet(one of the goals of this PR is to educate myself :) ) and tbh some parts of PR were "blindly" copy-pasted from traffic lights, so asking.

Yes if we're viewing all of them as node penalties applicable by direction, then it's basically the same code.

If we wanted to model the impact of each sign/traffic light more accurately (e.g. junction size, unprotected turns, all the other stuff in #2652, etc) then we'd probably want to handle them differently, but I don't think we're at that stage yet.

mjjbell · 2022-10-28T14:35:08Z

include/extractor/traffic_lights.hpp

@@ -19,6 +20,36 @@ enum Direction
 };
 } // namespace TrafficLightClass

+// Stop Signs tagged on nodes can be present or not. In addition Stop Signs have
+// an optional way direction they apply to. If the direction is unknown from the
+// data we have to compute by checking the distance to the next intersection.


Hmm, looks like identifying the direction will require something additional to the traffic signal logic.

If the direction is unknown from the data we have to compute by checking the distance to the next intersection.

Do you mean this sentence? Tbh it was a copy-paste from some of older PRs and I noticed it only now...

It looks like ~77% of stop signs tagged with direction.

I will take a look what it will take to compute this "distance to the nearest intersection" - should be just a couple of heuristics, but not sure how to do that from code point of view, but if it is TOO difficult to be part of this PR I would probably propose to just ignore stop signs without direction for the time being and then return to it in separate PRs.

https://taginfo.openstreetmap.org/tags/highway=stop#combinations

mjjbell · 2022-10-28T14:35:34Z

profiles/car.lua

@@ -472,8 +480,12 @@ function process_turn(profile, turn)

 if turn.has_traffic_light then
 turn.duration = profile.properties.traffic_light_penalty
+ elseif turn.has_stop_sign then
+ -- TODO: use another constant


mjjbell · 2022-10-28T14:36:26Z

src/extractor/extraction_containers.cpp

@@ -979,6 +982,54 @@ void ExtractionContainers::PrepareTrafficSignals(
 log << "ok, after " << TIMER_SEC(prepare_traffic_signals) << "s";
 }

+// TODO: copy-paste
+void ExtractionContainers::PrepareStopSigns(const ReferencedStopSigns &referenced_stop_signs) {


Can probably be combined with the traffic signal functions.

Sure, I am going to refactor it a lot, just want to have preliminary "green light" about approach.

Yep, looks good 🟢

mjjbell · 2022-10-28T14:37:28Z

src/extractor/extraction_containers.cpp

+ // If a node is appearing multiple times in this map, then it's ambiguous.
+ // The node is an intersection and the traffic direction is being use for multiple
+ // ways. We can't be certain of the original intent. See:
+ // https://wiki.openstreetmap.org/wiki/Key:traffic_signals:direction


Might not be ambiguous for stop signs, would have to see what the OSM consensus is.

mjjbell · 2022-10-28T14:38:11Z

src/extractor/graph_compressor.cpp

+ const bool has_forward_stop_sign = stop_signs.HasSignal(node_u, node_v);
+ const bool has_reverse_stop_sign = stop_signs.HasSignal(node_w, node_v);
+
+ // TODO: can we have a case when we have both traffic signal and stop sign? how should we handle it?


Anything is possible in OSM...

mjjbell · 2022-10-28T14:45:58Z

Perhaps the other thing to consider is the navigation guidance. I didn't really think about it in #6153 as traffic signals already existed. Adding stop/give-way signs is something that the guidance code should be aware of.

In fact, given I didn't change any of it, I suspect it doesn't work with traffic signal directions either.

SiarheiFedartsou · 2022-10-31T21:02:59Z

src/extractor/scripting_environment_lua.cpp

@@ -284,17 +284,26 @@ void Sol2ScriptingEnvironment::InitContext(LuaScriptingContext &context)
 [&context, &get_location_tag](const osmium::Node &node, const char *key) {
 return get_location_tag(context, node.location(), key);
 });
-
+ // just for backward compatibility
+ // TODO: remove in v6
 context.state.new_enum("traffic_lights",


I propose to use common traffic_flow_control_direction enum instead of existing traffic_lights

SiarheiFedartsou · 2022-10-31T21:07:03Z

features/car/give_way_sign_penalties.feature

+ Background:
+ Given the profile "car"
+
+ Scenario: Car - Encounters a give way sign


It is almost copy-paste of existing tests for traffic lights

osrm-backend/features/car/traffic_light_penalties.feature

Line 7 in afe3445

Scenario: Car - Encounters a traffic light

Probably worth having here all tests we have for traffic lights, but is there way to reduce copy-paste here somehow? E.g. somehow run the same test multiple times, but with different highway tag values for each run?

I expect there's some support for parametrization of tests, but we're on an ancient Cucumber version, so might take a lot of effort. Plus, having a separate file per penalty feature will allow the functionality to diverge if required.

SiarheiFedartsou · 2022-10-31T21:09:42Z

include/extractor/traffic_flow_control_nodes.hpp

+};
+
+// represents traffic lights, stop signs, give way signs, etc.
+struct TrafficFlowControlNodes


Not sure about naming here, if there are better ideas will be happy to discuss.

Can we try to re-use the same mechanism in the future for example for railway crossings? https://wiki.openstreetmap.org/wiki/Tag:railway%3Dlevel_crossing

SiarheiFedartsou · 2022-10-31T21:11:33Z

profiles/car.lua

@@ -28,6 +30,8 @@ function setup()
 use_turn_restrictions = true,
 left_hand_driving = false,
 traffic_light_penalty = 2,
+ stop_sign_penalty = 2,


Just from own driving experience: stop signs are quite annoying and often you have to stay as much as on traffic lights, but "give ways" are usually more relaxed

SiarheiFedartsou · 2022-10-31T21:14:45Z

profiles/car.lua

@@ -471,9 +481,14 @@ function process_turn(profile, turn)
 local turn_bias = turn.is_left_hand_driving and 1. / profile.turn_bias or profile.turn_bias

 if turn.has_traffic_light then


Initially I had a question "what to do if node has traffic light and stop sign at the same time?", IMO the answer is quite simple: here we just apply one of penalties, but end users may go further and apply combination of penalties in this case if they want or change their priority.

I think applying the max penalty out of the three would make sense to me. This logic achieves that too.

SiarheiFedartsou · 2022-10-31T21:18:45Z

src/extractor/extractor_callbacks.cpp

 {
 external_memory.external_traffic_signals.push_back({id, result_node.traffic_lights});
 }
+ // TODO: we ignore `ALL` for both stop signs and give way signs, because we cannot understand


I have an idea how to relatively simply implement it(at least for vast majority of cases), but I'd propose to do in separate PR.

Why can't we understand all-way stop/give-ways?

Can we treat them the same as traffic lights without direction?

SiarheiFedartsou · 2022-11-08T21:37:38Z

@mjjbell @DennisOSRM can we may be make second round of review here?

mjjbell

LGTM 👍

mjjbell · 2023-08-20T16:34:59Z

features/car/give_way_sign_penalties.feature

+ Background:
+ Given the profile "car"
+
+ Scenario: Car - Encounters a give way sign


I expect there's some support for parametrization of tests, but we're on an ancient Cucumber version, so might take a lot of effort. Plus, having a separate file per penalty feature will allow the functionality to diverge if required.

mjjbell · 2023-08-20T16:36:52Z

include/extractor/extraction_node.hpp

@@ -1,7 +1,8 @@
 #ifndef EXTRACTION_NODE_HPP
 #define EXTRACTION_NODE_HPP

-#include "traffic_lights.hpp"
+#include "traffic_flow_control_nodes.hpp"
+#include <cstdint>


mjjbell · 2023-08-20T16:42:20Z

profiles/car.lua

@@ -471,9 +481,14 @@ function process_turn(profile, turn)
 local turn_bias = turn.is_left_hand_driving and 1. / profile.turn_bias or profile.turn_bias

 if turn.has_traffic_light then


I think applying the max penalty out of the three would make sense to me. This logic achieves that too.

mjjbell · 2023-08-20T16:43:20Z

src/extractor/extraction_containers.cpp

@@ -1224,7 +1232,7 @@ ExtractionContainers::ReferencedTrafficSignals ExtractionContainers::IdentifyTra
 util::for_each_indexed(ways_list.cbegin(), ways_list.cend(), set_segments);

 util::for_each_pair(
- signal_segments, [](const auto pair_a, const auto pair_b) {
+ node_segments, [](const auto pair_a, const auto pair_b) {


Minor: Warning message below could be updated to refer to traffic control flow nodes.

mjjbell · 2023-08-20T16:50:42Z

src/extractor/extractor_callbacks.cpp

 {
 external_memory.external_traffic_signals.push_back({id, result_node.traffic_lights});
 }
+ // TODO: we ignore `ALL` for both stop signs and give way signs, because we cannot understand


Why can't we understand all-way stop/give-ways?

Can we treat them the same as traffic lights without direction?

SiarheiFedartsou added 2 commits October 27, 2022 21:13

wip

8a4af59

Take stop signs into account during routing

7f635f2

SiarheiFedartsou commented Oct 27, 2022

View reviewed changes

Take stop signs into account during routing

72ea34f

mjjbell reviewed Oct 28, 2022

View reviewed changes

SiarheiFedartsou added 14 commits October 28, 2022 22:04

Take stop signs into account during routing

36f3a5e

Take stop signs into account during routing

3558ec9

Take stop signs into account during routing

4432756

Take stop signs into account during routing

350862d

Take stop signs into account during routing

9788317

Take stop signs into account during routing

c28a683

Take stop signs into account during routing

62298cf

Take stop signs into account during routing

be1b866

Take stop signs into account during routing

b887e72

Merge branch 'master' into sf-stop-sign

eb72ca4

Take stop signs into account during routing

a7142ee

Take stop signs into account during routing

3080be5

Take stop signs into account during routing

ee008e7

Take stop signs into account during routing

7508262

SiarheiFedartsou commented Oct 31, 2022

View reviewed changes

SiarheiFedartsou changed the title ~~Take stop signs into account during routing~~ Take stop and give way signs into account during routing Oct 31, 2022

SiarheiFedartsou commented Oct 31, 2022

View reviewed changes

SiarheiFedartsou added 2 commits October 31, 2022 22:22

Take stop signs into account during routing

3f7a629

Take stop signs into account during routing

04ee126

SiarheiFedartsou marked this pull request as ready for review October 31, 2022 22:39

SiarheiFedartsou requested a review from mjjbell October 31, 2022 22:39

SiarheiFedartsou added the Ready for Review label Oct 31, 2022

DennisOSRM approved these changes Dec 3, 2022

View reviewed changes

mjjbell approved these changes Aug 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take stop and give way signs into account during routing #6426

Take stop and give way signs into account during routing #6426

SiarheiFedartsou commented Oct 27, 2022 •

edited

SiarheiFedartsou Oct 27, 2022

mjjbell Oct 28, 2022 •

edited

SiarheiFedartsou Oct 27, 2022 •

edited

mjjbell Oct 28, 2022

mjjbell Oct 28, 2022

SiarheiFedartsou Oct 28, 2022

mjjbell Oct 28, 2022

SiarheiFedartsou Oct 28, 2022

mjjbell Oct 28, 2022

mjjbell Oct 28, 2022

SiarheiFedartsou Oct 28, 2022

mjjbell Oct 28, 2022

mjjbell Oct 28, 2022

mjjbell Oct 28, 2022

mjjbell commented Oct 28, 2022 •

edited

SiarheiFedartsou Oct 31, 2022

SiarheiFedartsou Oct 31, 2022 •

edited

mjjbell Aug 20, 2023

SiarheiFedartsou Oct 31, 2022

SiarheiFedartsou Oct 31, 2022

SiarheiFedartsou Oct 31, 2022

SiarheiFedartsou Oct 31, 2022

mjjbell Aug 20, 2023

SiarheiFedartsou Oct 31, 2022 •

edited

mjjbell Aug 20, 2023

SiarheiFedartsou commented Nov 8, 2022

mjjbell left a comment •

edited

mjjbell Aug 20, 2023

mjjbell Aug 20, 2023

mjjbell Aug 20, 2023

mjjbell Aug 20, 2023

mjjbell Aug 20, 2023


		struct TrafficSignals final : public RoadObjects {};

		// TODO: better naming ?

		@@ -471,9 +481,14 @@ function process_turn(profile, turn)
		local turn_bias = turn.is_left_hand_driving and 1. / profile.turn_bias or profile.turn_bias

		if turn.has_traffic_light then

Take stop and give way signs into account during routing #6426

Are you sure you want to change the base?

Take stop and give way signs into account during routing #6426

Conversation

SiarheiFedartsou commented Oct 27, 2022 • edited

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

mjjbell Oct 28, 2022 • edited

Choose a reason for hiding this comment

SiarheiFedartsou Oct 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjjbell commented Oct 28, 2022 • edited

Choose a reason for hiding this comment

SiarheiFedartsou Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiarheiFedartsou Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Nov 8, 2022

mjjbell left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Oct 27, 2022 •

edited

mjjbell Oct 28, 2022 •

edited

SiarheiFedartsou Oct 27, 2022 •

edited

mjjbell commented Oct 28, 2022 •

edited

SiarheiFedartsou Oct 31, 2022 •

edited

SiarheiFedartsou Oct 31, 2022 •

edited

mjjbell left a comment •

edited