Add block index information to SpaceChange.

This is cheap to include and will enable `CsmTodo` to make an immediate
decision about whether to record the individual block change, and
render it as an instance, or to schedule mesh updates for the chunk and
all adjacent chunks.

CHANGELOG.md

@@ -33,6 +33,9 @@
     - All functions using `usize` to identify a coordinate axis now use `math::Axis` instead.
       `Face6::axis_number()` and `Face7::axis_number()` are now called `axis()`.
 
+    - `space::SpaceChange` now includes block indices, not just positions, in cube changes;
+      and all variants have been renamed for clarity.
+
     - In block names and universe member names, `Cow<'static, str>` and `Arc<str>` have been replaced with `arcstr::ArcStr`.
       This type allows both refcounting and static literal strings.
 

all-is-cubes-gpu/src/in_wgpu/space.rs

@@ -636,15 +636,15 @@ impl Listener<SpaceChange> for TodoListener {
             SpaceChange::EveryBlock => {
                 todo.light = None;
             }
-            SpaceChange::Lighting(p) => {
+            SpaceChange::CubeLight { cube } => {
                 // None means we're already at "update everything"
                 if let Some(set) = &mut todo.light {
-                    set.insert(p);
+                    set.insert(cube);
                 }
             }
-            SpaceChange::Block(..) => {}
-            SpaceChange::Number(..) => {}
-            SpaceChange::BlockValue(..) => {}
+            SpaceChange::CubeBlock { .. } => {}
+            SpaceChange::BlockIndex(..) => {}
+            SpaceChange::BlockEvaluation(..) => {}
         }
     }
 

all-is-cubes-mesh/src/dynamic/chunked_mesh.rs

@@ -638,20 +638,22 @@ impl<const CHUNK_SIZE: GridCoordinate> Listener<SpaceChange> for TodoListener<CH
                 todo.blocks.clear();
                 todo.chunks.clear();
             }
