Fix a bug caused by support OSM traffic signal directions

[GitBenjamin]

I try fix a bug caused by support OSM traffic signal directions. With mjjbell's help, I learned a lot. I hope to get @mjjbell help and support.

Issue

#6153 (comment)

• Problem 1: When crossing a directed traffic signal, the directed traffic signal sometimes does not work.
• Problem 2: There is a bug in the duration penalty of traffic signal, and the master branch also supports Fix snapping target locations to ways used in turn restrictions #6339. When weight_name = duration or weight_name = routability, a crash of the macth_api may be triggered. Because the directed traffic signal sometimes does not work in the Edge-expanded Graph, but it does in the "duplicate" edges created for the restriction graph. Finally, the sum of the forwad_weight and reverse_weight is not equal to total_weight. Last but not least, they are not loop.
  

  osrm-backend/src/engine/routing_algorithms/routing_base_ch.cpp

  Line 149 in 31e31a6

  if (weight != forward_heap.GetKey(middle) + reverse_heap.GetKey(middle))

  
  And then, it will crash in the line.
  

  osrm-backend/include/engine/routing_algorithms/routing_base.hpp

  Line 182 in 31e31a6

  BOOST_ASSERT(endpoints.source_phantom.forward_segment_id.id == source_node_id ||

  
  

  osrm-backend/include/engine/routing_algorithms/routing_base.hpp

  Line 184 in 31e31a6

  BOOST_ASSERT(endpoints.target_phantom.forward_segment_id.id == target_node_id ||

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

#6153
#6339
#6153 (comment)

[GitBenjamin]

Hi, @SiarheiFedartsou. I added CHANGELOG and removed the comment. Thanks!

[mjjbell]

The change to traffic signals needs to be made in-place, as they're subsequently used as part of the edge-expanded graph generation too. As mentioned in the other thread, we could make a compressor wrapper for traffic signals, much like we do with TurnPaths.

osrm-backend/src/extractor/turn_path_compressor.cpp

Line 36 in 31e31a6

void TurnPathCompressor::Compress(const NodeID from, const NodeID via, const NodeID to)

[GitBenjamin]

The change to traffic signals needs to be made in-place, as they're subsequently used as part of the edge-expanded graph generation too. As mentioned in the other thread, we could make a compressor wrapper for traffic signals, much like we do with TurnPaths.

osrm-backend/src/extractor/turn_path_compressor.cpp

Line 36 in 31e31a6

void TurnPathCompressor::Compress(const NodeID from, const NodeID via, const NodeID to)

Thank you very much for your guidance 😄. I will try to make a compressor wrapper for traffic signals, like you do with TurnPaths. I need some time to complete the modification, and add test cases to the Cucumber suite.

[GitBenjamin]

Hi @mjjbell, I'm sorry that i only started working on this recently. I study the TurnPathCompressor class, but i find that the traffic signal problem seems to be different.

--- Related method ---

osrm-backend/src/extractor/edge_based_graph_factory.cpp

Lines 622 to 624 in 31e31a6

	const auto &from_node =
	isTrivial ? node_along_road_entering
	: m_compressed_edge_container.GetLastEdgeSourceID(node_based_edge_from);

osrm-backend/src/extractor/edge_based_graph_factory.cpp

Line 630 in 31e31a6

const auto is_traffic_light = m_traffic_signals.HasSignal(from_node, intersection_node);

The TrafficSignals info used during the generation of the edge-expanded graph. But the HasSignal method is not affected by src/extractor/graph_compressor.cpp. As the GetLastEdgeSourceID method returns the original node_id of the segment (even if the node has been compressed), the from_node & intersection_node are the original relation in the OSM nodes. So we don't have to worry about the HasSignal losing traffic signal feature due to compression.

Do we still need to make a compressor wrapper for traffic signals?

[mjjbell]

Yes, well, this is currently working only because we're not considering the compression here.

If we start compressing the traffic signals, this becomes simpler, as we will no longer care if it's a trivial edge or not.

For example, this currently works as expected

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

    Background:
        Given the profile "car"

    Scenario: Car - Traffic signal direction turn with edge compression
        Given the node map
            """
                    d
                    |
                    2
                    |
            a-1-b - c--f
                    |
                    e

            """

        And the ways
            | nodes | highway |
            | abc   | primary |
            | ce    | primary |
            | cd    | primary |
            | ce    | primary |

        And the nodes
            | node | highway         | traffic_signals:direction | # |
            | c    | traffic_signals | forward                   | ambiguous, but happens in OSM

        When I route I should get
            | from | to | time   | weight | #                             |
            | 1    | 2  |  35.1s | 35.1   | turn with traffic light    |
            | 2    | 1  |  29.9s | 29.9   | turn with no traffic light |

But it would fail if we correctly compress the traffic signals and don't update the edge based graph factory logic.

The case that is currently not being handled properly is when there is no turn, as shown here
#6153 (comment)

[GitBenjamin]

ok, do you still need me to make a compressor wrapper for traffic signals ？😄

[mjjbell]

No, I included it in the traffic signal, as it's simple enough.

osrm-backend/include/extractor/traffic_signals.hpp

Line 23 in 8526cc7

void Compress(NodeID from, NodeID via, NodeID to)

[mjjbell]

It would be beneficial to add minimal test cases to the Cucumber suite that capture the two problems you have identified.

[GitBenjamin]

It would be beneficial to add minimal test cases to the Cucumber suite that capture the two problems you have identified.

Hi, @mjjbell. I added two test cases to the Cucumber suite, but it only capture one problems (Problem 1). Another problem may require a more complex routing network to catch.😔

[mjjbell]

@GitBenjamin I'm going to add the fix to this PR for problem 1.

It's unclear if problem 2 would also be fixed, but we can address that in a separate PR once we have identified a minimal reproduction test case.

[GitBenjamin]

@GitBenjamin I'm going to add the fix to this PR for problem 1.

It's unclear if problem 2 would also be fixed, but we can address that in a separate PR once we have identified a minimal reproduction test case.

Problem 2 no longer occurs when I call Match API again, after I modify the traffic light compressed logic. I can try to do an analysis of that part of osm data again.