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

Fix manuever overrides finding bug #6739

Merged
merged 4 commits into from
Mar 24, 2024

Conversation

rezashokry
Copy link
Contributor

Issue

I was trying to add some maneuver override relation to the map and I noticed some of them are not working.
I tried to find out why and I figured out this is because there is a vector of maneuver overrides which is assumed to be sorted to use binary search on it in lookup but its not sorted at the binary search stage.
The vector is already sorted in extract function but after the renumber in partition function the vector becomes unsorted.

Extraction:
https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/edge_based_graph_factory.cpp#L1225

Renumbering in partitioner: (which doesn't maintain the sorted array)
https://github.com/Project-OSRM/osrm-backend/blob/master/src/partitioner/partitioner.cpp#L163C1-L163C1

Finding maneuver overrides for each node:
https://github.com/Project-OSRM/osrm-backend/blob/master/src/engine/guidance/post_processing.cpp#L582

Binary search on the vector:
https://github.com/Project-OSRM/osrm-backend/blob/master/include/engine/datafacade/contiguous_internalmem_datafacade.hpp#L600

I just simply sort the vector after the rename function before the partition function writes it down.

Tasklist

Requirements / Relations

This pull request fixes this issue.

@mjjbell
Copy link
Member

mjjbell commented Mar 17, 2024

Good spot. Looks like it happened in #4907
I'll create a test case from your original example to confirm the fix.

@mjjbell
Copy link
Member

mjjbell commented Mar 17, 2024

As this is a bug related to the multi-level partitioning, it's difficult to add a neat Cucumber test case.

However, I've verified the fix using the example PBFs from the issue details - thanks for providing them, it's very helpful.

@mjjbell
Copy link
Member

mjjbell commented Mar 17, 2024

@rezashokry can you add an entry to CHANGELOG.md ?

@mjjbell mjjbell force-pushed the fix-maneuver-overrides-bug branch from 11adea8 to e1aecf2 Compare March 24, 2024 20:30
@mjjbell mjjbell merged commit 367933f into Project-OSRM:master Mar 24, 2024
1 check passed
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.

Multi maneuove relation bug!
2 participants