-            SpaceChange::Block(p) => {
-                todo.modify_block_and_adjacent(p, |chunk_todo| {
+            SpaceChange::CubeBlock {
+                cube,
+                old_block_index: _,
+                new_block_index: _,
+                ..
+            } => {
+                // TODO: use block index information to decide whether the new block
+                // should be rendered as an instanced block rather than chunk mesh
+                todo.modify_block_and_adjacent(cube, |chunk_todo| {
                     chunk_todo.recompute_mesh = true;
                 });
             }
-            SpaceChange::Lighting(_p) => {
+            SpaceChange::CubeLight { .. } => {
                 // Meshes are not affected by light
             }
-            SpaceChange::Number(index) => {
-                if !todo.all_blocks_and_chunks {
-                    todo.blocks.insert(index);
-                }
-            }
-            SpaceChange::BlockValue(index) => {
+            SpaceChange::BlockIndex(index) | SpaceChange::BlockEvaluation(index) => {
                 if !todo.all_blocks_and_chunks {
                     todo.blocks.insert(index);
                 }

all-is-cubes-mesh/src/dynamic/chunked_mesh/tests.rs

@@ -40,11 +40,11 @@ fn update_adjacent_chunk_positive() {
         (ChunkPos::new(0, 0, 0), ChunkTodo::CLEAN),
         (ChunkPos::new(1, 0, 0), ChunkTodo::CLEAN),
     ]);
-    listener.receive(SpaceChange::Block(Cube::new(
-        CHUNK_SIZE - 1,
-        CHUNK_SIZE / 2,
-        CHUNK_SIZE / 2,
-    )));
+    listener.receive(SpaceChange::CubeBlock {
+        cube: Cube::new(CHUNK_SIZE - 1, CHUNK_SIZE / 2, CHUNK_SIZE / 2),
+        old_block_index: 123,
+        new_block_index: 456,
+    });
     assert_eq!(
         read_todo_chunks(&todo),
         vec![
@@ -76,11 +76,11 @@ fn update_adjacent_chunk_negative() {
         (ChunkPos::new(0, 0, 0), ChunkTodo::CLEAN),
         (ChunkPos::new(1, 0, 0), ChunkTodo::CLEAN),
     ]);
-    listener.receive(SpaceChange::Block(Cube::new(
-        0,
-        CHUNK_SIZE / 2,
-        CHUNK_SIZE / 2,
-    )));
+    listener.receive(SpaceChange::CubeBlock {
+        cube: Cube::new(0, CHUNK_SIZE / 2, CHUNK_SIZE / 2),
+        old_block_index: 123,
+        new_block_index: 456,
+    });
     assert_eq!(
         read_todo_chunks(&todo),
         vec![
@@ -108,16 +108,24 @@ fn todo_ignores_absent_chunks() {
     let todo: Arc<Mutex<CsmTodo<CHUNK_SIZE>>> = Default::default();
     let listener = TodoListener(Arc::downgrade(&todo));
 
-    let p = Cube::from(GridPoint::new(1, 1, 1) * (CHUNK_SIZE / 2));
+    let cube = Cube::from(GridPoint::new(1, 1, 1) * (CHUNK_SIZE / 2));
     // Nothing happens...
-    listener.receive(SpaceChange::Block(p));
+    listener.receive(SpaceChange::CubeBlock {
+        cube,
+        old_block_index: 0,
+        new_block_index: 0,
+    });
     assert_eq!(read_todo_chunks(&todo), vec![]);
     // until the chunk exists in the table already.
     todo.lock()
         .unwrap()
         .chunks
         .insert(ChunkPos::new(0, 0, 0), ChunkTodo::CLEAN);
-    listener.receive(SpaceChange::Block(p));
+    listener.receive(SpaceChange::CubeBlock {
+        cube,
+        old_block_index: 0,
+        new_block_index: 0,
+    });
     assert_eq!(
         read_todo_chunks(&todo),
         vec![(

all-is-cubes/src/block.rs

@@ -488,20 +488,24 @@ impl Block {
                 if let Some(listener) = &filter.listener {
                     block_space.listen(listener.clone().filter(move |msg| {
                         match msg {
-                            SpaceChange::Block(cube)
+                            SpaceChange::CubeBlock { cube, .. }
                                 if full_resolution_bounds.contains_cube(cube) =>
                             {
                                 Some(BlockChange::new())
                             }
-                            SpaceChange::Block(_) => None,
+                            SpaceChange::CubeBlock { .. } => None,
                             SpaceChange::EveryBlock => Some(BlockChange::new()),
 
                             // TODO: It would be nice if the space gave more precise updates
                             // such that we could conclude e.g. "this is a new/removed block
                             // in an unaffected area" without needing to store any data.
-                            SpaceChange::BlockValue(_) => Some(BlockChange::new()),
-                            SpaceChange::Lighting(_) => None,
-                            SpaceChange::Number(_) => None,
+                            SpaceChange::BlockEvaluation(_) => Some(BlockChange::new()),
+
+                            // Light does not matter.
+                            SpaceChange::CubeLight { .. } => None,
+
+                            // Index changes by themselves cannot affect the result.
+                            SpaceChange::BlockIndex(_) => None,
                         }
                     }));
                 }

all-is-cubes/src/raytracer/updating.rs

@@ -191,10 +191,10 @@ impl Listener<SpaceChange> for TodoListener {
                 todo.blocks.clear();
                 todo.cubes.clear()
             }
-            SpaceChange::Lighting(p) | SpaceChange::Block(p) => {
-                todo.cubes.insert(p);
+            SpaceChange::CubeLight { cube, .. } | SpaceChange::CubeBlock { cube, .. } => {
+                todo.cubes.insert(cube);
             }
-            SpaceChange::Number(index) | SpaceChange::BlockValue(index) => {
+            SpaceChange::BlockIndex(index) | SpaceChange::BlockEvaluation(index) => {
                 todo.blocks.insert(index);
             }
         }

all-is-cubes/src/space.rs

@@ -347,7 +347,7 @@ impl Space {
 
             // Replacing one unique block with a new one.
             //
-            // This special case is worth having because it means that if a block is
+            // This special case is worth having because it means that if a unique block is
             // *modified* (read-modify-write) then the entry is preserved, and rendering
             // may be able to optimize that case.
             //
@@ -357,7 +357,12 @@ impl Space {
                 .palette
                 .try_replace_unique(old_block_index, block, &self.change_notifier)
             {
-                self.side_effects_of_set(old_block_index, position, contents_index);
+                self.side_effects_of_set(
+                    old_block_index,
+                    old_block_index,
+                    position,
+                    contents_index,
+                );
                 return Ok(true);
             }
 
@@ -373,7 +378,7 @@ impl Space {
             // Write actual space change.
             self.contents[contents_index] = new_block_index;
 
-            self.side_effects_of_set(new_block_index, position, contents_index);
+            self.side_effects_of_set(old_block_index, new_block_index, position, contents_index);
             Ok(true)
         } else {
             Err(SetCubeError::OutOfBounds {
@@ -383,20 +388,21 @@ impl Space {
         }
     }
 
-    /// Implement the consequences of changing a block.
+    /// Implement the consequences of changing what block occupies a cube.
     ///
-    /// `content_index` is redundant with `position` but saves computation.
+    /// `contents_index` is redundant with `position` but saves computation.
     #[inline]
     fn side_effects_of_set(
         &mut self,
-        block_index: BlockIndex,
-        position: Cube,
+        old_block_index: BlockIndex,
+        new_block_index: BlockIndex,
+        cube: Cube,
         contents_index: usize,
     ) {
-        let evaluated = &self.palette.entry(block_index).evaluated;
+        let evaluated = &self.palette.entry(new_block_index).evaluated;
 
         if evaluated.attributes.tick_action.is_some() {
-            self.cubes_wanting_ticks.insert(position);
+            self.cubes_wanting_ticks.insert(cube);
         }
 
         // TODO: Move this into a function in the lighting module since it is so tied to lighting
@@ -413,14 +419,14 @@ impl Space {
                 // (Note: This does not empirically have any significant effect on overall
                 // lighting performance — these trivial updates are not most of the cost.
                 // But it'll at least save a little bit of memory.)
-                self.light_update_queue.remove(position);
+                self.light_update_queue.remove(cube);
 
-                self.change_notifier.notify(SpaceChange::Lighting(position));
+                self.change_notifier.notify(SpaceChange::CubeLight { cube });
             } else {
-                self.light_needs_update(position, light::Priority::NEWLY_VISIBLE);
+                self.light_needs_update(cube, light::Priority::NEWLY_VISIBLE);
             }
             for face in Face6::ALL {
-                if let Some(neighbor) = position.checked_add(face.normal_vector()) {
+                if let Some(neighbor) = cube.checked_add(face.normal_vector()) {
                     // Perform neighbor light updates if they can be affected by us
                     if !self.get_evaluated(neighbor).opaque[face.opposite()] {
                         self.light_needs_update(neighbor, light::Priority::NEWLY_VISIBLE);
@@ -429,7 +435,11 @@ impl Space {
             }
         }
 
-        self.change_notifier.notify(SpaceChange::Block(position));
+        self.change_notifier.notify(SpaceChange::CubeBlock {
+            cube,
+            old_block_index,
+            new_block_index,
+        });
     }
 
     /// Replace blocks in `region` with a block computed by the function.
@@ -1003,23 +1013,45 @@ impl fmt::Display for SetCubeError {
         }
     }
 }
+
 /// Description of a change to a [`Space`] for use in listeners.
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 #[allow(clippy::exhaustive_enums)] // any change will probably be breaking anyway
 pub enum SpaceChange {
-    // TODO: This set of names is not very clear and self-consistent.
-    /// The block at the given location was replaced.
-    Block(Cube),
+    /// The block occupying the specified cube was replaced.
+    CubeBlock {
+        /// The cube whose contents changed.
+        cube: Cube,
+        /// The index within [`Space::block_data()`] that the space contained prior to this message.
+        old_block_index: BlockIndex,
+        /// The index within [`Space::block_data()`] that the space contains after this message.
+        ///
+        /// Note that it may be the case that `old_block_index == new_block_index`.
+        /// This does not mean that a block is replaced with itself
+        /// (that would not produce any notifications),
+        /// but rather that a block that occurred exactly once in the space was replaced with a
+        /// different block. In this situation, a [`SpaceChange::BlockIndex`] message is also sent.
+        new_block_index: BlockIndex,
+    },
+
     /// The light level value at the given location changed.
-    Lighting(Cube),
+    CubeLight {
+        /// The cube whose light level changed.
+        cube: Cube,
+    },
+
     /// The given block index number was reassigned and now refers to a different
     /// [`Block`] value.
-    Number(BlockIndex),
-    /// The definition of the block referred to by the given block index number was
-    /// changed; the result of [`Space::get_evaluated`] may differ.
-    BlockValue(BlockIndex),
-    /// Equivalent to [`SpaceChange::Block`] for every cube and [`SpaceChange::Number`]
-    /// for every index.
+    BlockIndex(BlockIndex),
+
+    /// The evaluation of the block referred to by the given block index number has
+    /// changed. The result of [`Space::get_evaluated()`] for that index may differ, but
+    /// the [`Block`] value remains equal.
+    BlockEvaluation(BlockIndex),
+
+    /// The space contents were completely overwritten in some way.
+    /// This should be understood as equivalent to [`SpaceChange::CubeBlock`] for every cube
+    /// and [`SpaceChange::BlockIndex`] for every index.
     EveryBlock,
 }
 

all-is-cubes/src/space/light/tests.rs

@@ -121,8 +121,9 @@ fn set_cube_opaque_notification() {
     let mut space = Space::empty_positive(1, 1, 1);
     let sink = Sink::new();
     space.listen(
-        sink.listener()
-            .filter(|change| matches!(change, SpaceChange::Lighting(_)).then_some(change)),
+        sink.listener().filter(|change| {
+            matches!(change, SpaceChange::CubeLight { cube: _ }).then_some(change)
+        }),
     );
     // Self-test that the initial condition is not trivially the answer we're looking for
     assert_ne!(space.get_lighting([0, 0, 0]), PackedLight::OPAQUE);
@@ -132,7 +133,9 @@ fn set_cube_opaque_notification() {
     assert_eq!(space.get_lighting([0, 0, 0]), PackedLight::OPAQUE);
     assert_eq!(
         sink.drain(),
-        vec![SpaceChange::Lighting(Cube::new(0, 0, 0))]
+        vec![SpaceChange::CubeLight {
+            cube: Cube::new(0, 0, 0)
+        }]
     );
 }
 

all-is-cubes/src/space/light/updater.rs

@@ -175,7 +175,7 @@ impl Space {
             cost += 200;
             // TODO: compute index only once
             self.lighting[self.bounds().index(cube).unwrap()] = new_light_value;
-            self.change_notifier.notify(SpaceChange::Lighting(cube));
+            self.change_notifier.notify(SpaceChange::CubeLight { cube });
 
             // If neighbors have missing (not just stale) light values, fill them in too.
             for dir in Face6::ALL {
@@ -194,8 +194,9 @@ impl Space {
                             continue;
                         }
                         self.lighting[neighbor_index] = PackedLight::guess(new_light_value.value());
-                        self.change_notifier
-                            .notify(SpaceChange::Lighting(neighbor_cube));
+                        self.change_notifier.notify(SpaceChange::CubeLight {
+                            cube: neighbor_cube,
+                        });
                         // We don't put the neighbor on the update queue because it should
                         // already be there.
                     }

all-is-cubes/src/space/palette.rs

@@ -152,7 +152,7 @@ impl Palette {
                         );
                         self.block_to_index
                             .insert(block.clone(), new_index as BlockIndex);
-                        notifier.notify(SpaceChange::Number(new_index as BlockIndex));
+                        notifier.notify(SpaceChange::BlockIndex(new_index as BlockIndex));
                         return Ok(new_index as BlockIndex);
                     }
                 }
@@ -166,7 +166,7 @@ impl Palette {
             // Grow the vector.
             self.entries.push(new_data);
             self.block_to_index.insert(block.clone(), new_index);
-            notifier.notify(SpaceChange::Number(new_index));
+            notifier.notify(SpaceChange::BlockIndex(new_index));
             Ok(new_index)
         }
     }
@@ -198,7 +198,7 @@ impl Palette {
             self.block_to_index
                 .insert(new_block.clone(), old_block_index);
 
-            notifier.notify(SpaceChange::Number(old_block_index));
+            notifier.notify(SpaceChange::BlockIndex(old_block_index));
 
             true
         } else {
@@ -247,7 +247,7 @@ impl Palette {
             let mut try_eval_again = hashbrown::HashSet::new();
             let mut todo = self.todo.lock().unwrap();
             for block_index in todo.blocks.drain() {
-                notifier.notify(SpaceChange::BlockValue(block_index));
+                notifier.notify(SpaceChange::BlockEvaluation(block_index));
                 let data: &mut SpaceBlockData = &mut self.entries[usize::from(block_index)];
 
                 // TODO: We may want to have a higher-level error handling by pausing the Space