diff --git a/all-is-cubes/src/space.rs b/all-is-cubes/src/space.rs index 9881b254c..b7e9ba58c 100644 --- a/all-is-cubes/src/space.rs +++ b/all-is-cubes/src/space.rs @@ -318,13 +318,30 @@ impl Space { ) -> Result { // Delegate to a monomorphic function. // This may reduce compile time and code size. - self.set_impl(position.into(), &block.into()) + Self::set_impl( + &mut MutationCtx { + palette: &mut self.palette, + contents: self.contents.as_mut(), + light: &mut self.light, + change_buffer: &mut self.change_notifier.buffer(), + cubes_wanting_ticks: &mut self.cubes_wanting_ticks, + }, + position.into(), + &block.into(), + ) } - fn set_impl(&mut self, position: Cube, block: &Block) -> Result { - if let Some(contents_index) = self.contents.index(position) { - let old_block_index = self.contents.as_linear()[contents_index]; - let old_block = self.palette.entry(old_block_index).block(); + /// Implementation of replacing the block in a single cube, as in [`Self::set()`]. + /// Monomorphic to keep codegen costs low. + /// Takes individual borrowed fields to enable use of `ChangeBuffer`. + fn set_impl( + ctx: &mut MutationCtx<'_, '_>, + position: Cube, + block: &Block, + ) -> Result { + if let Some(contents_index) = ctx.contents.index(position) { + let old_block_index = ctx.contents.as_linear()[contents_index]; + let old_block = ctx.palette.entry(old_block_index).block(); if *old_block == *block { // No change. return Ok(false); @@ -338,11 +355,12 @@ impl Space { // // It also means that the externally observable block index behavior is easier // to characterize and won't create unnecessary holes. - if self + if ctx .palette - .try_replace_unique(old_block_index, block, &self.change_notifier) + .try_replace_unique(old_block_index, block, ctx.change_buffer) { - self.side_effects_of_set( + Self::side_effects_of_set( + ctx, old_block_index, old_block_index, position, @@ -352,23 +370,27 @@ impl Space { } // Find or allocate index for new block. This must be done before other mutations since it can fail. - let new_block_index = self - .palette - .ensure_index(block, &self.change_notifier, true)?; + let new_block_index = ctx.palette.ensure_index(block, ctx.change_buffer, true)?; // Update counts - self.palette.decrement_maybe_free(old_block_index); - self.palette.increment(new_block_index); + ctx.palette.decrement_maybe_free(old_block_index); + ctx.palette.increment(new_block_index); // Write actual space change. - self.contents.as_linear_mut()[contents_index] = new_block_index; - - self.side_effects_of_set(old_block_index, new_block_index, position, contents_index); + ctx.contents.as_linear_mut()[contents_index] = new_block_index; + + Self::side_effects_of_set( + ctx, + old_block_index, + new_block_index, + position, + contents_index, + ); Ok(true) } else { Err(SetCubeError::OutOfBounds { modification: GridAab::single_cube(position), - space_bounds: self.bounds(), + space_bounds: ctx.contents.bounds(), }) } } @@ -378,33 +400,30 @@ impl Space { /// `contents_index` is redundant with `position` but saves computation. #[inline] fn side_effects_of_set( - &mut self, + ctx: &mut MutationCtx<'_, '_>, old_block_index: BlockIndex, new_block_index: BlockIndex, cube: Cube, contents_index: usize, ) { - let evaluated = &self.palette.entry(new_block_index).evaluated; + let evaluated = &ctx.palette.entry(new_block_index).evaluated; if evaluated.attributes.tick_action.is_some() { - self.cubes_wanting_ticks.insert(cube); + ctx.cubes_wanting_ticks.insert(cube); } - { - // inlined borrow_light_update_context() to not conflict with `evaluated` borrow - let (light, uc) = ( - &mut self.light, - light::UpdateCtx { - contents: self.contents.as_ref(), - palette: &self.palette, - change_notifier: &self.change_notifier, - }, - ); - - light.modified_cube_needs_update(uc, cube, evaluated, contents_index); - } + ctx.light.modified_cube_needs_update( + light::UpdateCtx { + contents: ctx.contents.as_ref(), + palette: ctx.palette, + }, + ctx.change_buffer, + cube, + evaluated, + contents_index, + ); - self.change_notifier.notify(SpaceChange::CubeBlock { + ctx.change_buffer.push(SpaceChange::CubeBlock { cube, old_block_index, new_block_index, @@ -450,11 +469,20 @@ impl Space { space_bounds: self.bounds(), }); } + + let mutation_ctx = &mut MutationCtx { + palette: &mut self.palette, + contents: self.contents.as_mut(), + light: &mut self.light, + change_buffer: &mut self.change_notifier.buffer(), + cubes_wanting_ticks: &mut self.cubes_wanting_ticks, + }; + for cube in region.interior_iter() { if let Some(block) = function(cube) { // TODO: Optimize side effect processing by batching lighting updates for // when we know what's now opaque or not. - self.set(cube, block.borrow())?; + Self::set_impl(mutation_ctx, cube, block.borrow())?; } } Ok(()) @@ -550,7 +578,7 @@ impl Space { deadline: time::Deadline, ) -> (SpaceStepInfo, UniverseTransaction) { // Process changed block definitions. - let evaluations = self.palette.step::(&self.change_notifier); + let evaluations = self.palette.step::(&mut self.change_notifier.buffer()); // Process cubes_wanting_ticks. let start_cube_ticks = I::now(); @@ -572,9 +600,10 @@ impl Space { let space_behaviors_to_lighting = I::now(); let light = { - let (light_storage, uc) = self.borrow_light_update_context(); + let (light_storage, uc, mut change_buffer) = self.borrow_light_update_context(); light_storage.update_lighting_from_queue::( uc, + &mut change_buffer, deadline.remaining_since(space_behaviors_to_lighting), ) }; @@ -764,13 +793,16 @@ impl Space { epsilon: u8, mut progress_callback: impl FnMut(LightUpdatesInfo), ) -> usize { - let (light, uc) = self.borrow_light_update_context(); + let (light, uc, mut change_buffer) = self.borrow_light_update_context(); let epsilon = light::Priority::from_difference(epsilon); let mut total = 0; loop { - let info = - light.update_lighting_from_queue::(uc, Some(Duration::from_secs_f32(0.25))); + let info = light.update_lighting_from_queue::( + uc, + &mut change_buffer, + Some(Duration::from_secs_f32(0.25)), + ); progress_callback(info); @@ -805,14 +837,16 @@ impl Space { self.physics = physics; let physics = self.physics.clone(); // TODO: put physics in UpdateCtx? - let (light, uc) = self.borrow_light_update_context(); + let (light, uc, mut change_buffer) = self.borrow_light_update_context(); light.maybe_reinitialize_for_physics_change( uc, &physics, uc.palette.all_block_opacities_as_category(), ); - self.change_notifier.notify(SpaceChange::Physics); + // TODO: We should notify specifically whether the light changed, + // but there isn't a message for that. + change_buffer.push(SpaceChange::Physics); } /// Returns the current default [`Spawn`], which determines where new [`Character`]s @@ -837,8 +871,10 @@ impl Space { /// /// TODO: Revisit whether this is a good public API. pub fn fast_evaluate_light(&mut self) { - let (light, uc) = self.borrow_light_update_context(); + let (light, uc, _change_buffer) = self.borrow_light_update_context(); light.fast_evaluate_light(uc); + + // TODO: change_buffer.push(SpaceChange::EveryBlock), or something } #[doc(hidden)] // kludge used by session for tool usage @@ -846,8 +882,8 @@ impl Space { &mut self, budget: Duration, ) -> LightUpdatesInfo { - let (light, uc) = self.borrow_light_update_context(); - light.update_lighting_from_queue::(uc, Some(budget)) + let (light, uc, mut change_buffer) = self.borrow_light_update_context(); + light.update_lighting_from_queue::(uc, &mut change_buffer, Some(budget)) } /// Compute the new lighting value for a cube. @@ -859,13 +895,13 @@ impl Space { where D: light::LightComputeOutput, { + // Unlike borrow_light_update_context(), this returns only &s let (light, uc) = { ( &self.light, light::UpdateCtx { contents: self.contents.as_ref(), palette: &self.palette, - change_notifier: &self.change_notifier, }, ) }; @@ -878,14 +914,16 @@ impl Space { } /// Produce split borrows of `self` to run light updating functions. - fn borrow_light_update_context(&mut self) -> (&mut LightStorage, light::UpdateCtx<'_>) { + fn borrow_light_update_context( + &mut self, + ) -> (&mut LightStorage, light::UpdateCtx<'_>, ChangeBuffer<'_>) { ( &mut self.light, light::UpdateCtx { contents: self.contents.as_ref(), palette: &self.palette, - change_notifier: &self.change_notifier, }, + self.change_notifier.buffer(), ) } @@ -1355,3 +1393,15 @@ impl<'s> Extract<'s> { } } } + +// TODO: Tune this buffer size parameter, and validate it isn't overly large on the stack. +type ChangeBuffer<'notifier> = crate::listen::Buffer<'notifier, SpaceChange, 16>; + +/// Argument passed to [`Space`] mutation methods that are used in bulk mutations. +struct MutationCtx<'a, 'n> { + contents: Vol<&'a mut [BlockIndex]>, + light: &'a mut LightStorage, + palette: &'a mut Palette, + cubes_wanting_ticks: &'a mut HbHashSet, + change_buffer: &'a mut ChangeBuffer<'n>, +} diff --git a/all-is-cubes/src/space/light/updater.rs b/all-is-cubes/src/space/light/updater.rs index e0c3e27c6..ecacd9370 100644 --- a/all-is-cubes/src/space/light/updater.rs +++ b/all-is-cubes/src/space/light/updater.rs @@ -14,7 +14,6 @@ use rayon::iter::{IntoParallelIterator as _, ParallelIterator as _}; use super::debug::LightComputeOutput; use crate::block::{self, EvaluatedBlock}; -use crate::listen; use crate::math::{ Cube, Face6, FaceMap, FreeCoordinate, Geometry, NotNan, OpacityCategory, Rgb, VectorOps, Vol, }; @@ -22,8 +21,8 @@ use crate::raycast::{Ray, RaycastStep}; use crate::space::light::{LightUpdateQueue, LightUpdateRayInfo, LightUpdateRequest, Priority}; use crate::space::palette::Palette; use crate::space::{ - BlockIndex, BlockSky, GridAab, LightPhysics, LightStatus, PackedLight, PackedLightScalar, Sky, - SpaceChange, SpacePhysics, + BlockIndex, BlockSky, ChangeBuffer, GridAab, LightPhysics, LightStatus, PackedLight, + PackedLightScalar, Sky, SpaceChange, SpacePhysics, }; use crate::time::{Duration, Instant}; use crate::util::StatusText; @@ -147,6 +146,7 @@ impl LightStorage { pub(in crate::space) fn modified_cube_needs_update( &mut self, uc: UpdateCtx<'_>, + change_buffer: &mut ChangeBuffer<'_>, cube: Cube, evaluated: &EvaluatedBlock, contents_index: usize, @@ -169,7 +169,7 @@ impl LightStorage { // But it'll at least save a little bit of memory.) self.light_update_queue.remove(cube); - uc.change_notifier.notify(SpaceChange::CubeLight { cube }); + change_buffer.push(SpaceChange::CubeLight { cube }); } else { self.light_needs_update(cube, Priority::NEWLY_VISIBLE); } @@ -193,6 +193,7 @@ impl LightStorage { pub(in crate::space) fn update_lighting_from_queue( &mut self, uc: UpdateCtx<'_>, + change_buffer: &mut ChangeBuffer<'_>, budget: Option, ) -> LightUpdatesInfo { let mut light_update_count: usize = 0; @@ -237,7 +238,8 @@ impl LightStorage { self.last_light_updates.push(output.cube); } light_update_count += 1; - let (difference, cube_cost) = self.apply_lighting_update(uc, output); + let (difference, cube_cost) = + self.apply_lighting_update(uc, change_buffer, output); max_difference = max_difference.max(difference); cost += cube_cost; } @@ -257,7 +259,8 @@ impl LightStorage { let computation = self.compute_lighting(uc, cube); - let (difference, cube_cost) = self.apply_lighting_update(uc, computation); + let (difference, cube_cost) = + self.apply_lighting_update(uc, change_buffer, computation); max_difference = max_difference.max(difference); cost += cube_cost; if cost >= max_cost { @@ -287,6 +290,7 @@ impl LightStorage { fn apply_lighting_update( &mut self, uc: UpdateCtx<'_>, + change_buffer: &mut ChangeBuffer<'_>, computation: ComputedLight<()>, ) -> (PackedLightScalar, usize) { let ComputedLight { @@ -305,7 +309,7 @@ impl LightStorage { cost += 200; // TODO: compute volume index of the cube only once self.contents[cube] = new_light_value; - uc.change_notifier.notify(SpaceChange::CubeLight { cube }); + change_buffer.push(SpaceChange::CubeLight { cube }); // If neighbors have missing (not just stale) light values, fill them in too. for dir in Face6::ALL { @@ -324,7 +328,7 @@ impl LightStorage { continue; } *neighbor_light = PackedLight::guess(new_light_value.value()); - uc.change_notifier.notify(SpaceChange::CubeLight { + change_buffer.push(SpaceChange::CubeLight { cube: neighbor_cube, }); // We don't put the neighbor on the update queue because it should @@ -353,10 +357,7 @@ impl LightStorage { (difference_priority, cost) } - /// Compute the new lighting value for a cube. - /// - /// The returned vector of points lists those cubes which the computed value depends on - /// (imprecisely; empty cubes passed through are not listed). + /// Compute the new lighting value for a cube, returning it rather than storing it. #[inline] #[doc(hidden)] // pub to be used by all-is-cubes-gpu for debugging pub(in crate::space) fn compute_lighting( @@ -438,6 +439,8 @@ impl LightStorage { /// Clear and recompute light data and update queue, in a way which gets fast approximate /// results suitable for flat landscapes mostly lit from above (the +Y axis). /// + /// Does not send any change notifications. + /// /// TODO: Revisit whether this is a good public API. pub(in crate::space) fn fast_evaluate_light(&mut self, uc: UpdateCtx<'_>) { self.light_update_queue.clear(); // Going to refill it @@ -512,12 +515,12 @@ impl LightStorage { } } -/// Argument passed to [`LightStorage`] methods to provide access to the rest of the space. -#[derive(Copy, Clone, Debug)] +/// Argument passed to [`LightStorage`] methods to provide immutable and shareable access to the +/// rest of the space. (Don't try to add any `&mut` references to this!) +#[derive(Clone, Copy, Debug)] pub(in crate::space) struct UpdateCtx<'a> { pub(in crate::space) contents: Vol<&'a [BlockIndex]>, pub(in crate::space) palette: &'a Palette, - pub(in crate::space) change_notifier: &'a listen::Notifier, } impl<'a> UpdateCtx<'a> { diff --git a/all-is-cubes/src/space/palette.rs b/all-is-cubes/src/space/palette.rs index cf7db709a..99663d2bd 100644 --- a/all-is-cubes/src/space/palette.rs +++ b/all-is-cubes/src/space/palette.rs @@ -8,7 +8,7 @@ use itertools::Itertools as _; use crate::block::{self, Block, BlockChange, EvaluatedBlock, AIR, AIR_EVALUATED}; use crate::listen::{self, Listener as _}; use crate::math::{self, OpacityCategory}; -use crate::space::{BlockIndex, SetCubeError, SpaceChange}; +use crate::space::{BlockIndex, ChangeBuffer, SetCubeError, SpaceChange}; use crate::time::Instant; use crate::util::maybe_sync::Mutex; use crate::util::TimeStats; @@ -78,6 +78,7 @@ impl Palette { blocks: &mut dyn ExactSizeIterator, ) -> Result<(Self, hashbrown::HashMap), PaletteError> { let dummy_notifier = listen::Notifier::new(); + let dummy_buffer = &mut dummy_notifier.buffer(); let len = blocks.len(); if len.saturating_sub(1) > (BlockIndex::MAX as usize) { @@ -93,7 +94,7 @@ impl Palette { let mut remapping = hashbrown::HashMap::new(); for (original_index, block) in (0..).zip(blocks) { let new_index = new_self - .ensure_index(&block, &dummy_notifier, false) + .ensure_index(&block, dummy_buffer, false) .expect("palette iterator lied about its length"); if new_index != original_index { remapping.insert(original_index, new_index); @@ -135,7 +136,7 @@ impl Palette { pub(super) fn ensure_index( &mut self, block: &Block, - notifier: &listen::Notifier, + change_buffer: &mut ChangeBuffer<'_>, use_zeroed_entries: bool, ) -> Result { if let Some(&old_index) = self.block_to_index.get(block) { @@ -153,7 +154,7 @@ impl Palette { ); self.block_to_index .insert(block.clone(), new_index as BlockIndex); - notifier.notify(SpaceChange::BlockIndex(new_index as BlockIndex)); + change_buffer.push(SpaceChange::BlockIndex(new_index as BlockIndex)); return Ok(new_index as BlockIndex); } } @@ -167,7 +168,7 @@ impl Palette { // Grow the vector. self.entries.push(new_data); self.block_to_index.insert(block.clone(), new_index); - notifier.notify(SpaceChange::BlockIndex(new_index)); + change_buffer.push(SpaceChange::BlockIndex(new_index)); Ok(new_index) } } @@ -178,7 +179,7 @@ impl Palette { &mut self, old_block_index: BlockIndex, new_block: &Block, - notifier: &listen::Notifier, + change_buffer: &mut ChangeBuffer<'_>, ) -> bool { if self.entries[old_block_index as usize].count == 1 && !self.block_to_index.contains_key(new_block) @@ -199,7 +200,7 @@ impl Palette { self.block_to_index .insert(new_block.clone(), old_block_index); - notifier.notify(SpaceChange::BlockIndex(old_block_index)); + change_buffer.push(SpaceChange::BlockIndex(old_block_index)); true } else { @@ -238,17 +239,14 @@ impl Palette { } /// Reevaluate changed blocks. - pub(crate) fn step( - &mut self, - notifier: &listen::Notifier, - ) -> TimeStats { + pub(crate) fn step(&mut self, change_buffer: &mut ChangeBuffer<'_>) -> TimeStats { let mut last_start_time = I::now(); let mut evaluations = TimeStats::default(); { 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::BlockEvaluation(block_index)); + change_buffer.push(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 diff --git a/all-is-cubes/src/space/space_txn.rs b/all-is-cubes/src/space/space_txn.rs index df7547499..39d681e78 100644 --- a/all-is-cubes/src/space/space_txn.rs +++ b/all-is-cubes/src/space/space_txn.rs @@ -225,6 +225,16 @@ impl Transaction for SpaceTransaction { _outputs: &mut dyn FnMut(Self::Output), ) -> Result<(), CommitError> { let mut to_activate = Vec::new(); + + // Create a mutation context, which lets us batch change notifications from this commit. + let mut ctx = crate::space::MutationCtx { + palette: &mut space.palette, + contents: space.contents.as_mut(), + light: &mut space.light, + change_buffer: &mut space.change_notifier.buffer(), + cubes_wanting_ticks: &mut space.cubes_wanting_ticks, + }; + for ( &cube, CubeTransaction { @@ -239,7 +249,7 @@ impl Transaction for SpaceTransaction { let cube = Cube::from(cube); if let Some(new) = new { - match space.set(cube, new) { + match Space::set_impl(&mut ctx, cube, new) { Ok(_) => Ok(()), Err(SetCubeError::OutOfBounds { .. }) if !conserved => { // ignore