Use strong handle in CompiledParticleEffect (djeedai#306)

This fixes a panic in the extract code, where the `EffectAsset` was
unloaded after the particle effect was compiled but before the
extraction finished. The idea of using a weak handle was to avoid
preventing the asset from unloading, but because
`CompiledParticleEffect` will anyway observe any change on the source
`ParticleEffect` and its handle, clearing that handle will make
`CompiledParticleEffect` also clear its own handle, even if strong,
which will result in the asset being "freed" from references held by
Hanabi.

Fixes djeedai#289

CHANGELOG.md

@@ -39,9 +39,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Expr` is now `Copy`, making it even more lightweight.
 - `ExprWriter::prop()` now panics if the property doesn't exist. This ensures the created `WriterExpr` is well-formed, which was impossible to validate at expression write time previously.
 - Effects rendered with `AlphaMode::Mask` now write to the depth buffer. Other effects continue to not write to it. This fixes particle flickering for alpha masked effects only.
-git
 - Bind groups for effect rendering are now created in a separate system in the `EffectSystems::PrepareBindGroups` set, itself part of Bevy's `RenderSet::PrepareBindGroups`. They're also cached, which increases the performance of rendering many effects.
 - Merged the init and update pass bind groups for the particle buffer and associated resources in `EffectBfufer`. The new unified resources use the `_sim` (simulation) suffix.
+- `CompiledParticleEffect` now holds a strong handle to the same `EffectAsset` as the `ParticleEffect` it's compiled from. This ensures the asset is not unloaded while in use during the frame. To allow an `EffectAsset` to unload, clear the handle of the `ParticleEffect`, then allow the `CompiledParticleEffect` to observe the change and clear its own handle too.
 
 ### Removed
 

src/lib.rs

@@ -1096,7 +1096,7 @@ impl EffectShaderSource {
 /// ([`Visibility::Visible`]).
 #[derive(Debug, Clone, Component)]
 pub struct CompiledParticleEffect {
-    /// Weak handle to the underlying asset.
+    /// Handle to the underlying asset.
     asset: Handle<EffectAsset>,
     /// Cached simulation condition, to avoid having to query the asset each
     /// time we need it.
@@ -1127,12 +1127,19 @@ impl Default for CompiledParticleEffect {
 }
 
 impl CompiledParticleEffect {
+    /// Clear the compiled data from this component.
+    pub(crate) fn clear(&mut self) {
+        self.asset = Handle::default();
+        self.effect_shader = None;
+        self.particle_texture = None;
+    }
+
     /// Update the compiled effect from its asset and instance.
     pub(crate) fn update(
         &mut self,
         rebuild: bool,
         #[cfg(feature = "2d")] z_layer_2d: FloatOrd,
-        weak_handle: Handle<EffectAsset>,
+        handle: Handle<EffectAsset>,
         asset: &EffectAsset,
         shaders: &mut ResMut<Assets<Shader>>,
         shader_cache: &mut ResMut<ShaderCache>,
@@ -1141,16 +1148,21 @@ impl CompiledParticleEffect {
             "Updating (rebuild:{}) compiled particle effect '{}' ({:?})",
             rebuild,
             asset.name,
-            weak_handle
+            handle
         );
 
-        debug_assert!(weak_handle.is_weak());
+        // #289 - Panic in fn extract_effects
+        // We now keep a strong handle. Since CompiledParticleEffect is kept in sync
+        // with the source ParticleEffect, this shouldn't produce any strong cyclic
+        // dependency.
+        debug_assert!(handle.is_strong());
+
         // Note: if something marked the ParticleEffect as changed (via Mut for example)
         // but didn't actually change anything, or at least didn't change the asset,
         // then we may end up here with the same asset handle. Don't try to be
         // too smart, and rebuild everything anyway, it's easier than trying to
         // diff what may or may not have changed.
-        self.asset = weak_handle;
+        self.asset = handle;
         self.simulation_condition = asset.simulation_condition;
 
         // Check if the instance changed. If so, rebuild some data from this compiled
@@ -1377,12 +1389,19 @@ fn compile_effects(
             need_rebuild,
             #[cfg(feature = "2d")]
             z_layer_2d,
-            effect.handle.clone_weak(),
+            effect.handle.clone(),
             asset,
             &mut shaders,
             &mut shader_cache,
         );
     }
+
+    // Clear removed effects, to allow them to be released by the asset server
+    for (_, effect, mut compiled_effect) in q_effects.iter_mut() {
+        if effects.get(&effect.handle).is_none() {
+            compiled_effect.clear();
+        }
+    }
 }
 
 /// Update all properties of a [`ParticleEffect`] into its associated
@@ -1902,7 +1921,7 @@ else { return c1; }
 
             // `compile_effects()` always updates the CompiledParticleEffect
             assert_eq!(compiled_particle_effect.asset, handle);
-            assert!(compiled_particle_effect.asset.is_weak());
+            assert!(compiled_particle_effect.asset.is_strong());
             assert!(compiled_particle_effect.effect_shader.is_some());
         }
 
@@ -1937,7 +1956,7 @@ else { return c1; }
 
             // `compile_effects()` always updates the CompiledParticleEffect
             assert_eq!(compiled_particle_effect.asset, handle);
-            assert!(compiled_particle_effect.asset.is_weak());
+            assert!(compiled_particle_effect.asset.is_strong());
             assert!(compiled_particle_effect.effect_shader.is_some());
         }
     }
@@ -2040,7 +2059,7 @@ else { return c1; }
                 // `compile_effects()` always updates the CompiledParticleEffect of new effects,
                 // even if hidden
                 assert_eq!(compiled_particle_effect.asset, handle);
-                assert!(compiled_particle_effect.asset.is_weak());
+                assert!(compiled_particle_effect.asset.is_strong());
                 assert!(compiled_particle_effect.effect_shader.is_some());
 
                 // Toggle visibility and tick once more; this shouldn't panic (regression; #182)
@@ -2068,7 +2087,7 @@ else { return c1; }
 
                 // `compile_effects()` always updates the CompiledParticleEffect
                 assert_eq!(compiled_particle_effect.asset, handle);
-                assert!(compiled_particle_effect.asset.is_weak());
+                assert!(compiled_particle_effect.asset.is_strong());
                 assert!(compiled_particle_effect.effect_shader.is_some());
             }
         }