Fix street routing on roundabout

[vpaturet]

Summary

As detailed in #5706, OTP fails to find a path between 2 points under certain routing network configurations.
Further analysis shows that A* cannot traverse a loop made of 2 edges:
A ------>B------>A

This is due to:

• A* refusing to backtrack on an edge that goes back to its previous state (thus preventing infinite loops?)
  In other words: if the edge e has been traversed in state n, reverse(e) cannot be traversed in state n+1.
• A technical implementation detail in the temporary edge class org.opentripplanner.street.model.edge.TemporaryPartialStreetEdge that defines the reverse of a temporary edge as the reverse of its parent.

In the corner case of the loop:
A ------>B------>A
if the trip origin is A and the destination is a temporary vertex T between B and A:
A ------>B---T--->A
The temporary edge B-T has B-A as its parent, thus reverse(B-T)=reverse(B-A)=A-B
As a result B-T cannot be traversed after A-B.

Proposed fix:
It is unclear why the reverse of a temporary edge should be the reverse of its parent.
Assuming this is only an optimization detail, this pull request removes the overloaded method TemporaryPartialStreetEdge.reverseOf() so that the default implementation applies.

Issue

Closes #5706

Unit tests

Added unit test

Documentation

No

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.78%. Comparing base (6c956b8) to head (b2ee65f).
Report is 29 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5732      +/-   ##
=============================================
+ Coverage      67.76%   67.78%   +0.01%     
- Complexity     16471    16478       +7     
=============================================
  Files           1901     1901              
  Lines          72114    72128      +14     
  Branches        7430     7429       -1     
=============================================
+ Hits           48869    48892      +23     
+ Misses         20734    20723      -11     
- Partials        2511     2513       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leonardehrenfried]

@abyrd I don't think we ever formally asked you if you are willing to review this. Are you?

[abyrd]

It's not clear why the isReverseOf logic was overridden this way in the case of TemporaryPartialStreetEdge. As discussed on the call, the reason may be lost to time, and it looks like the proposed change should work as expected.

The reasons why isReverseOf is checked are better known:

• Going back to the same vertex can never improve the state at that vertex. (All edge costs are constrained to be positive - if they weren't, this would clearly lead to infinite loops.) So if we naively traverse all edges encountered during a search, about half of the edge traversals are useless. Checking whether an edge leads back to the previously visited vertex should always be faster/cheaper than actually computing the cost of traversing it, and effectively skips the edge traversal computations for these useless traversals.
• Besides never yielding a better state, traversing the reverse edge would involve a usually-illegal U turn. In cases where U turns are feasible and beneficial, the two directions of the street are often mapped as separate ways. Indeed they'd have to be to improve the cost of reaching an intermediate vertex by making a U turn.