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

Via way traffic signals #4327

Merged
merged 6 commits into from
Aug 4, 2017
Merged

Via way traffic signals #4327

merged 6 commits into from
Aug 4, 2017

Conversation

MoKob
Copy link

@MoKob MoKob commented Jul 24, 2017

Issue

Second step towards #2681.

This PR removes the restriction that we cannot compress traffic_signals or anything similar that offers a node penalty.

The basic idea behind node-penalties is that we add a turn. So abc with a traffic signal at b becomes ab bc with a turn of type NoTurn in between and a turn penalty that matches the expected delay of the traffic signal.

This way of modelling node-penalties complicates multiple features in OSRM:

  • turn lanes
  • sliproad handling
  • via way restrictions
  • turn angle computations

and possibly even more.

This PR models node-penalties as segments within the graph. abc becomes abbc with the node-penalty applied to the segment bb. This way we don't create artificial turns that influence a lot of the codebase.

Tasklist

  • add regression / cucumber cases (see docs/testing.md)
  • clean-up
    • remove debug
      -[]strip lookahead code from turn lanes / sliproads / turn-angles... -> should move to a dedicated ticket and kept out of this PR
  • review
  • adjust for comments

Requirements / Relations

Requires #4255

@MoKob MoKob force-pushed the via-way-traffic-signals branch 4 times, most recently from 5363a27 to 8f4e363 Compare July 25, 2017 14:30
@MoKob MoKob mentioned this pull request Jul 26, 2017
4 tasks
@MoKob MoKob force-pushed the via-way-traffic-signals branch 2 times, most recently from f6bc488 to d95c880 Compare July 31, 2017 14:15
@MoKob MoKob requested review from danpat and oxidase July 31, 2017 14:22
Copy link
Contributor

@oxidase oxidase left a comment

Choose a reason for hiding this comment

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

There is no way to keep consistent node weights and durations after traffic updates. Should we ignore it?

{
graph.GetEdgeData(forward_e1).weight += *node_weight_penalty;
graph.GetEdgeData(reverse_e1).weight += *node_weight_penalty;
graph.GetEdgeData(forward_e1).duration += *node_duration_penalty;
Copy link
Contributor

Choose a reason for hiding this comment

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

From here additional duration penalties will be included into speed annotations.

@@ -217,6 +240,14 @@ void GraphCompressor::Compress(const std::unordered_set<NodeID> &barrier_nodes,
graph.GetEdgeData(forward_e1).duration += forward_duration2;
graph.GetEdgeData(reverse_e1).duration += reverse_duration2;

if (node_weight_penalty && node_duration_penalty)
{
graph.GetEdgeData(forward_e1).weight += *node_weight_penalty;
Copy link
Contributor

Choose a reason for hiding this comment

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

Additional weight penalties will be lost after update of any segment of the edge-based graph node, so it is possible to get inconsistent weights values.

@oxidase oxidase force-pushed the via-way-traffic-signals branch 3 times, most recently from a84d39d to a186a06 Compare August 2, 2017 11:19
@oxidase
Copy link
Contributor

oxidase commented Aug 2, 2017

@MoKob i have added a test with traffic updates. Removal artificial traffic node works perfect with updates!

One point is a correctness of CSV files: a line x,x,y can potentially change values of an x,x artificial segment. It is possible to check after CSV parsing at

return parser(paths);
}

EDIT: Another point it is not clear should artificial segments be in unpacked_path at

unpacked_path.push_back(PathData{id_vector[segment_idx + 1],
name_index,
weight_vector[segment_idx],
0,
duration_vector[segment_idx],
0,
extractor::guidance::TurnInstruction::NO_TURN(),
{{0, INVALID_LANEID}, INVALID_LANE_DESCRIPTIONID},
travel_mode,
classes,
EMPTY_ENTRY_CLASS,
datasource_vector[segment_idx],
util::guidance::TurnBearing(0),
util::guidance::TurnBearing(0)});
or not?

@oxidase oxidase force-pushed the via-way-traffic-signals branch from a186a06 to ff292da Compare August 3, 2017 13:06
@MoKob MoKob merged commit 4757c96 into master Aug 4, 2017
@MoKob MoKob deleted the via-way-traffic-signals branch August 4, 2017 09:19
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