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 routes incorrectly ending on a road with a modal filter #189

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

dabreegster
Copy link
Collaborator

Cici spotted quite a notable bug:
image
An interior road right on the edge with a filter on it should not have any shortcuts! View shortcut mode reveals the problem is routes ending there:
image

Previously, I think we got away with this because of

if router_input.has_modal_filter(road.id) {
. Roads with filters weren't part of the CH graph at all. But the change to (road, direction) pairs and handling all roads connected to the start/end intersection somehow let this slip in.

@@ -158,6 +158,9 @@ impl Intersection {
if to_road.id == from_road.id {
return None;
}
if router_input.has_modal_filter(to_road.id) {
return None;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we will relax this later to solve part of #181. When a route starts or ends on a filtered road, we can travel only one direction along it, depending on the start position.

@dabreegster dabreegster requested a review from michaelkirk March 6, 2025 13:52
Copy link
Contributor

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

LGTM!

Yeah, this was just a bad bug in my implementation.

let Route { steps } = map.router_before.route_from_roads(r(3), r(2)).unwrap();
assert_eq!(
steps,
vec![(r(3), Direction::Backwards), (r(2), Direction::Backwards)]
);

// Filter r2
map.add_modal_filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding a test! I think routing could be a bit of a whack-a-mole problem, so it's good to make sure we're not regressing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Easy to add tests once there's at least one :) Let the virtuous cycle continue!

@dabreegster dabreegster merged commit c675488 into main Mar 6, 2025
1 check passed
@dabreegster dabreegster deleted the shortcuts_ending_on_filter branch March 6, 2025 14:18
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.

2 participants