a-b-street/abstreet
1
1
Fork 0
mirror of https://github.com/a-b-street/abstreet.git synced 2024-11-23 17:07:12 +03:00
Code Issues Projects Releases Wiki Activity

Make the LTN tool more robust to broken blockfinding. #841

Browse Source 
Try the cheap validation first. If it breaks, fall back to the expensive
version.
This commit is contained in:
Dustin Carlino 2022-03-05 17:21:53 +00:00
parent a4153891e2
commit 99747ff34e
5 changed files with 153 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
apps/game/src/debug/blockfinder.rs
View File

@ -166,7 +166,14 @@ impl State<App> for Blockfinder {
// ID...
self.world.delete(id);
}
let results = Perimeter::merge_all(&app.primary.map, perimeters, true);
let stepwise_debug = true;
let use_expensive_blockfinding = false;
let results = Perimeter::merge_all(
&app.primary.map,
perimeters,
stepwise_debug,
use_expensive_blockfinding,
);
let debug = results.len() > 1;
for perimeter in results {
let id = self.new_id();
@ -220,10 +227,13 @@ impl State<App> for Blockfinder {
for perimeters in partitions {
// If we got more than one result back, merging partially failed. Oh
// well?
let stepwise_debug = false;
let use_expensive_blockfinding = false;
merged.extend(Perimeter::merge_all(
&app.primary.map,
perimeters,
false,
stepwise_debug,
use_expensive_blockfinding,
));
}
self.add_blocks_with_coloring(ctx, app, merged, &mut Timer::throwaway());

163
apps/ltn/src/partition.rs
View File

@ -43,6 +43,8 @@ pub struct Partitioning {
// Invariant: This is a surjection, every block belongs to exactly one neighborhood
block_to_neighborhood: BTreeMap<BlockID, NeighborhoodID>,
use_expensive_blockfinding: bool,
}
impl Partitioning {
@ -56,6 +58,8 @@ impl Partitioning {
neighborhood_id_counter: 0,
block_to_neighborhood: BTreeMap::new(),
use_expensive_blockfinding: false,
}
}
@ -64,78 +68,97 @@ impl Partitioning {
}
pub fn seed_using_heuristics(app: &App, timer: &mut Timer) -> Partitioning {
let map = &app.map;
timer.start("find single blocks");
let mut single_blocks = Vec::new();
let mut single_block_perims = Vec::new();
for mut perim in Perimeter::find_all_single_blocks(map) {
// TODO Some perimeters don't blockify after collapsing dead-ends. So do this upfront,
// and separately work on any blocks that don't show up.
// https://github.com/a-b-street/abstreet/issues/841
perim.collapse_deadends();
if let Ok(block) = perim.to_block(map) {
single_block_perims.push(block.perimeter.clone());
single_blocks.push(block);
}
}
timer.stop("find single blocks");
timer.start("partition");
let partitions = Perimeter::partition_by_predicate(single_block_perims, |r| {
// "Interior" roads of a neighborhood aren't classified as arterial
map.get_r(r).get_rank() == RoadRank::Local
});
let mut merged = Vec::new();
for perimeters in partitions {
// If we got more than one result back, merging partially failed. Oh well?
merged.extend(Perimeter::merge_all(map, perimeters, false));
}
timer.stop("partition");
timer.start_iter("blockify", merged.len());
let mut blocks = Vec::new();
for perimeter in merged {
timer.next();
match perimeter.to_block(map) {
Ok(block) => {
blocks.push(block);
}
Err(err) => {
warn!("Failed to make a block from a merged perimeter: {}", err);
// Try the easy thing first, but then give up
'METHOD: for use_expensive_blockfinding in [false, true] {
let map = &app.map;
timer.start("find single blocks");
let mut single_blocks = Vec::new();
let mut single_block_perims = Vec::new();
for mut perim in Perimeter::find_all_single_blocks(map) {
// TODO Some perimeters don't blockify after collapsing dead-ends. So do this
// upfront, and separately work on any blocks that don't show up.
// https://github.com/a-b-street/abstreet/issues/841
perim.collapse_deadends();
if let Ok(block) = perim.to_block(map) {
single_block_perims.push(block.perimeter.clone());
single_blocks.push(block);
}
}
}
timer.stop("find single blocks");
let mut neighborhoods = BTreeMap::new();
for block in blocks {
neighborhoods.insert(NeighborhoodID(neighborhoods.len()), (block, Color::RED));
}
let neighborhood_id_counter = neighborhoods.len();
let mut p = Partitioning {
map: map.get_name().clone(),
neighborhoods,
single_blocks,
timer.start("partition");
let partitions = Perimeter::partition_by_predicate(single_block_perims, |r| {
// "Interior" roads of a neighborhood aren't classified as arterial
map.get_r(r).get_rank() == RoadRank::Local
});
neighborhood_id_counter,
block_to_neighborhood: BTreeMap::new(),
};
// TODO We could probably build this up as we go
for id in p.all_block_ids() {
if let Some(neighborhood) = p.neighborhood_containing(id) {
p.block_to_neighborhood.insert(id, neighborhood);
} else {
// TODO This will break everything downstream, so bail out immediately
panic!(
"Block doesn't belong to any neighborhood?! {:?}",
p.get_block(id).perimeter
);
let mut merged = Vec::new();
for perimeters in partitions {
// If we got more than one result back, merging partially failed. Oh well?
let stepwise_debug = false;
merged.extend(Perimeter::merge_all(
map,
perimeters,
stepwise_debug,
use_expensive_blockfinding,
));
}
}
timer.stop("partition");
p.recalculate_coloring();
p
timer.start_iter("blockify", merged.len());
let mut blocks = Vec::new();
for perimeter in merged {
timer.next();
match perimeter.to_block(map) {
Ok(block) => {
blocks.push(block);
}
Err(err) => {
warn!("Failed to make a block from a merged perimeter: {}", err);
}
}
}
let mut neighborhoods = BTreeMap::new();
for block in blocks {
neighborhoods.insert(NeighborhoodID(neighborhoods.len()), (block, Color::RED));
}
let neighborhood_id_counter = neighborhoods.len();
let mut p = Partitioning {
map: map.get_name().clone(),
neighborhoods,
single_blocks,
neighborhood_id_counter,
block_to_neighborhood: BTreeMap::new(),
use_expensive_blockfinding,
};
// TODO We could probably build this up as we go
for id in p.all_block_ids() {
if let Some(neighborhood) = p.neighborhood_containing(id) {
p.block_to_neighborhood.insert(id, neighborhood);
} else {
if !use_expensive_blockfinding {
// Try the expensive check, then
error!(
"Block doesn't belong to any neighborhood? Retrying with expensive checks {:?}",
p.get_block(id).perimeter
);
continue 'METHOD;
}
// This will break everything downstream, so bail out immediately
panic!(
"Block doesn't belong to any neighborhood?! {:?}",
p.get_block(id).perimeter
);
}
}
p.recalculate_coloring();
return p;
}
unreachable!()
}
/// True if the coloring changed
@ -380,7 +403,13 @@ impl Partitioning {
perimeters.push(self.get_block(id).perimeter.clone());
}
let mut blocks = Vec::new();
for perim in Perimeter::merge_all(map, perimeters, false) {
let stepwise_debug = false;
for perim in Perimeter::merge_all(
map,
perimeters,
stepwise_debug,
self.use_expensive_blockfinding,
) {
blocks.push(perim.to_block(map)?);
}
Ok(blocks)

65
map_model/src/objects/block.rs
View File

@ -4,15 +4,11 @@ use std::fmt;
use anyhow::Result;
use serde::{Deserialize, Serialize};
use abstio::MapName;
use abstutil::wraparound_get;
use geom::{Polygon, Pt2D, Ring};
use crate::{CommonEndpoint, Direction, LaneID, Map, RoadID, RoadSideID, SideOfRoad};
// See https://github.com/a-b-street/abstreet/issues/841. Slow but correct when enabled.
const LOSSLESS_BLOCKFINDING: bool = true;
/// A block is defined by a perimeter that traces along the sides of roads. Inside the perimeter,
/// the block may contain buildings and interior roads. In the simple case, a block represents a
/// single "city block", with no interior roads. It may also cover a "neighborhood", where the
@ -136,16 +132,6 @@ impl Perimeter {
skip.insert(r.id);
}
}
// TODO This map crashes otherwise. Workaround temporarily.
if map.get_name() == &MapName::new("fr", "lyon", "center") {
for r in map.all_roads() {
if r.zorder != 0 {
skip.insert(r.id);
}
}
}
skip
}
@ -170,7 +156,13 @@ impl Perimeter {
/// TODO Due to https://github.com/a-b-street/abstreet/issues/841, it seems like rotation
/// sometimes breaks `to_block`, so for now, always revert to the original upon failure.
// TODO Would it be cleaner to return a Result here and always restore the invariant?
fn try_to_merge(&mut self, map: &Map, other: &mut Perimeter, debug_failures: bool) -> bool {
fn try_to_merge(
&mut self,
map: &Map,
other: &mut Perimeter,
debug_failures: bool,
use_expensive_blockfinding: bool,
) -> bool {
let orig_self = self.clone();
let orig_other = other.clone();
@ -275,18 +267,23 @@ impl Perimeter {
// Make sure we didn't wind up with any internal dead-ends
self.collapse_deadends();
// TODO Something in this method is buggy and produces invalid merges. Use a lightweight
// detection and bail out for now. https://github.com/a-b-street/abstreet/issues/841
if LOSSLESS_BLOCKFINDING {
if let Err(err) = self.check_continuity(map) {
debug!(
"A merged perimeter couldn't be blockified: {}. {:?}",
err, self
);
*self = orig_self;
*other = orig_other;
return false;
}
// TODO Something in this method is buggy and produces invalid merges.
// https://github.com/a-b-street/abstreet/issues/841
// First try a lightweight detection for problems. If the caller detects the net result is
// invalid, they'll override this flag and try again.
let err = if use_expensive_blockfinding {
self.clone().to_block(map).err()
} else {
self.check_continuity(map).err()
};
if let Some(err) = err {
debug!(
"A merged perimeter couldn't be blockified: {}. {:?}",
err, self
);
*self = orig_self;
*other = orig_other;
return false;
}
true
@ -306,7 +303,12 @@ impl Perimeter {
/// Try to merge all given perimeters. If successful, only one perimeter will be returned.
/// Perimeters are never "destroyed" -- if not merged, they'll appear in the results. If
/// `stepwise_debug` is true, returns after performing just one merge.
pub fn merge_all(map: &Map, mut input: Vec<Perimeter>, stepwise_debug: bool) -> Vec<Perimeter> {
pub fn merge_all(
map: &Map,
mut input: Vec<Perimeter>,
stepwise_debug: bool,
use_expensive_blockfinding: bool,
) -> Vec<Perimeter> {
// Internal dead-ends break merging, so first collapse of those. Do this before even
// looking for neighbors, since find_common_roads doesn't understand dead-ends.
for p in &mut input {
@ -324,7 +326,12 @@ impl Perimeter {
}
for other in &mut results {
if other.try_to_merge(map, &mut perimeter, stepwise_debug) {
if other.try_to_merge(
map,
&mut perimeter,
stepwise_debug,
use_expensive_blockfinding,
) {
// To debug, return after any single change
debug = stepwise_debug;
continue 'INPUT;

4
tests/goldenfiles/blockfinding.txt
View File

@ -17,4 +17,6 @@ data/system/gb/london/maps/southwark.bin
data/system/gb/manchester/maps/levenshulme.bin
1351 single blocks (1 failures to blockify), 8 partial merges, 0 failures to blockify partitions
data/system/fr/lyon/maps/center.bin
5117 single blocks (9 failures to blockify), 164 partial merges, 0 failures to blockify partitions
5220 single blocks (5 failures to blockify), 149 partial merges, 1 failures to blockify partitions
data/system/us/seattle/maps/north_seattle.bin
3376 single blocks (1 failures to blockify), 15 partial merges, 1 failures to blockify partitions

9
tests/src/main.rs
View File

@ -259,6 +259,7 @@ fn test_blockfinding() -> Result<()> {
MapName::new("gb", "london", "southwark"),
MapName::new("gb", "manchester", "levenshulme"),
MapName::new("fr", "lyon", "center"),
MapName::new("us", "seattle", "north_seattle"),
] {
let map = map_model::Map::load_synchronously(name.path(), &mut timer);
let mut single_blocks = Perimeter::find_all_single_blocks(&map);
@ -277,7 +278,10 @@ fn test_blockfinding() -> Result<()> {
let mut num_partial_merges = 0;
let mut merged = Vec::new();
for perimeters in partitions {
let newly_merged = Perimeter::merge_all(&map, perimeters, false);
let stepwise_debug = false;
let use_expensive_blockfinding = false;
let newly_merged =
Perimeter::merge_all(&map, perimeters, stepwise_debug, use_expensive_blockfinding);
if newly_merged.len() > 1 {
num_partial_merges += 1;
}
@ -287,8 +291,7 @@ fn test_blockfinding() -> Result<()> {
let mut num_merged_block_failures = 0;
for perimeter in merged {
if perimeter.to_block(&map).is_err() {
// Note this means the LTN UI will crash upfront -- every block must be in the
// partitioning.
// Note this means the LTN UI will fallback to use_expensive_blockfinding = true
num_merged_block_failures += 1;
}
}