-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
8a4af59
7f635f2
72ea34f
36f3a5e
3558ec9
4432756
350862d
9788317
c28a683
62298cf
be1b866
b887e72
eb72ca4
a7142ee
3080be5
ee008e7
7508262
3f7a629
04ee126
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
@routing @car @stop_sign | ||
Feature: Car - Handle stop signs | ||
|
||
Background: | ||
Given the profile "car" | ||
|
||
Scenario: Car - Encounters a stop sign | ||
Given the node map | ||
""" | ||
a-1-b-2-c | ||
|
||
d-3-e-4-f | ||
|
||
g-h-i k-l-m | ||
| | | ||
j n | ||
|
||
""" | ||
|
||
And the ways | ||
| nodes | highway | | ||
| abc | primary | | ||
| def | primary | | ||
| ghi | primary | | ||
| klm | primary | | ||
| hj | primary | | ||
| ln | primary | | ||
|
||
And the nodes | ||
| node | highway | | ||
| e | stop | | ||
| l | stop | | ||
|
||
When I route I should get | ||
| from | to | time | # | | ||
| 1 | 2 | 11.1s | no turn with no stop sign | | ||
| 3 | 4 | 13.1s | no turn with stop sign | | ||
| g | j | 18.7s | turn with no stop sign | | ||
| k | n | 20.7s | turn with stop sign | |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,22 +2,30 @@ | |
#define EXTRACTION_NODE_HPP | ||
|
||
#include "traffic_lights.hpp" | ||
#include <cstdint> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed? |
||
|
||
namespace osrm | ||
{ | ||
namespace extractor | ||
{ | ||
|
||
|
||
struct ExtractionNode | ||
{ | ||
ExtractionNode() : traffic_lights(TrafficLightClass::NONE), barrier(false) {} | ||
void clear() | ||
{ | ||
traffic_lights = TrafficLightClass::NONE; | ||
stop_sign = StopSign::Direction::NONE; | ||
give_way = GiveWay::Direction::NONE; | ||
barrier = false; | ||
} | ||
TrafficLightClass::Direction traffic_lights; | ||
bool barrier; | ||
|
||
|
||
StopSign::Direction stop_sign; | ||
GiveWay::Direction give_way; | ||
}; | ||
} // namespace extractor | ||
} // namespace osrm | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
#ifndef OSRM_EXTRACTOR_TRAFFIC_LIGHTS_DATA_HPP_ | ||
#define OSRM_EXTRACTOR_TRAFFIC_LIGHTS_DATA_HPP_ | ||
|
||
#include <cstdint> | ||
namespace osrm | ||
{ | ||
namespace extractor | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean this sentence? Tbh it was a copy-paste from some of older PRs and I noticed it only now... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// | ||
// Impl. detail: namespace + enum instead of enum class to make Luabind happy | ||
namespace StopSign | ||
{ | ||
enum Direction : std::uint8_t | ||
{ | ||
NONE = 0, | ||
DIRECTION_ALL = 1, | ||
DIRECTION_FORWARD = 2, | ||
DIRECTION_REVERSE = 3 | ||
}; | ||
} | ||
|
||
// Give Way is the complement to priority roads. Tagging is the same as Stop Signs. | ||
// See explanation above. | ||
namespace GiveWay | ||
{ | ||
enum Direction : std::uint8_t | ||
{ | ||
NONE = 0, | ||
DIRECTION_ALL = 1, | ||
DIRECTION_FORWARD = 2, | ||
DIRECTION_REVERSE = 3 | ||
}; | ||
} | ||
|
||
|
||
} // namespace extractor | ||
} // namespace osrm | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,8 @@ namespace osrm | |
namespace extractor | ||
{ | ||
|
||
struct TrafficSignals | ||
// TODO: better naming | ||
struct RoadObjects | ||
{ | ||
std::unordered_set<NodeID> bidirectional_nodes; | ||
std::unordered_set<std::pair<NodeID, NodeID>, boost::hash<std::pair<NodeID, NodeID>>> | ||
|
@@ -22,6 +23,14 @@ struct TrafficSignals | |
return bidirectional_nodes.count(to) > 0 || unidirectional_segments.count({from, to}) > 0; | ||
} | ||
}; | ||
|
||
struct TrafficSignals final : public RoadObjects {}; | ||
|
||
// TODO: better naming ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe If we view them as node penalties applicable by traversal direction, then I can see them being part of the same logical grouping. |
||
struct StopSigns final : public RoadObjects {}; | ||
struct GiveWaySigns final : public RoadObjects {}; | ||
|
||
|
||
} // namespace extractor | ||
} // namespace osrm | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ Sequence = require('lib/sequence') | |
Handlers = require("lib/way_handlers") | ||
Relations = require("lib/relations") | ||
TrafficSignal = require("lib/traffic_signal") | ||
StopSign = require("lib/stop_sign") | ||
GiveWay = require("lib/give_way") | ||
find_access_tag = require("lib/access").find_access_tag | ||
limit = require("lib/maxspeed").limit | ||
Utils = require("lib/utils") | ||
|
@@ -362,6 +364,12 @@ function process_node(profile, node, result, relations) | |
|
||
-- check if node is a traffic light | ||
result.traffic_lights = TrafficSignal.get_value(node) | ||
|
||
-- check if node is stop sign | ||
result.stop_sign = StopSign.get_value(node) | ||
|
||
-- check if node is a give way sign | ||
result.give_way = GiveWay.get_value(node) | ||
end | ||
|
||
function process_way(profile, way, result, relations) | ||
|
@@ -472,8 +480,12 @@ function process_turn(profile, turn) | |
|
||
if turn.has_traffic_light then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
turn.duration = profile.properties.traffic_light_penalty | ||
elseif turn.has_stop_sign then | ||
-- TODO: use another constant | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
turn.duration = profile.properties.traffic_light_penalty | ||
end | ||
|
||
|
||
if turn.number_of_roads > 2 or turn.source_mode ~= turn.target_mode or turn.is_u_turn then | ||
if turn.angle >= 0 then | ||
turn.duration = turn.duration + turn_penalty / (1 + math.exp( -((13 / turn_bias) * turn.angle/180 - 6.5*turn_bias))) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
local GiveWay = {} | ||
|
||
function GiveWay.get_value(node) | ||
local tag = node:get_value_by_key("highway") | ||
if "give_way" == tag then | ||
local direction = node:get_value_by_key("direction") | ||
if direction then | ||
if "forward" == direction then | ||
return give_way.direction_forward | ||
end | ||
if "backward" == direction then | ||
return give_way.direction_reverse | ||
end | ||
end | ||
-- return give_way.direction_all | ||
return true | ||
end | ||
-- return give_way.none | ||
return false | ||
end | ||
|
||
return GiveWay | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
local StopSign = {} | ||
|
||
function StopSign.get_value(node) | ||
local tag = node:get_value_by_key("highway") | ||
if "stop" == tag then | ||
local direction = node:get_value_by_key("direction") | ||
if direction then | ||
if "forward" == direction then | ||
return stop_sign.direction_forward | ||
end | ||
if "backward" == direction then | ||
return stop_sign.direction_reverse | ||
end | ||
end | ||
return stop_sign.direction_all | ||
end | ||
return stop_sign.none | ||
end | ||
|
||
return StopSign | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.