Merge InitModifier and UpdateModifier (#292)

Merge the `InitModifier` and `UpdateModifier` supertraits into their
`Modifier` subtrait, and delete them. This unifies handling of modifiers
for the init and update contexts. The modifiers can now easily be
compatible with both contexts by simply returning the appropriate
`ModifierContext` without having to duplicate the implementation into
each supertrait.

At this time, `RenderModifier` still contains some hard-coded elements
not yet transitioned to a fully data driven model (mainly, anything
related to texture resources), and therefore cannot be merged too.
However the longer term plan is to eventually merge it and retain a
single `Modifier` trait.

CHANGELOG.md

@@ -11,17 +11,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added a new `ConformToSphereModifier` acting as an attractor applying a force toward a point (sphere center) to all particles in range, and making particles conform ("stick") to the sphere surface.
 - Added `vec2` and `vec3` functions that allow construction of vectors from dynamic parts.
 - Added missing `VectorValue::as_uvecX()` and `impl From<UVecX> for VectorValue` for all `X=2,3,4`.
+- Added a new `ShaderWriter` helper, common to the init and update modifier contexts, and which replaces the `InitContext` and `UpdateContext`.
+- Added a simple `impl Display for ModifierContext`.
+- Added `Modifier::apply()` which replaces the now deleted `InitModifier::apply_init()` and `UdpdateModifier::apply_update()`.
+- Added `RenderModifier::as_modifier()` to upcast a `RenderModifier` to a `Modifier`.
+- Added `RenderModifier::boxed_render_clone()` to clone a `RenderModifier` into a boxed self (instead of a `BoxedModifier`, as we can't upcast from `Box<dyn RenderModifier>` to `Box<dyn Modifier>`). Added `impl Clone for Box<dyn RenderModifier>` based on this.
+- Added `EffectAsset::add_modifier()` to add a pre-boxed `Modifier` (so, a `BoxedModifier`) to the init or update context, and added `EffectAsset::add_render_modifier()` to add a pre-boxed `RenderModifier` to the render context.
 
 ### Changed
 
 - `ExprHandle` is now `#[repr(transparent)]`, which guarantees that `Option<ExprHandle>` has the same size as `ExprHandle` itself (4 bytes).
 - `EffectProperties::set_if_changed()` now returns the `Mut` variable it takes as input, to allow subsequent calls.
 - `VectorValue::new_uvecX()` now take a `UVecX` instead of individual components, like for all other scalar types.
+- Merged the `InitModifier` and `UpdateModifier` traits into the `Modifier` subtrait; see other changelog entries for details. This helps manage modifiers in a unified way, and generally simplifies writing and maintain modifiers compatible with both the init and update contexts.
+- `EffectAsset::init()` and `EffectAsset::update()` now take a `Modifier`-bound type, and validate its `ModifierContext` is compatible (and panics if not).
+- `EffectAsset::render()` now panics if the modifier is not compatible with the `ModifierContext::Render`. Note that this indicates a malformed render modifier, because all objects implementing `RenderModifier` must include `ModifierContext::Render` in their `Modifier::context()`.
 
 ### Removed
 
 - Removed the `screen_space_size` field from the `SetSizeModifier`. Use the new `ScreenSpaceSizeModifier` to use a screen-space size.
 - Removed the built-in `ForceFieldSource` and associated `ForceFieldModifier`. Use the new `ConformToSphereModifer` instead. The behavior might change a bit as the conforming code is not strictly identical; use the `force_field.rs` example with the `examples_world_inspector` feature to tweak the parameters in real time and observe how they work and change the effect.
+- Removed `InitContext` and `UpdateContext`; they're replaced with `ShaderWriter`.
+- Removed `InitModifer` and `UpdateModifer`. Modifiers for the init and update contexts now only need to implement the base `Modifier` trait.
+- Removed downcast methods from `Modifier` (`as_init()`, `as_init_mut()`, `as_update()`, `as_update_mut()`).
+- Removed the various helper macros `impl_mod_xxx!()` to implement modifier traits; simply implement the trait by hand instead.
 
 ### Fixed
 

examples/init.rs

@@ -65,7 +65,7 @@ const SIZE: Vec2 = Vec2::splat(0.1);
 
 fn base_effect<M, F>(name: impl Into<String>, mut make_modifier: F) -> EffectAsset
 where
-    M: InitModifier + Send + Sync + 'static,
+    M: Modifier + Send + Sync + 'static,
     F: FnMut(&ExprWriter) -> M,
 {
     let writer = ExprWriter::new();

src/asset.rs

@@ -4,12 +4,13 @@ use bevy::{
     utils::{default, thiserror::Error, BoxedFuture, HashSet},
 };
 use serde::{Deserialize, Serialize};
+use std::ops::Deref;
 
 use crate::{
     graph::Value,
-    modifier::{InitModifier, RenderModifier, UpdateModifier},
-    BoxedModifier, ExprHandle, Module, ParticleLayout, Property, PropertyLayout, SimulationSpace,
-    Spawner,
+    modifier::{Modifier, RenderModifier},
+    BoxedModifier, ExprHandle, ModifierContext, Module, ParticleLayout, Property, PropertyLayout,
+    SimulationSpace, Spawner,
 };
 
 /// Type of motion integration applied to the particles of a system.
@@ -212,7 +213,7 @@ pub struct EffectAsset {
     /// Render modifiers defining the effect.
     #[reflect(ignore)]
     // TODO - Can't manage to implement FromReflect for BoxedModifier in a nice way yet
-    render_modifiers: Vec<BoxedModifier>,
+    render_modifiers: Vec<Box<dyn RenderModifier>>,
     /// Properties of the effect.
     ///
     /// Properties must have a unique name. Manually adding two or more
@@ -374,49 +375,101 @@ impl EffectAsset {
 
     /// Add an initialization modifier to the effect.
     ///
-    /// [`with_property()`]: crate::EffectAsset::with_property
-    /// [`add_property()`]: crate::EffectAsset::add_property
+    /// # Panics
+    ///
+    /// Panics if the modifier doesn't support the init context (that is,
+    /// `modifier.context()` returns a flag which doesn't include
+    /// [`ModifierContext::Init`]).
     #[inline]
     pub fn init<M>(mut self, modifier: M) -> Self
     where
-        M: InitModifier + Send + Sync + 'static,
+        M: Modifier + Send + Sync + 'static,
     {
+        assert!(modifier.context().contains(ModifierContext::Init));
         self.init_modifiers.push(Box::new(modifier));
         self
     }
 
     /// Add an update modifier to the effect.
     ///
-    /// [`with_property()`]: crate::EffectAsset::with_property
-    /// [`add_property()`]: crate::EffectAsset::add_property
+    /// # Panics
+    ///
+    /// Panics if the modifier doesn't support the update context (that is,
+    /// `modifier.context()` returns a flag which doesn't include
+    /// [`ModifierContext::Update`]).
     #[inline]
     pub fn update<M>(mut self, modifier: M) -> Self
     where
-        M: UpdateModifier + Send + Sync + 'static,
+        M: Modifier + Send + Sync + 'static,
     {
+        assert!(modifier.context().contains(ModifierContext::Update));
         self.update_modifiers.push(Box::new(modifier));
         self
     }
 
+    /// Add a [`BoxedModifier`] to the specific context.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the context is [`ModifierContext::Render`]; use
+    /// [`add_render_modifier()`] instead.
+    ///
+    /// Panics if the input `context` contains more than one context (the
+    /// bitfield contains more than 1 bit set) or no context at all (zero bit
+    /// set).
+    ///
+    /// Panics if the modifier doesn't support the context specified (that is,
+    /// `modifier.context()` returns a flag which doesn't include `context`).
+    ///
+    /// [`add_render_modifier()`]: crate::EffectAsset::add_render_modifier
+    pub fn add_modifier(mut self, context: ModifierContext, modifier: Box<dyn Modifier>) -> Self {
+        assert!(context == ModifierContext::Init || context == ModifierContext::Update);
+        assert!(modifier.context().contains(context));
+        if context == ModifierContext::Init {
+            self.init_modifiers.push(modifier);
+        } else {
+            self.update_modifiers.push(modifier);
+        }
+        self
+    }
+
     /// Add a render modifier to the effect.
     ///
-    /// [`with_property()`]: crate::EffectAsset::with_property
-    /// [`add_property()`]: crate::EffectAsset::add_property
+    /// # Panics
+    ///
+    /// Panics if the modifier doesn't support the render context (that is,
+    /// `modifier.context()` returns a flag which doesn't include
+    /// [`ModifierContext::Render`]).
     #[inline]
     pub fn render<M>(mut self, modifier: M) -> Self
     where
         M: RenderModifier + Send + Sync + 'static,
     {
+        assert!(modifier.context().contains(ModifierContext::Render));
         self.render_modifiers.push(Box::new(modifier));
         self
     }
 
+    /// Add a [`RenderModifier`] to the render context.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the modifier doesn't support the render context (that is,
+    /// `modifier.context()` returns a flag which doesn't include
+    /// [`ModifierContext::Render`]).
+    pub fn add_render_modifier(mut self, modifier: Box<dyn RenderModifier>) -> Self {
+        assert!(modifier.context().contains(ModifierContext::Render));
+        self.render_modifiers.push(modifier);
+        self
+    }
+
     /// Get a list of all the modifiers of this effect.
-    pub fn modifiers(&self) -> impl Iterator<Item = &BoxedModifier> {
+    pub fn modifiers(&self) -> impl Iterator<Item = &dyn Modifier> {
         self.init_modifiers
             .iter()
-            .chain(self.update_modifiers.iter())
-            .chain(self.render_modifiers.iter())
+            .map(|m| m.deref())
+            .chain(self.update_modifiers.iter().map(|m| m.deref()))
+            .chain(self.render_modifiers.iter().map(|m| m.as_modifier()))
     }
 
     /// Get a list of all the init modifiers of this effect.
@@ -425,8 +478,14 @@ impl EffectAsset {
     /// executing in the [`ModifierContext::Init`] context.
     ///
     /// [`ModifierContext::Init`]: crate::ModifierContext::Init
-    pub fn init_modifiers(&self) -> impl Iterator<Item = &dyn InitModifier> {
-        self.init_modifiers.iter().filter_map(|m| m.as_init())
+    pub fn init_modifiers(&self) -> impl Iterator<Item = &dyn Modifier> {
+        self.init_modifiers.iter().filter_map(|m| {
+            if m.context().contains(ModifierContext::Init) {
+                Some(m.deref())
+            } else {
+                None
+            }
+        })
     }
 
     /// Get a list of all the update modifiers of this effect.
@@ -435,8 +494,14 @@ impl EffectAsset {
     /// executing in the [`ModifierContext::Update`] context.
     ///
     /// [`ModifierContext::Update`]: crate::ModifierContext::Update
-    pub fn update_modifiers(&self) -> impl Iterator<Item = &dyn UpdateModifier> {
-        self.update_modifiers.iter().filter_map(|m| m.as_update())
+    pub fn update_modifiers(&self) -> impl Iterator<Item = &dyn Modifier> {
+        self.update_modifiers.iter().filter_map(|m| {
+            if m.context().contains(ModifierContext::Update) {
+                Some(m.deref())
+            } else {
+                None
+            }
+        })
     }
 
     /// Get a list of all the render modifiers of this effect.
@@ -561,6 +626,31 @@ mod tests {
         assert_eq!(layout.offset("unknown"), None);
     }
 
+    #[test]
+    fn add_modifiers() {
+        let mut m = Module::default();
+        let expr = m.lit(3.);
+
+        for modifier_context in [ModifierContext::Init, ModifierContext::Update] {
+            let effect = EffectAsset::default().add_modifier(
+                modifier_context,
+                Box::new(SetAttributeModifier::new(Attribute::POSITION, expr)),
+            );
+            assert_eq!(effect.modifiers().count(), 1);
+            let m = effect.modifiers().next().unwrap();
+            assert!(m.context().contains(modifier_context));
+        }
+
+        {
+            let effect = EffectAsset::default().add_render_modifier(Box::new(SetColorModifier {
+                color: CpuValue::Single(Vec4::ONE),
+            }));
+            assert_eq!(effect.modifiers().count(), 1);
+            let m = effect.modifiers().next().unwrap();
+            assert!(m.context().contains(ModifierContext::Render));
+        }
+    }
+
     #[test]
     fn test_apply_modifiers() {
         let mut module = Module::default();
@@ -600,33 +690,29 @@ mod tests {
 
         let property_layout = PropertyLayout::default();
         let particle_layout = ParticleLayout::default();
-        let mut init_context = InitContext::new(&property_layout, &particle_layout);
+        let mut init_context =
+            ShaderWriter::new(ModifierContext::Init, &property_layout, &particle_layout);
         assert!(init_pos_sphere
-            .apply_init(&mut module, &mut init_context)
+            .apply(&mut module, &mut init_context)
             .is_ok());
         assert!(init_vel_sphere
-            .apply_init(&mut module, &mut init_context)
-            .is_ok());
-        assert!(init_age.apply_init(&mut module, &mut init_context).is_ok());
-        assert!(init_lifetime
-            .apply_init(&mut module, &mut init_context)
+            .apply(&mut module, &mut init_context)
             .is_ok());
+        assert!(init_age.apply(&mut module, &mut init_context).is_ok());
+        assert!(init_lifetime.apply(&mut module, &mut init_context).is_ok());
         // assert_eq!(effect., init_context.init_code);
 
         let mut module = Module::default();
         let accel_mod = AccelModifier::constant(&mut module, Vec3::ONE);
         let drag_mod = LinearDragModifier::constant(&mut module, 3.5);
         let property_layout = PropertyLayout::default();
         let particle_layout = ParticleLayout::default();
-        let mut update_context = UpdateContext::new(&property_layout, &particle_layout);
-        assert!(accel_mod
-            .apply_update(&mut module, &mut update_context)
-            .is_ok());
-        assert!(drag_mod
-            .apply_update(&mut module, &mut update_context)
-            .is_ok());
+        let mut update_context =
+            ShaderWriter::new(ModifierContext::Update, &property_layout, &particle_layout);
+        assert!(accel_mod.apply(&mut module, &mut update_context).is_ok());
+        assert!(drag_mod.apply(&mut module, &mut update_context).is_ok());
         assert!(ConformToSphereModifier::new(origin, one, one, one, one)
-            .apply_update(&mut module, &mut update_context)
+            .apply(&mut module, &mut update_context)
             .is_ok());
         // assert_eq!(effect.update_layout, update_layout);
 

src/attributes.rs

@@ -1576,10 +1576,7 @@ impl ParticleLayout {
 mod tests {
     use super::*;
 
-    use bevy::{
-        math::{Vec2, Vec3, Vec4},
-        reflect::TypeRegistration,
-    };
+    use bevy::reflect::TypeRegistration;
     use naga::{front::wgsl::Frontend, proc::Layouter};
 
     // Ensure the size and alignment of all types conforms to the WGSL spec by

src/gradient.rs

@@ -4,10 +4,7 @@ use bevy::{
     utils::FloatOrd,
 };
 use serde::{Deserialize, Serialize};
-use std::{
-    hash::{Hash, Hasher},
-    vec::Vec,
-};
+use std::hash::{Hash, Hasher};
 
 /// Describes a type that can be linearly interpolated between two keys.
 ///