Invalidate some bind group on buffer reallocate

examples/utils/mod.rs

@@ -2,16 +2,16 @@
 
 use std::{fmt::Display, num::NonZeroU8};
 
-use crate::prelude::*;
 use bevy::{
     log::LogPlugin,
     prelude::*,
     render::{settings::WgpuSettings, RenderPlugin},
 };
-
 #[cfg(feature = "examples_world_inspector")]
 use bevy_inspector_egui::quick::WorldInspectorPlugin;
 
+use crate::prelude::*;
+
 /// Helper system to enable closing the example application by pressing the
 /// escape key (ESC).
 pub fn close_on_esc(mut ev_app_exit: EventWriter<AppExit>, input: Res<ButtonInput<KeyCode>>) {

src/asset.rs

@@ -1,18 +1,16 @@
 use std::ops::Deref;
 
+#[cfg(feature = "serde")]
+use bevy::asset::{io::Reader, AssetLoader, AsyncReadExt, LoadContext};
 use bevy::{
     asset::Asset,
     reflect::Reflect,
     utils::{default, HashSet},
 };
-
-#[cfg(feature = "serde")]
-use bevy::asset::{io::Reader, AssetLoader, AsyncReadExt, LoadContext};
+use serde::{Deserialize, Serialize};
 #[cfg(feature = "serde")]
 use thiserror::Error;
 
-use serde::{Deserialize, Serialize};
-
 use crate::{
     modifier::{Modifier, RenderModifier},
     spawn::{Cloner, Initializer},

src/plugin.rs

@@ -15,6 +15,8 @@ use bevy::{
     time::{time_system, TimeSystem},
 };
 
+#[cfg(feature = "serde")]
+use crate::asset::EffectAssetLoader;
 use crate::{
     asset::EffectAsset,
     compile_effects, gather_removed_effects,
@@ -34,9 +36,6 @@ use crate::{
     RemovedEffectsEvent, Spawner,
 };
 
-#[cfg(feature = "serde")]
-use crate::asset::EffectAssetLoader;
-
 /// Labels for the Hanabi systems.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, SystemSet)]
 pub enum EffectSystems {

src/render/effect_cache.rs

@@ -454,9 +454,14 @@ impl EffectBuffer {
         render_device: &RenderDevice,
         group_binding: BufferBinding,
     ) {
-        if self.simulate_bind_group.is_some() {
-            return;
-        }
+        // Note: We can't cache this bind group, because not only the group buffer can
+        // get reallocated (we do track that), but the offset/size of the group binding
+        // can change each frame, and we don't track them (we re-allocate them
+        // linearly each frame). So just re-create that bind group each frame
+        // too.
+        // FIXME: Check if Buffer::id is unique for the buffer lifetime. Also we could
+        // track the offset/size manually to check for a change, and
+        // auto-invalidate.
 
         let layout = self.particle_layout_bind_group_sim();
         let label = format!("hanabi:bind_group_sim_batch{}", buffer_index);
@@ -489,11 +494,28 @@ impl EffectBuffer {
         self.simulate_bind_group = Some(bind_group);
     }
 
+    /// Invalidate any existing simulate bind group.
+    ///
+    /// Invalidate any existing bind group previously created by
+    /// [`create_sim_bind_group()`], generally because a buffer was
+    /// re-allocated. This forces a re-creation of the bind group
+    /// next time [`create_sim_bind_group()`] is called.
+    ///
+    /// [`create_sim_bind_group()`]: self::EffectBuffer::create_sim_bind_group
+    fn invalidate_sim_bind_group(&mut self) {
+        self.simulate_bind_group = None;
+    }
+
     /// Return the cached bind group for the init and update passes.
     ///
     /// This is the per-buffer bind group at binding @1 which binds all
     /// per-buffer resources shared by all effect instances batched in a single
-    /// buffer.
+    /// buffer. The bind group is created by [`create_sim_bind_group()`], and
+    /// cached until a call to [`invalidate_sim_bind_group()`] clears the cached
+    /// reference.
+    ///
+    /// [`create_sim_bind_group()`]: self::EffectBuffer::create_sim_bind_group
+    /// [`invalidate_sim_bind_group()`]: self::EffectBuffer::invalidate_sim_bind_group
     pub fn sim_bind_group(&self) -> Option<&BindGroup> {
         self.simulate_bind_group.as_ref()
     }
@@ -740,6 +762,18 @@ impl EffectCache {
         &mut self.buffers
     }
 
+    /// Invalidate all the "simulation" bind groups for all buffers.
+    ///
+    /// This iterates over all valid buffers and calls
+    /// [`EffectBuffer::invalidate_sim_bind_group()`] on each one.
+    pub fn invalidate_sim_bind_groups(&mut self) {
+        for buffer in &mut self.buffers {
+            if let Some(buffer) = buffer {
+                buffer.invalidate_sim_bind_group();
+            }
+        }
+    }
+
     pub fn insert(
         &mut self,
         asset: Handle<EffectAsset>,

src/render/mod.rs

@@ -2341,6 +2341,10 @@ pub(crate) fn prepare_effects(
         .write_buffer(&render_device, &render_queue)
     {
         // The buffer changed; invalidate all bind groups for all effects.
+        effect_cache.invalidate_sim_bind_groups();
+
+        // Note: effects_meta.dr_indirect_bind_group is re-created each frame,
+        // no need to clear explicitly.
     }
 
     // Update simulation parameters
@@ -3303,7 +3307,6 @@ pub(crate) fn prepare_bind_groups(
         };
 
         // Bind group for the init compute shader to simulate particles.
-        // TODO - move this creation in RenderSet::PrepareBindGroups
         effect_buffer.create_sim_bind_group(
             effect_batches.buffer_index,
             &render_device,
@@ -3882,7 +3885,7 @@ impl Node for VfxSimulateNode {
         }
 
         // Compute init pass
-        // let mut total_group_count = 0;
+        let mut total_group_count = 0;
         {
             {
                 trace!("loop over effect batches...");
@@ -3920,12 +3923,7 @@ impl Node for VfxSimulateNode {
                                     .dispatch_buffer_indices
                                     .render_effect_metadata_buffer_index;
 
-                                // FIXME - Currently we unconditionally count
-                                // all groups because the dispatch pass always
-                                // runs on all groups. We should consider if
-                                // it's worth skipping e.g. dormant or finished
-                                // effects at the cost of extra complexity.
-                                // total_group_count += batches.group_batches.len() as u32;
+                                total_group_count += batches.group_batches.len() as u32;
 
                                 let Some(init_pipeline) = pipeline_cache.get_compute_pipeline(
                                     batches.init_and_update_pipeline_ids[dest_group_index as usize]
@@ -4169,38 +4167,37 @@ impl Node for VfxSimulateNode {
                         timestamp_writes: None,
                     });
 
-            // Dispatch indirect dispatch compute job
-            trace!("record commands for indirect dispatch pipeline...");
-
-            // FIXME - The `vfx_indirect` shader assumes a contiguous array of ParticleGroup
-            // structures. So we need to pass the full array size, and we
-            // just update the unused groups for nothing. Otherwise we might
-            // update some unused group and miss some used ones, if there's any gap
-            // in the array.
-            const WORKGROUP_SIZE: u32 = 64;
-            let total_group_count = effects_meta.particle_group_buffer.len() as u32;
-            let workgroup_count = (total_group_count + WORKGROUP_SIZE - 1) / WORKGROUP_SIZE;
-
-            // Setup compute pass
-            compute_pass.set_pipeline(&dispatch_indirect_pipeline.pipeline);
-            compute_pass.set_bind_group(
-                0,
-                // FIXME - got some unwrap() panic here, investigate... possibly race
-                // condition!
-                effects_meta.dr_indirect_bind_group.as_ref().unwrap(),
-                &[],
-            );
-            compute_pass.set_bind_group(
-                1,
-                effects_meta.sim_params_bind_group.as_ref().unwrap(),
-                &[],
-            );
-            compute_pass.dispatch_workgroups(workgroup_count, 1, 1);
-            trace!(
-                "indirect dispatch compute dispatched: num_batches={} workgroup_count={}",
-                total_group_count,
-                workgroup_count
-            );
+            // Dispatch indirect dispatch compute job.
+            // FIXME - we do an explicit check again here because there was an unwrap()
+            // which triggered some panic. Because we also check above, this
+            // means there's likely a race condition hidden somewhere.
+            if let (Some(dr_indirect_bind_group), Some(sim_params_bind_group)) = (
+                effects_meta.dr_indirect_bind_group.as_ref(),
+                effects_meta.sim_params_bind_group.as_ref(),
+            ) {
+                trace!("record commands for indirect dispatch pipeline for {total_group_count} groups...");
+
+                const WORKGROUP_SIZE: u32 = 64;
+                let workgroup_count = (total_group_count + WORKGROUP_SIZE - 1) / WORKGROUP_SIZE;
+
+                // Setup compute pass
+                compute_pass.set_pipeline(&dispatch_indirect_pipeline.pipeline);
+                compute_pass.set_bind_group(0, dr_indirect_bind_group, &[]);
+                compute_pass.set_bind_group(1, sim_params_bind_group, &[]);
+                compute_pass.dispatch_workgroups(workgroup_count, 1, 1);
+                trace!(
+                    "indirect dispatch compute dispatched: num_batches={} workgroup_count={}",
+                    total_group_count,
+                    workgroup_count
+                );
+            } else {
+                if effects_meta.dr_indirect_bind_group.is_none() {
+                    warn!("Missing indirect dispatch bind group. This is a bug.");
+                }
+                if effects_meta.sim_params_bind_group.is_none() {
+                    warn!("Missing indirect dispatch SimParams bind group. This is a bug.");
+                }
+            }
         }
 
         // Compute update pass

src/render/vfx_indirect.wgsl

@@ -22,14 +22,9 @@ fn main(@builtin(global_invocation_id) global_invocation_id: vec3<u32>) {
 
     // Cap at maximum number of groups to process
     let index = global_invocation_id.x;
-
-    // FIXME - the group_buffer array has gaps, we can't limit to the number of effects in use
-    // otherwise we'll miss some and process the unused gaps instead of some active ones.
-    // Since all writes below are idempotent, except the ping/pong one, there's no harm updating
-    // unused effect rows.
-    // if (index >= sim_params.num_groups) {
-    //     return;
-    // }
+    if (index >= sim_params.num_groups) {
+        return;
+    }
 
     // Cap at group array size, just for safety
     if (index >= arrayLength(&group_buffer)) {