Skip to content

Fix multimodal ferry reclassification#5139

Merged
kevinkreiser merged 11 commits intovalhalla:masterfrom
kinkard:fix-multimodal-ferry-reclassification
Mar 25, 2025
Merged

Fix multimodal ferry reclassification#5139
kevinkreiser merged 11 commits intovalhalla:masterfrom
kinkard:fix-multimodal-ferry-reclassification

Conversation

@kinkard
Copy link
Member

@kinkard kinkard commented Mar 5, 2025

Issue

Valhalla fails to take LeShuttle (tunnel between UK and France) for London -> Paris route - https://www.openstreetmap.org/directions?engine=fossgis_valhalla_car&route=51.50%2C-0.15%3B48.84%2C2.35#map=8/50.126/1.890
image

with a quite interesting expansion graph around the tunnel entrance

image

The reason for such behaviour is that ShortestPath() ferry connections reclassification algorithm might find a route, that is impossible to drive (e.g., an hgv-only edge, followed by non-hgv edge) due to exclusive access restrictions.

Solution

This PR adds NodeLabel::access property to track overall path access, accumulating it for both edges and nodes, fixing the "catch as match accesses as possible" iterations.

How London -> Paris route around LeShuttle looks like with this PR
image

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

This PR generalises the fix made in #4854

Comment on lines +645 to +660
const std::string ascii_map = R"(
A=====X
|
|
B I---M==N
/| |\
/ | | \
E | | K
| | | |
F | | J
\ | | /
\| |/
C H
| |
D---G
)";
Copy link
Member Author

@kinkard kinkard Mar 5, 2025

Choose a reason for hiding this comment

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

Before this PR, in such cases, reclassification could find the shortest path for ABCDGHI with path_access == 0 (as BC is for hgv only and HI is for cars) and would stop there, unable to identify a better path.

@@ -70,7 +70,7 @@ std::pair<uint32_t, bool> ShortestPath(const uint32_t start_node_idx,
overall_access_before = overall_access_after;
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously only the path with overall_access_after == 0 has been found for LeShuttle entrance, preventing this algorithm from further iterations

@nilsnolde
Copy link
Member

Yep, thanks, that makes sense to me! And makes the overall logic a bit saner as well:)

FWIW, @kevinkreiser had a wild idea a while back, where we'd ditch the entire reclassification (also for links) and simply move the roads in the hierarchies (without updating the classification). After all, that's all we want from it and it'd prevent problems further down, e.g. for openlr matching etc. I did that work in a private fork a while ago and it seemed to work. However, it was never merged bcs of all the potential (cross-team) problems for guidance, their openlr matching algo, visual tiles etc, which would have to be tested carefully. Good news is that the public project wouldn't be affected by that very much. Though RAD testing could also here come up with some diffs, it's possible our guidance code shows some adverse effects, maybe it's been working around the classification issues.. Also, I think in that case we wouldn't have to do this before building the graph, we could probably move it into the enhance stage and maybe consolidate a few of the routing algos there (not_thru & density also run one full planet expansion each).

@nilsnolde nilsnolde self-requested a review March 20, 2025 20:39
nilsnolde
nilsnolde previously approved these changes Mar 20, 2025
@kevinkreiser kevinkreiser merged commit 603984f into valhalla:master Mar 25, 2025
7 checks passed
@kinkard kinkard deleted the fix-multimodal-ferry-reclassification branch March 25, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants