Simplify pathfinding with zones #582

dabreegster · 2021-03-22T21:09:38Z

Background

Access-restricted zones (just "zones" after this) are meant to represent a few situations where "through-traffic" is restricted:

low traffic neighborhoods (in the UK)
stay healthy streets (Seattle)
private/gated neighborhoods (Broadmoor in Seattle)

They're the funny pink colored roads:

If you edit one of them and "change access restrictions", you see the explanation of how they work:

"No through-traffic" means a trip can't cut through the zone. Access only works if the trip begins or ends in that zone. This captures the real-world semantics: residents of that zone can always go in/out, people delivering or visiting can go there, but otherwise, nobody can enter.

The rules for through-traffic are further broken down by mode. For Stay Healthy Streets, for example, the point is to encourage foot/bike traffic and discourage car traffic.

Current implementation

When I implemented the pathfinding for zones last spring, for some reason I took an all-or-nothing approach. The graph that either Mr. Dijkstra searches or that feed into the contraction hierarchies omit roads inside of a zone. When map.pathfind runs, it sees if the request starts or ends in a zone. If so, it looks through all of the "borders" into/out of a zone, finds paths that fulfill the overall request, and delegates to a Dijkstra's impl to pathfind within a zone. It's complicated. There's some very hairy code to glue the pieces of the path together, especially for walking. It's not even a complete impl -- requesting to go from zone1 to a different zone2 is just unimplemented.

New implementation

This PR first rips out all of the complicated old stuff, then does something much simpler: penalizes entry into a zone at all turns crossing from outside a zone into the zone. If this penalty is high enough, then the same effect ought to be achieved -- through traffic routes around it naturally, but if a request starts or ends in the zone, then it has to incur that cost to make the graph search work at all.

Benefits

Radically simplifies the code. This is particularly useful to do to prepare for Pathfinding v2 / Exit driveways onto any lane #555, which will overhaul pathfinding completely from using lanes to using roads. Getting rid of all this special casing and path gluing makes the first step of that project much easier.
Better performance. We can just use CHs normally now; we don't need to check if a request starts/ends in a zone, separately runs Dijkstra's, and glue anything.
It handles the zone1 -> zone2 case without any particular code/effort needed.
It's a solid step towards fulfilling Traffic Diverters #488, which asks to explicitly model traffic diverters. I'm not sure how these will differ from zones, but expressing entry into the zone as a big penalty feels like it maps onto the concept of a diverter pretty well. A car can drive around a Stay Healthy St barrier, but there's some penalty discouraging it.

Validation of this change

Individual

I used the "start new trip" tool to request some individual paths in the Arboretum map. I loaded the "broadmoor access" edits and verified that car trips through Broadmoor still don't happen, but bike/foot trips do.

While doing this, I kept seeing a few cases with unexpected behavior. One example is:

Even with Broadmoor opened to bike traffic, this path doesn't make use of it. I created the new "Debug all costs" tool to figure out why. It prints the total cost of the chosen path, then floodfills from the start and tracks the cost at all roads, and shows that in tooltips. If you hover over roads near Broadmoor, you can sort of trace where the expected path becomes more expensive than the chosen one. In this case, I realized the problem is the side of the road that the destination is on. The cost of making a loop to finish at the building from the right side of the road legitimately does bump the cost up to prefer going through Lake Wash Blvd. Choosing a different building indeed makes bikes use the shortcut. This silliness will be fixed with #555 eventually.

In aggregate

Before regenerating prebaked results, I enabled the new pathfinding and looked at the sim to make sure people weren't suddenly flooding through Broadmoor. Except... oops, they were. I was confused by this, but as I checked individual people cutting through, they were all valid cases starting/ending in Broadmoor. Any guesses?

... The number of cancelled trips was the hint. Before this change, about 3400 trips fail. After, only 2600. Lots of the trips were failing before were hitting some bug with the previous zone pathfinding impl:

P4685 succeeds after the change. So that explains the increased traffic through Broadmoor.

TODO: Regenerate all maps and make sure there are no gridlock regressions. Running that now.

Sanity checking

I added validate_zones to check every single path that gets created. It makes sure a path doesn't enter a zone unless the destination is in that zone. But it has lots of false positives, because of stuff like this:

If you start in the circled roads, you have no choice but to cross through a zone to reach most of the map. This is an artifact of how the access restrictions are tagged in OSM. Technically those inner roads are also private, but they're not always mapped that way.

Ideally this'd be fixed at map import time. We could look for roads that can't reach most of the graph without crossing a zone, and fold them into that zone. But meh, not urgent.

…stricted zones. There's a much simpler implementation, it transpires. #555, #574

michaelkirk

Changes look great! It's easy to review a bunch of deletes.

And the rationale/plan laid out in your PR description all sounds good.

michaelkirk · 2021-03-22T21:59:23Z

map_model/src/objects/zone.rs

@@ -63,94 +59,6 @@ impl Zone {

        zones
    }
-
-    /// Run slower Dijkstra's within the interior of a private zone. Don't go outside the borders.


🔥 🔥 🔥

michaelkirk · 2021-03-22T22:04:05Z

map_model/src/pathfind/pathfinder.rs

        if req.constraints == PathConstraints::Pedestrian {
            if req.start.lane() == req.end.lane() {
                return Some(one_step_walking_path(&req, map));
            }
-            let steps = walking_path_to_steps(self.simple_walking_path(&req, map)?, map);
+            let nodes = match self {
+                Pathfinder::Dijkstra => dijkstra::simple_walking_path(&req, map)?,


Can you elaborate on when we expect to be using Dijkstra at this point?

Only if you import a map passing --skip_ch. None of the "official" maps in S3 use this; I just do it to debug occasionally. I could add a comment here, but the docstring on Pathfinder above sort of answers this.

... One more slight exception, pathfind_avoiding_lanes uses Dijkstra always. This gets called when congestion caps (an experimental feature) are applied through map edits and/or flags to the sim. I'll add a comment there.

gotcha, thanks!

michaelkirk · 2021-03-22T22:04:39Z

map_model/src/pathfind/pathfinder.rs

@@ -147,193 +93,4 @@ impl Pathfinder {
            Pathfinder::CH(ref mut p) => p.apply_edits(map, timer),
        }
    }
-


🔥 🔥 🔥

michaelkirk · 2021-03-22T22:06:35Z

map_model/src/pathfind/driving.rs

@@ -10,6 +10,7 @@ use abstutil::MultiMap;



minor nit: Apparently this is used by bikes too? Can we rename this module to pathfind::vehicle?

Sounds good, I'll do that in a followup.

Relatedly, I need to rename CarID to VehicleID and Bus* (ID, Stop, Route...) to PublicTransit* (buses or trains).

…rossing into the zones. #555, #574 Regenerate all maps. Gridlock-wise, Rainier and Poundbury broke, but Wallingford started working again. Acceptable cost for a change this useful; I'll work on fixing those maps later.

dabreegster · 2021-03-22T23:06:24Z

I'm uploading the 1.4 GB (sigh) of changes to S3 now. The Broadmoor proposal still has roughly the same expected effects. Rainier and Poundbury start gridlocking with this change, but Wallingford stops. I'll plunge ahead and fix those later. Very rough analysis: they start gridlocking because less trips get cancelled.

…t too, and likewise for driving_cost

Rip out all of the special code for pathfinding into/out of access-re…

2da2922

…stricted zones. There's a much simpler implementation, it transpires. #555, #574

dabreegster requested a review from michaelkirk March 22, 2021 21:12

michaelkirk approved these changes Mar 22, 2021

View reviewed changes

Rename pathfind/driving -> pathfind/vehicles to reflect bikes using i…

a04aec6

…t too, and likewise for driving_cost

dabreegster force-pushed the pathfinding_zones branch from 4a5bf7b to a04aec6 Compare March 22, 2021 23:28

dabreegster merged commit e57caca into master Mar 22, 2021

dabreegster deleted the pathfinding_zones branch March 22, 2021 23:35

dabreegster mentioned this pull request Mar 24, 2021

wasm loading maps broke #586

Closed

dabreegster mentioned this pull request Jul 14, 2021

Why does the preparation time vary so much for different maps? easbar/fast_paths#34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify pathfinding with zones #582

Simplify pathfinding with zones #582

dabreegster commented Mar 22, 2021

michaelkirk left a comment

michaelkirk Mar 22, 2021

michaelkirk Mar 22, 2021

dabreegster Mar 22, 2021

michaelkirk Mar 22, 2021

michaelkirk Mar 22, 2021

michaelkirk Mar 22, 2021

dabreegster Mar 22, 2021

dabreegster commented Mar 22, 2021

Simplify pathfinding with zones #582

Simplify pathfinding with zones #582

Conversation

dabreegster commented Mar 22, 2021

Background

Current implementation

New implementation

Benefits

Validation of this change

Individual

In aggregate

Sanity checking

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Mar 22, 2021

Choose a reason for hiding this comment

michaelkirk Mar 22, 2021

Choose a reason for hiding this comment

dabreegster Mar 22, 2021

Choose a reason for hiding this comment

michaelkirk Mar 22, 2021

Choose a reason for hiding this comment

michaelkirk Mar 22, 2021

Choose a reason for hiding this comment

michaelkirk Mar 22, 2021

Choose a reason for hiding this comment

dabreegster Mar 22, 2021

Choose a reason for hiding this comment

dabreegster commented Mar 22, 2021