Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

SiarheiFedartsou
Copy link
Member

@SiarheiFedartsou SiarheiFedartsou commented Oct 27, 2022

Issue

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

Tasklist

Requirements / Relations

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


struct TrafficSignals final : public RoadObjects {};

// TODO: better naming ?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mjjbell mjjbell Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,39 @@
@routing @car @stop_sign
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Screenshot 2022-10-28 at 16 39 59

profiles/car.lua Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably be combined with the traffic signal functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks good 🟢

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything is possible in OSM...

@mjjbell
Copy link
Member

mjjbell commented Oct 28, 2022

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.

@@ -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",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to use common traffic_flow_control_direction enum instead of existing traffic_lights

Background:
Given the profile "car"

Scenario: Car - Encounters a give way sign
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

// represents traffic lights, stop signs, give way signs, etc.
struct TrafficFlowControlNodes
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SiarheiFedartsou SiarheiFedartsou changed the title Take stop signs into account during routing Take stop and give way signs into account during routing Oct 31, 2022
{
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
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@SiarheiFedartsou
Copy link
Member Author

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

Copy link
Member

@mjjbell mjjbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Background:
Given the profile "car"

Scenario: Car - Encounters a give way sign
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

#include "traffic_lights.hpp"
#include "traffic_flow_control_nodes.hpp"
#include <cstdint>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed?

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link

github-actions bot commented Jul 8, 2024

This PR seems to be stale. Is it still relevant?

@github-actions github-actions bot added the Stale label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants