Prevent many cases of blocks with overlapping geometry or that extend

too far, by not trying to trace near railways or cycle-only
bridges/tunnels. This is an imperfect heuristic, but it makes
significant progress in most maps.

game/src/debug/mod.rs

@@ -788,9 +788,14 @@ impl ContextualActions for Actions {
             }
             (ID::Lane(l), "trace this block") => {
                 app.primary.current_selection = None;
+                let map = &app.primary.map;
                 return Transition::Push(
-                    match Perimeter::single_block(&app.primary.map, l)
-                        .and_then(|perim| perim.to_block(&app.primary.map))
+                    match Perimeter::single_block(
+                        map,
+                        l,
+                        &Perimeter::find_roads_to_skip_tracing(map),
+                    )
+                    .and_then(|perim| perim.to_block(map))
                     {
                         Ok(block) => blockfinder::OneBlock::new_state(ctx, app, block),
                         Err(err) => {

map_model/src/objects/block.rs

@@ -35,10 +35,16 @@ pub struct Perimeter {
 impl Perimeter {
     /// Starting at any lane, snap to the nearest side of that road, then begin tracing a single
     /// block, with no interior roads. This will fail if a map boundary is reached. The results are
-    /// unusual when crossing the entrance to a tunnel or bridge.
-    pub fn single_block(map: &Map, start: LaneID) -> Result<Perimeter> {
+    /// unusual when crossing the entrance to a tunnel or bridge, and so `skip` is used to avoid
+    /// tracing there.
+    pub fn single_block(map: &Map, start: LaneID, skip: &HashSet<RoadID>) -> Result<Perimeter> {
         let mut roads = Vec::new();
         let start_road_side = map.get_l(start).get_nearest_side_of_road(map);
+
+        if skip.contains(&start_road_side.road) {
+            bail!("Started on a road we shouldn't trace");
+        }
+
         // We need to track which side of the road we're at, but also which direction we're facing
         let mut current_road_side = start_road_side;
         let mut current_intersection = map.get_l(start).dst_i;
@@ -47,7 +53,8 @@ impl Perimeter {
             if i.is_border() {
                 bail!("hit the map boundary");
             }
-            let sorted_roads = i.get_road_sides_sorted_by_incoming_angle(map);
+            let mut sorted_roads = i.get_road_sides_sorted_by_incoming_angle(map);
+            sorted_roads.retain(|id| !skip.contains(&id.road));
             let idx = sorted_roads
                 .iter()
                 .position(|x| *x == current_road_side)
@@ -85,6 +92,8 @@ impl Perimeter {
     /// This calculates all single block perimeters for the entire map. The resulting list does not
     /// cover roads near the map boundary.
     pub fn find_all_single_blocks(map: &Map) -> Vec<Perimeter> {
+        let skip = Perimeter::find_roads_to_skip_tracing(map);
+
         let mut seen = HashSet::new();
         let mut perimeters = Vec::new();
         for lane in map.all_lanes() {
@@ -92,7 +101,7 @@ impl Perimeter {
             if seen.contains(&side) {
                 continue;
             }
-            match Perimeter::single_block(map, lane.id) {
+            match Perimeter::single_block(map, lane.id, &skip) {
                 Ok(perimeter) => {
                     seen.extend(perimeter.roads.clone());
                     perimeters.push(perimeter);
@@ -111,6 +120,21 @@ impl Perimeter {
         perimeters
     }
 
+    /// Trying to form blocks near railways or cycleways that involve bridges/tunnels often causes
+    /// overlapping geometry or blocks that're way too large. These are extremely imperfect
+    /// heuristics to avoid the worst problems.
+    pub fn find_roads_to_skip_tracing(map: &Map) -> HashSet<RoadID> {
+        let mut skip = HashSet::new();
+        for r in map.all_roads() {
+            if r.is_light_rail() {
+                skip.insert(r.id);
+            } else if r.is_cycleway() && r.zorder != 0 {
+                skip.insert(r.id);
+            }
+        }
+        skip
+    }
+
     /// A perimeter has the first and last road matching up, but that's confusing to
     /// work with. Temporarily undo that.
     fn undo_invariant(&mut self) {
@@ -218,6 +242,16 @@ impl Perimeter {
         // This order assumes everything is clockwise to start with.
         self.roads.append(&mut other.roads);
 
+        // TODO This case was introduced with find_roads_to_skip_tracing. Not sure why.
+        if self.roads.is_empty() {
+            if debug_failures {
+                warn!("Two perimeters had every road in common: {:?}", common);
+            }
+            *self = orig_self;
+            *other = orig_other;
+            return false;
+        }
+
         self.interior.extend(common);
         self.interior.append(&mut other.interior);
 
@@ -282,6 +316,7 @@ impl Perimeter {
     /// If the perimeter follows any dead-end roads, "collapse" them and instead make the perimeter
     /// contain the dead-end.
     pub fn collapse_deadends(&mut self) {
+        let orig = self.clone();
         self.undo_invariant();
 
         // TODO Workaround https://github.com/a-b-street/abstreet/issues/834. If this is a loop
@@ -308,6 +343,11 @@ impl Perimeter {
         }
 
         self.roads = roads;
+        if self.roads.is_empty() {
+            // TODO This case was introduced with find_roads_to_skip_tracing. Not sure why.
+            *self = orig;
+            return;
+        }
         self.restore_invariant();
     }
 

tests/goldenfiles/blockfinding.txt

@@ -1,16 +1,18 @@
 data/system/us/seattle/maps/montlake.bin
-    157 single blocks (0 failures to blockify), 1 partial merges, 0 failures to blockify partitions
+    158 single blocks (0 failures to blockify), 1 partial merges, 0 failures to blockify partitions
 data/system/us/seattle/maps/downtown.bin
-    1443 single blocks (0 failures to blockify), 13 partial merges, 0 failures to blockify partitions
+    1433 single blocks (0 failures to blockify), 12 partial merges, 0 failures to blockify partitions
 data/system/us/seattle/maps/lakeslice.bin
-    1033 single blocks (1 failures to blockify), 4 partial merges, 0 failures to blockify partitions
+    1035 single blocks (1 failures to blockify), 4 partial merges, 0 failures to blockify partitions
 data/system/us/phoenix/maps/tempe.bin
-    406 single blocks (3 failures to blockify), 4 partial merges, 0 failures to blockify partitions
-data/system/gb/leeds/maps/north.bin
-    2582 single blocks (4 failures to blockify), 20 partial merges, 0 failures to blockify partitions
+    382 single blocks (1 failures to blockify), 1 partial merges, 0 failures to blockify partitions
 data/system/gb/bristol/maps/east.bin
-    1061 single blocks (4 failures to blockify), 6 partial merges, 0 failures to blockify partitions
+    1031 single blocks (4 failures to blockify), 7 partial merges, 0 failures to blockify partitions
+data/system/gb/leeds/maps/north.bin
+    2548 single blocks (4 failures to blockify), 21 partial merges, 0 failures to blockify partitions
 data/system/gb/london/maps/camden.bin
-    3519 single blocks (4 failures to blockify), 34 partial merges, 0 failures to blockify partitions
+    3445 single blocks (3 failures to blockify), 37 partial merges, 0 failures to blockify partitions
 data/system/gb/london/maps/southwark.bin
-    3477 single blocks (5 failures to blockify), 46 partial merges, 0 failures to blockify partitions
+    3286 single blocks (5 failures to blockify), 42 partial merges, 0 failures to blockify partitions
+data/system/gb/manchester/maps/levenshulme.bin
+    1337 single blocks (1 failures to blockify), 2 partial merges, 0 failures to blockify partitions

tests/src/main.rs

@@ -253,10 +253,11 @@ fn test_blockfinding() -> Result<()> {
         MapName::seattle("downtown"),
         MapName::seattle("lakeslice"),
         MapName::new("us", "phoenix", "tempe"),
-        MapName::new("gb", "leeds", "north"),
         MapName::new("gb", "bristol", "east"),
+        MapName::new("gb", "leeds", "north"),
         MapName::new("gb", "london", "camden"),
         MapName::new("gb", "london", "southwark"),
+        MapName::new("gb", "manchester", "levenshulme"),
     ] {
         let map = map_model::Map::load_synchronously(name.path(), &mut timer);
         let mut single_blocks = Perimeter::find_all_single_blocks(&map);