Invalidate more bind group on changes (#414)

Invalidate the bind groups for the simulation, containing the particle
group buffer, when that buffer is reallocated or when the group changes
position in the buffer. This was causing old buffers to be referenced
when the group(s) changed, creating artifacts in ribbon rendering in
particular.

Similarly, invalidate the bind groups referencing a property buffer when
that buffer is reallocated, for the same reasons. This fixes a crash
previously introduced by the new property buffer change.

CHANGELOG.md

@@ -9,14 +9,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Changed the `ParticleEffect` component to require the `CompiledParticleEffect` and `SyncToRenderWorld` components.
 - Renamed `PropertyLayout::size()` to `cpu_size()` to prevent confusion.
-  The GPU size is given by `min_binding_size()`, and can be greater.
+  The GPU size is given by `min_binding_size()`, and can be greater due to WGSL padding rules being different from Rust ones.
 - `PropertyLayout::generate_code()` now returns an `Option<String>` for clarity,
   which is `None` if the layout is empty (as opposed to an empty string previously).
+- Removed effects are now deallocated from the render world before the extract schedule, instead of during it.
+  This should have no consequence, unless you were using a system inserted explicitly before Hanabi's extraction.
 
 ### Fixed
 
 - Fixed a bug with opaque effects not rendering.
 
+### Removed
+
+- Removed the `EffectSystems::GatherRemovedEffects` system set.
+  Removed effects are now processed via observers, which execute during the render world sync just before extraction.
+  Removed the `RemovedEffectsEvent` type too.
+
 ## [0.14.0] 2024-12-09
 
 ### Added

src/lib.rs

@@ -172,11 +172,7 @@ use std::fmt::Write as _;
 
 #[cfg(feature = "2d")]
 use bevy::math::FloatOrd;
-use bevy::{
-    prelude::*,
-    render::sync_world::{MainEntity, RenderEntity, SyncToRenderWorld},
-    utils::HashSet,
-};
+use bevy::{prelude::*, render::sync_world::SyncToRenderWorld, utils::HashSet};
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 
@@ -1564,8 +1560,7 @@ fn compile_effects(
     {
         // If the ParticleEffect didn't change, and the compiled one is for the correct
         // asset, then there's nothing to do.
-        let need_rebuild =
-            effect.is_changed() || material.as_ref().map_or(false, |r| r.is_changed());
+        let need_rebuild = effect.is_changed() || material.as_ref().is_some_and(|r| r.is_changed());
         if !need_rebuild && (compiled_effect.asset == effect.handle) {
             continue;
         }
@@ -1633,47 +1628,6 @@ fn update_properties_from_asset(
     }
 }
 
-/// Event sent by [`gather_removed_effects()`] with the list of effects removed
-/// during this frame.
-///
-/// The event is consumed during the extract phase by the [`extract_effects()`]
-/// system, to clean-up unused GPU resources.
-///
-/// [`extract_effects()`]: crate::render::extract_effects
-#[derive(Event)]
-struct RemovedEffectsEvent {
-    entities: Vec<(MainEntity, Option<RenderEntity>)>,
-}
-
-/// Gather all the removed [`ParticleEffect`] components to allow cleaning-up
-/// unused GPU resources.
-///
-/// This system executes inside the [`EffectSystems::GatherRemovedEffects`]
-/// set of the [`PostUpdate`] schedule.
-fn gather_removed_effects(
-    q_effects: Query<Option<RenderEntity>, With<ParticleEffect>>,
-    mut removed_effects: RemovedComponents<ParticleEffect>,
-    mut removed_effects_event_writer: EventWriter<RemovedEffectsEvent>,
-) {
-    let entities: Vec<Entity> = removed_effects.read().collect();
-    if !entities.is_empty() {
-        let entities = entities
-            .iter()
-            .map(|main_entity| {
-                (
-                    MainEntity::from(*main_entity),
-                    q_effects
-                        .get(*main_entity)
-                        .ok()
-                        .flatten()
-                        .map(RenderEntity::from),
-                )
-            })
-            .collect();
-        removed_effects_event_writer.send(RemovedEffectsEvent { entities });
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use std::ops::DerefMut;

src/plugin.rs

@@ -22,22 +22,23 @@ use bevy::{
 use crate::asset::EffectAssetLoader;
 use crate::{
     asset::EffectAsset,
-    compile_effects, gather_removed_effects,
+    compile_effects,
     properties::EffectProperties,
     render::{
-        add_remove_effects, batch_effects, extract_effect_events, extract_effects,
-        prepare_bind_groups, prepare_effects, prepare_gpu_resources, queue_effects,
-        DispatchIndirectPipeline, DrawEffects, EffectAssetEvents, EffectBindGroups, EffectCache,
-        EffectsMeta, ExtractedEffects, GpuDispatchIndirect, GpuParticleGroup,
-        GpuRenderEffectMetadata, GpuRenderGroupIndirect, GpuSpawnerParams, ParticlesInitPipeline,
-        ParticlesRenderPipeline, ParticlesUpdatePipeline, PropertyCache, ShaderCache, SimParams,
-        StorageType as _, VfxSimulateDriverNode, VfxSimulateNode,
+        add_effects, batch_effects, extract_effect_events, extract_effects,
+        on_remove_cached_effect, on_remove_cached_properties, prepare_bind_groups, prepare_effects,
+        prepare_gpu_resources, queue_effects, DispatchIndirectPipeline, DrawEffects,
+        EffectAssetEvents, EffectBindGroups, EffectCache, EffectsMeta, ExtractedEffects,
+        GpuDispatchIndirect, GpuParticleGroup, GpuRenderEffectMetadata, GpuRenderGroupIndirect,
+        GpuSpawnerParams, ParticlesInitPipeline, ParticlesRenderPipeline, ParticlesUpdatePipeline,
+        PropertyCache, ShaderCache, SimParams, StorageType as _, VfxSimulateDriverNode,
+        VfxSimulateNode,
     },
     spawn::{self, Random},
     tick_initializers,
     time::effect_simulation_time_system,
     update_properties_from_asset, CompiledParticleEffect, EffectSimulation, ParticleEffect,
-    RemovedEffectsEvent, Spawner,
+    Spawner,
 };
 
 /// Labels for the Hanabi systems.
@@ -69,16 +70,6 @@ pub enum EffectSystems {
     /// be taken into account in the same frame.
     UpdatePropertiesFromAsset,
 
-    /// Gather all removed [`ParticleEffect`] components during the
-    /// [`PostUpdate`] set, to clean-up unused GPU resources.
-    ///
-    /// Systems deleting entities with a [`ParticleEffect`] component should run
-    /// before this set if they want the particle effect is cleaned-up during
-    /// the same frame.
-    ///
-    /// [`ParticleEffect`]: crate::ParticleEffect
-    GatherRemovedEffects,
-
     /// Prepare effect assets for the extracted effects.
     ///
     /// Part of Bevy's own [`RenderSet::PrepareAssets`].
@@ -191,7 +182,6 @@ impl Plugin for HanabiPlugin {
     fn build(&self, app: &mut App) {
         // Register asset
         app.init_asset::<EffectAsset>()
-            .add_event::<RemovedEffectsEvent>()
             .insert_resource(Random(spawn::new_rng()))
             .init_resource::<ShaderCache>()
             .init_resource::<Time<EffectSimulation>>()
@@ -203,7 +193,6 @@ impl Plugin for HanabiPlugin {
                         // ComputedVisibility was updated.
                         .after(VisibilitySystems::VisibilityPropagate),
                     EffectSystems::CompileEffects,
-                    EffectSystems::GatherRemovedEffects,
                 ),
             )
             .configure_sets(
@@ -222,7 +211,6 @@ impl Plugin for HanabiPlugin {
                     tick_initializers.in_set(EffectSystems::TickSpawners),
                     compile_effects.in_set(EffectSystems::CompileEffects),
                     update_properties_from_asset.in_set(EffectSystems::UpdatePropertiesFromAsset),
-                    gather_removed_effects.in_set(EffectSystems::GatherRemovedEffects),
                     check_visibility::<WithCompiledParticleEffect>
                         .in_set(VisibilitySystems::CheckVisibility),
                 ),
@@ -314,7 +302,7 @@ impl Plugin for HanabiPlugin {
             .add_systems(
                 Render,
                 (
-                    (add_remove_effects, prepare_effects, batch_effects)
+                    (add_effects, prepare_effects, batch_effects)
                         .chain()
                         .in_set(EffectSystems::PrepareEffectAssets)
                         // Ensure we run after Bevy prepared the render Mesh
@@ -331,6 +319,10 @@ impl Plugin for HanabiPlugin {
                         .after(prepare_assets::<GpuImage>),
                 ),
             );
+        render_app.world_mut().add_observer(on_remove_cached_effect);
+        render_app
+            .world_mut()
+            .add_observer(on_remove_cached_properties);
 
         // Register the draw function for drawing the particles. This will be called
         // during the main 2D/3D pass, at the Transparent2d/3d phase, after the

src/render/aligned_buffer_vec.rs

@@ -388,10 +388,15 @@ impl HybridAlignedBufferVec {
         self.capacity
     }
 
-    // #[inline]
-    // pub fn len(&self) -> usize {
-    //     self.values.len()
-    // }
+    /// Current buffer size, in bytes.
+    ///
+    /// This represents the size of the CPU data uploaded to GPU. Pending a GPU
+    /// buffer re-allocation or re-upload, this size might differ from the
+    /// actual GPU buffer size. But they're eventually consistent.
+    #[inline]
+    pub fn len(&self) -> usize {
+        self.values.len()
+    }
 
     /// Alignment, in bytes, of all the elements.
     #[allow(dead_code)]
@@ -508,7 +513,6 @@ impl HybridAlignedBufferVec {
         // Insert into existing space
         if let Some((_, index)) = best_slot {
             let row_range = self.free_rows.remove(index);
-            debug_assert_eq!(row_range.0.start as usize % self.item_align, 0);
             let offset = row_range.0.start as usize * self.item_align;
             let free_size = (row_range.0.end - row_range.0.start) as usize * self.item_align;
             let size = src.len();
@@ -535,7 +539,8 @@ impl HybridAlignedBufferVec {
             let size = src.len();
             let new_capacity = offset + size;
             if new_capacity > self.values.capacity() {
-                self.values.reserve(new_capacity - self.values.len())
+                let additional = new_capacity - self.values.len();
+                self.values.reserve(additional)
             }
 
             // Insert padding if needed