Support OSM traffic signal directions

[mjjbell]

Issue

Currently OSRM parses traffic signal nodes without consideration for the direction in which the signal applies.
This can lead to duplicated routing penalties, especially when a forward and backward signal are in close proximity on a way.

This commit adds support for directed signals to the extraction and graph creation. Signal penalties are only applied in the direction specified by the OSM tag.

We add the assignment of traffic directions to the lua scripts, maintaining backwards compatibility with the existing boolean traffic states.

As part of the changes to the internal structures used for tracking traffic signals during extraction, we stop serialising/deserialising signals to the .osrm file.

The traffic signals are only used by osrm-extract so whilst this is a data format change, it will not break any existing user processes.

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

Fixes #1317

[SiarheiFedartsou]

Nit. using Direction = uint8_t;

and why not make it enum btw?

[mjjbell]

I've just copied an existing type that interfaces with lua.
I didn't actually explore if enums work. I'll take a look.

[mjjbell]

Yep, it appears to work 👍

[SiarheiFedartsou]

Nit. I may be wrong, but it seems this is redundant(but probably I would somehow highlight that this enum is declared from outside in C++ code)

[mjjbell]

I've no idea where that's come from, will remove.

[SiarheiFedartsou]

Nit: Let’s consider extracting small function to avoid code duplication here. Also for me 10 feels as magic number here - may be extract to constant?

[SiarheiFedartsou]

Nit. Not sure we follow such code style everywhere but it seems it should be HasSignal

[SiarheiFedartsou]

LGTM 🚀 please take a look on my comments

[GitBenjamin]

Hi, @mjjbell, @SiarheiFedartsou.
I did a lot of testing and found that most traffic signal problems can be solved, but there are some problems with one type of scenario. Like this one, osm_data.

• Data

This is an intersection with a traffic signal in the center that acts in all four directions.

• Process (about traffic signal)

According to this solution, I copy the traffic signal to the middle node of the four roads and add "traffic_signals:direction" info to traffic_signal node.

• Route Test

When I get to route UnknownRoad to NewRoad, the duration is 13.5s, and when I get to route NewRoad to UnknownRoad, the duration is 33.5s, traffic signal penalty is 20s. The two schemes covered the same number of miles and traffic lights, but took different time.

So, I tweaked "traffic_signals:direction" a lot, but nothing worked out. I hope I can get your help.

[GitBenjamin]

Hi, @mjjbell, @SiarheiFedartsou. I did a lot of testing and found that most traffic signal problems can be solved, but there are some problems with one type of scenario. Like this one, osm_data.

• Data

This is an intersection with a traffic signal in the center that acts in all four directions.

• Process (about traffic signal)

According to this solution, I copy the traffic signal to the middle node of the four roads and add "traffic_signals:direction" info to traffic_signal node.

• Route Test

When I get to route UnknownRoad to NewRoad, the duration is 13.5s, and when I get to route NewRoad to UnknownRoad, the duration is 33.5s, traffic signal penalty is 20s. The two schemes covered the same number of miles and traffic lights, but took different time.

So, I tweaked "traffic_signals:direction" a lot, but nothing worked out. I hope I can get your help.

Hi, @mjjbell, @SiarheiFedartsou . I reviewed the code for the branch Support OSM traffic signal directions, i found a bug:

• firstly, the line is used to determine whether there is a traffic signal between node_u and node_v.
  

  osrm-backend/src/extractor/graph_compressor.cpp

  Line 210 in 31e31a6

  const bool has_forward_signal = traffic_signals.HasSignal(node_u, node_v);

• secondly, the line is used to update relation of nodes after the node_v is compressed.
  

  osrm-backend/src/extractor/graph_compressor.cpp

  Line 331 in 31e31a6

  graph.SetTarget(forward_e1, node_w);

• example

As shown in the picture above, when i first compress node_C, there will be no traffic signal penalty for D->A route. Because when i compress node_B, traffic_signals.HasSignal(node_A, node_B) and traffic_signals.HasSignal(node_D, node_B) are both false. Unless we compress node_B first, or graph.SetTarget(forward_e1, node_c) not executed when node_C is being compressed.

[mjjbell]

Yes, you're right. The traffic signals need a compressor wrapper to handle compression events, similar to the way turn path relations handle it.

osrm-backend/src/extractor/graph_compressor.cpp

Line 339 in 31e31a6

turn_path_compressor.Compress(node_u, node_v, node_w);

[GitBenjamin]

Yes, you're right. The traffic signals need a compressor wrapper to handle compression events, similar to the way turn path relations handle it.

osrm-backend/src/extractor/graph_compressor.cpp

Line 339 in 31e31a6

turn_path_compressor.Compress(node_u, node_v, node_w);

I try fix a bug caused by support OSM traffic signal directions. I hope to get you help and review.
#6724

[mjjbell]

@GitBenjamin using your example, I created a minimal test case that currently fails.

@routing @car @traffic_light
Feature: Car - Handle traffic lights

    Background:
        Given the profile "car"

    Scenario: Car - Traffic signal 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   | with traffic light  |
            | 2    | 1  |  33.3s | 33.3   | no traffic light    |

Let's get your PR updated so that this test passes.

[mjjbell]

@Redy1908 can you confirm what version of osrm-extract and the car.lua profile you are using?
As discussed in #6663, this can happen when you use a newer profile on an older execuatable.

There is no forward compatibility in this respect. In general, OSRM datasets are not compatible across minor releases.

[mjjbell]

I am currently using this profile with the most recent Docker image, osrm/osrm-backend which as you said seems to not have this change included. Therefore, if I wish to continue using the Docker image, I should either revert to an older profile or remove the related bits of code to this commit from the profile right?

The latest images are hosted on Github: https://github.com/Project-OSRM/osrm-backend/pkgs/container/osrm-backend
Docker Hub's last image was 5.25.0, so your case is an example of newer profile with older version.

If you are using a default profile, they come bundled with the container in /opt/

See the docker instructions for full details.