Try to add missing intermediate blocks in the LTN tool automatically. [rebuild] [release] #857

The result is still quite buggy, but feels like a step forwards.

Author: dabreegster

apps/ltn/src/app.rs

@@ -124,6 +124,8 @@ pub struct Session {
     // Plan a route:
     pub main_road_penalty: f64,
     pub show_walking_cycling_routes: bool,
+    // Select boundary:
+    pub add_intermediate_blocks: bool,
 
     // Shared in all modes
     pub layers: crate::components::Layers,
@@ -229,6 +231,7 @@ impl App {
             draw_neighbourhood_style: pages::PickAreaStyle::Simple,
             main_road_penalty: 1.0,
             show_walking_cycling_routes: false,
+            add_intermediate_blocks: true,
 
             layers: crate::components::Layers::new(ctx),
             manage_proposals: false,

apps/ltn/src/logic/partition.rs

@@ -180,19 +180,19 @@ impl Partitioning {
         p
     }
 
-    // TODO Explain return value
-    pub fn transfer_block(
+    /// Add all specified blocks to new_owner. `Ok(None)` is success.  `Ok(Some(x))` is also
+    /// success, but means the old neighbourhood of SOME block in `add_all` is now gone and
+    /// replaced with something new. (This call shouldn't be used to remove multiple blocks at
+    /// once, since interpreting the result is confusing!)
+    pub fn transfer_blocks(
         &mut self,
         map: &Map,
-        add_block: BlockID,
+        add_all: Vec<BlockID>,
         new_owner: NeighbourhoodID,
     ) -> Result<Option<NeighbourhoodID>> {
-        let old_owner = self.block_to_neighbourhood(add_block);
-        assert_ne!(old_owner, new_owner);
-
         // Is the newly expanded neighbourhood a valid perimeter?
         let mut new_owner_blocks = self.neighbourhood_to_blocks(new_owner);
-        new_owner_blocks.insert(add_block);
+        new_owner_blocks.extend(add_all.clone());
         let mut new_neighbourhood_blocks = self.make_merged_blocks(map, new_owner_blocks)?;
         if new_neighbourhood_blocks.len() != 1 {
             // This happens when a hole would be created by adding this block. There are probably
@@ -201,46 +201,53 @@ impl Partitioning {
         }
         let new_neighbourhood_block = new_neighbourhood_blocks.pop().unwrap();
 
-        // Is the old neighbourhood, minus this block, still valid?
-        // TODO refactor Neighbourhood to BlockIDs?
-        let mut old_owner_blocks = self.neighbourhood_to_blocks(old_owner);
-        old_owner_blocks.remove(&add_block);
-        if old_owner_blocks.is_empty() {
-            // We're deleting the old neighbourhood!
-            self.neighbourhoods.get_mut(&new_owner).unwrap().block = new_neighbourhood_block;
-            self.neighbourhoods.remove(&old_owner).unwrap();
-            self.block_to_neighbourhood.insert(add_block, new_owner);
-            // Tell the caller to recreate this SelectBoundary state, switching to the neighbourhood
-            // we just donated to, since the old is now gone
-            return Ok(Some(new_owner));
-        }
+        let old_owners: BTreeSet<NeighbourhoodID> = add_all
+            .iter()
+            .map(|block| self.block_to_neighbourhood[block])
+            .collect();
+        // Are each of the old neighbourhoods, minus any new blocks, still valid?
+        let mut return_value = None;
+        for old_owner in old_owners {
+            let mut old_owner_blocks = self.neighbourhood_to_blocks(old_owner);
+            for x in &add_all {
+                old_owner_blocks.remove(x);
+            }
+            if old_owner_blocks.is_empty() {
+                self.neighbourhoods.remove(&old_owner).unwrap();
+                return_value = Some(new_owner);
+                continue;
+            }
 
-        let mut old_neighbourhood_blocks =
-            self.make_merged_blocks(map, old_owner_blocks.clone())?;
-        // We might be splitting the old neighbourhood into multiple pieces! Pick the largest piece
-        // as the old_owner (so the UI for trimming a neighbourhood is less jarring), and create new
-        // neighbourhoods for the others.
-        old_neighbourhood_blocks.sort_by_key(|block| block.perimeter.interior.len());
-        self.neighbourhoods.get_mut(&old_owner).unwrap().block =
-            old_neighbourhood_blocks.pop().unwrap();
-        let new_splits = !old_neighbourhood_blocks.is_empty();
-        for split_piece in old_neighbourhood_blocks {
-            let new_neighbourhood = NeighbourhoodID(self.neighbourhood_id_counter);
-            self.neighbourhood_id_counter += 1;
-            self.neighbourhoods
-                .insert(new_neighbourhood, NeighbourhoodInfo::new(split_piece));
-        }
-        if new_splits {
-            // We need to update the owner of all single blocks in these new pieces
-            for id in old_owner_blocks {
-                self.block_to_neighbourhood
-                    .insert(id, self.neighbourhood_containing(id).unwrap());
+            let mut old_neighbourhood_blocks =
+                self.make_merged_blocks(map, old_owner_blocks.clone())?;
+            // We might be splitting the old neighbourhood into multiple pieces! Pick the largest piece
+            // as the old_owner (so the UI for trimming a neighbourhood is less jarring), and create new
+            // neighbourhoods for the others.
+            old_neighbourhood_blocks.sort_by_key(|block| block.perimeter.interior.len());
+            self.neighbourhoods.get_mut(&old_owner).unwrap().block =
+                old_neighbourhood_blocks.pop().unwrap();
+            let new_splits = !old_neighbourhood_blocks.is_empty();
+            for split_piece in old_neighbourhood_blocks {
+                let new_neighbourhood = NeighbourhoodID(self.neighbourhood_id_counter);
+                self.neighbourhood_id_counter += 1;
+                self.neighbourhoods
+                    .insert(new_neighbourhood, NeighbourhoodInfo::new(split_piece));
+            }
+            if new_splits {
+                // We need to update the owner of all single blocks in these new pieces
+                for id in old_owner_blocks {
+                    self.block_to_neighbourhood
+                        .insert(id, self.neighbourhood_containing(id).unwrap());
+                }
             }
         }
 
+        // Set up the newly expanded neighbourhood
         self.neighbourhoods.get_mut(&new_owner).unwrap().block = new_neighbourhood_block;
-        self.block_to_neighbourhood.insert(add_block, new_owner);
-        Ok(None)
+        for id in add_all {
+            self.block_to_neighbourhood.insert(id, new_owner);
+        }
+        Ok(return_value)
     }
 
     /// Needs to find an existing neighbourhood to take the block, or make a new one
@@ -273,7 +280,7 @@ impl Partitioning {
                 .iter()
                 .find(|(_, info)| info.block.perimeter.roads.contains(&other_side))
             {
-                return self.transfer_block(map, id, *new_owner);
+                return self.transfer_blocks(map, vec![id], *new_owner);
             }
         }
 
@@ -285,7 +292,7 @@ impl Partitioning {
             new_owner,
             NeighbourhoodInfo::new(self.get_block(id).clone()),
         );
-        let result = self.transfer_block(map, id, new_owner);
+        let result = self.transfer_blocks(map, vec![id], new_owner);
         if result.is_err() {
             // Revert the change above!
             self.neighbourhoods.remove(&new_owner).unwrap();

apps/ltn/src/pages/select_boundary.rs

@@ -7,7 +7,7 @@ use widgetry::mapspace::{World, WorldOutcome};
 use widgetry::tools::{Lasso, PopupMsg};
 use widgetry::{
     Color, Drawable, EventCtx, GeomBatch, GfxCtx, Key, Line, Outcome, Panel, State, Text, TextExt,
-    Widget,
+    Toggle, Widget,
 };
 
 use crate::components::{legend_entry, AppwidePanel, Mode};
@@ -188,9 +188,19 @@ impl SelectBoundary {
         if self.currently_have_block(app, id) {
             mut_partitioning!(app).remove_block_from_neighbourhood(&app.per_map.map, id)
         } else {
-            // Ignore the return value if the old neighbourhood is deleted
-            mut_partitioning!(app).transfer_block(&app.per_map.map, id, self.id)?;
-            Ok(None)
+            match mut_partitioning!(app).transfer_blocks(&app.per_map.map, vec![id], self.id) {
+                // Ignore the return value if the old neighbourhood is deleted
+                Ok(_) => Ok(None),
+                Err(err) => {
+                    if app.session.add_intermediate_blocks {
+                        let mut add_all = app.partitioning().find_intermediate_blocks(self.id, id);
+                        add_all.push(id);
+                        mut_partitioning!(app).transfer_blocks(&app.per_map.map, add_all, self.id)
+                    } else {
+                        Err(err)
+                    }
+                }
+            }
         }
     }
 
@@ -224,12 +234,13 @@ impl SelectBoundary {
                 let mut changed = false;
                 let mut still_todo = Vec::new();
                 timer.start_iter("try to add blocks", add_blocks.len());
+                // TODO Sometimes it'd help to add all at once!
                 for block_id in add_blocks.drain(..) {
                     timer.next();
                     if self.frontier.contains(&block_id) {
-                        if let Ok(_) = mut_partitioning!(app).transfer_block(
+                        if let Ok(_) = mut_partitioning!(app).transfer_blocks(
                             &app.per_map.map,
-                            block_id,
+                            vec![block_id],
                             self.id,
                         ) {
                             changed = true;
@@ -285,8 +296,8 @@ impl State<App> for SelectBoundary {
         {
             return t;
         }
-        if let Outcome::Clicked(x) = self.left_panel.event(ctx) {
-            match x.as_ref() {
+        match self.left_panel.event(ctx) {
+            Outcome::Clicked(x) => match x.as_ref() {
                 "Cancel" => {
                     // TODO If we destroyed the current neighbourhood, then we cancel, we'll pop
                     // back to a different neighbourhood than we started with. And also the original
@@ -302,7 +313,13 @@ impl State<App> for SelectBoundary {
                     self.left_panel = make_panel_for_lasso(ctx, &self.appwide_panel.top_panel);
                 }
                 _ => unreachable!(),
+            },
+            Outcome::Changed(_) => {
+                app.session.add_intermediate_blocks = self
+                    .left_panel
+                    .is_checked("add intermediate blocks automatically");
             }
+            _ => {}
         }
 
         match self.world.event(ctx) {
@@ -371,6 +388,12 @@ fn make_panel(ctx: &mut EventCtx, app: &App, id: NeighbourhoodID, top_panel: &Pa
                 Line(" and paint over blocks to remove"),
             ])
             .into_widget(ctx),
+            Toggle::checkbox(
+                ctx,
+                "add intermediate blocks automatically",
+                None,
+                app.session.add_intermediate_blocks,
+            ),
             format!(
                 "Neighbourhood area: {}",
                 app.partitioning().neighbourhood_area_km2(id)