Fix incorrect change detection triggering (#71)

Ensure change detection on components and assets is only triggered when
an animator effectively modifies said component or asset, and not
invariably just by the simple fact of the animator ticking each frame.

This change modifies the signature of the `component_animator_system()`
and `asset_animator_system()` public functions to directly consume a
`ResMut<Events<TweenCompleted>>` instead of an
`EventWriter<TweenCompleted>`, to work around some internal limitations.

It also publicly exposes a new `Targetable` trait used to work around
the impossibility to retrieve a `Mut<T>` from a `Mut<Assets<T>>`.
Instead, the trait provides an "target dereferencing" method
`target_mut()` which dereferences the target component or asset, and
triggers its change detection. The trait is implemented for all
components via the `ComponentTarget` object, and for all assets via the
`AssetTarget` object. The 3 types described here are publicly exposed to
workaround some Bevy limitations, and because the trait appears in the
public `Tweenable<T>` API. However users are discouraged from taking
strong dependencies on those, as they will be removed once Bevy provides
a way to achieve this conditionaly dereferencing without this
workaround.

Fixes #33

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added a `speed()` getter on `Animator<T>` and `AssetAnimator<T>`.
 - Added `set_elapsed(Duration)` and `elapsed() -> Duration` to the `Tweenable<T>` trait. Those methods are preferable over `set_progress()` and `progress()` as they avoid the conversion to floating-point values and any rounding errors.
 - Added a new `bevy_text` feature for `Text`-related built-in lenses.
+- Added `Targetable`, `ComponentTarget`, and `AssetTarget`, which should be considered private even though they appear in the public API. They are a workaround for Bevy 0.8 and will likely be removed in the future once the related Bevy limitation is lifted.
 
 ### Changed
 
@@ -20,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Delay::new()` now panics if the `duration` is zero. This prevents creating no-op `Delay` objects, and avoids an internal edge case producing wrong results.
 - Tweens moving to `TweenState::Completed` are now guaranteed to freeze their state. In particular, this means that their direction will not flip at the end of the last loop if their repeat strategy is `RepeatStrategy::MirroredRepeat`.
 - Moved the `TextColorLens` lens from the `bevy_ui` feature to the new `bevy_text` one, to allow using it without the Bevy UI crate.
+- Changed the signature of the `component_animator_system()` and `asset_animator_system()` public functions to directly consume a `ResMut<Events<TweenCompleted>>` instead of an `EventWriter<TweenCompleted>`, to work around some internal limitations.
 
 ### Removed
 
@@ -29,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fixed the animator speed feature, which got broken in #44.
+- Fixed change detection triggering unconditionally when `component_animator_system()` or `asset_animator_system()` are ticked, even when the animator did not mutate its target component or asset. (#33)
 
 ## [0.5.0] - 2022-08-04
 

src/lib.rs

@@ -164,7 +164,8 @@ pub use lens::Lens;
 pub use plugin::asset_animator_system;
 pub use plugin::{component_animator_system, AnimationSystem, TweeningPlugin};
 pub use tweenable::{
-    BoxedTweenable, Delay, Sequence, Tracks, Tween, TweenCompleted, TweenState, Tweenable,
+    BoxedTweenable, Delay, Sequence, Targetable, Tracks, Tween, TweenCompleted, TweenState,
+    Tweenable,
 };
 
 pub mod lens;

src/plugin.rs

@@ -3,8 +3,8 @@ use bevy::asset::Asset;
 use bevy::{ecs::component::Component, prelude::*};
 
 #[cfg(feature = "bevy_asset")]
-use crate::AssetAnimator;
-use crate::{Animator, AnimatorState, TweenCompleted};
+use crate::{tweenable::AssetTarget, AssetAnimator};
+use crate::{tweenable::ComponentTarget, Animator, AnimatorState, TweenCompleted};
 
 /// Plugin to add systems related to tweening of common components and assets.
 ///
@@ -71,16 +71,18 @@ pub enum AnimationSystem {
 pub fn component_animator_system<T: Component>(
     time: Res<Time>,
     mut query: Query<(Entity, &mut T, &mut Animator<T>)>,
-    mut event_writer: EventWriter<TweenCompleted>,
+    events: ResMut<Events<TweenCompleted>>,
 ) {
-    for (entity, ref mut target, ref mut animator) in query.iter_mut() {
+    let mut events: Mut<Events<TweenCompleted>> = events.into();
+    for (entity, target, mut animator) in query.iter_mut() {
         if animator.state != AnimatorState::Paused {
             let speed = animator.speed();
+            let mut target = ComponentTarget::new(target);
             animator.tweenable_mut().tick(
                 time.delta().mul_f32(speed),
-                target,
+                &mut target,
                 entity,
-                &mut event_writer,
+                &mut events,
             );
         }
     }
@@ -95,21 +97,169 @@ pub fn component_animator_system<T: Component>(
 #[cfg(feature = "bevy_asset")]
 pub fn asset_animator_system<T: Asset>(
     time: Res<Time>,
-    mut assets: ResMut<Assets<T>>,
+    assets: ResMut<Assets<T>>,
     mut query: Query<(Entity, &mut AssetAnimator<T>)>,
-    mut event_writer: EventWriter<TweenCompleted>,
+    events: ResMut<Events<TweenCompleted>>,
 ) {
-    for (entity, ref mut animator) in query.iter_mut() {
+    let mut events: Mut<Events<TweenCompleted>> = events.into();
+    let mut target = AssetTarget::new(assets);
+    for (entity, mut animator) in query.iter_mut() {
         if animator.state != AnimatorState::Paused {
+            target.handle = animator.handle().clone();
+            if !target.is_valid() {
+                continue;
+            }
             let speed = animator.speed();
-            if let Some(target) = assets.get_mut(&animator.handle()) {
-                animator.tweenable_mut().tick(
-                    time.delta().mul_f32(speed),
-                    target,
-                    entity,
-                    &mut event_writer,
-                );
+            animator.tweenable_mut().tick(
+                time.delta().mul_f32(speed),
+                &mut target,
+                entity,
+                &mut events,
+            );
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use bevy::prelude::{Events, IntoSystem, System, Transform, World};
+
+    use crate::{lens::TransformPositionLens, *};
+
+    /// A simple isolated test environment with a [`World`] and a single
+    /// [`Entity`] in it.
+    struct TestEnv {
+        world: World,
+        entity: Entity,
+    }
+
+    impl TestEnv {
+        /// Create a new test environment containing a single entity with a
+        /// [`Transform`], and add the given animator on that same entity.
+        pub fn new<T: Component>(animator: T) -> Self {
+            let mut world = World::new();
+            world.init_resource::<Events<TweenCompleted>>();
+
+            let mut time = Time::default();
+            time.update();
+            world.insert_resource(time);
+
+            let entity = world
+                .spawn()
+                .insert(Transform::default())
+                .insert(animator)
+                .id();
+
+            Self { world, entity }
+        }
+
+        /// Get the test world.
+        pub fn world_mut(&mut self) -> &mut World {
+            &mut self.world
+        }
+
+        /// Tick the test environment, updating the simulation time and ticking
+        /// the given system.
+        pub fn tick(&mut self, duration: Duration, system: &mut dyn System<In = (), Out = ()>) {
+            // Simulate time passing by updating the simulation time resource
+            {
+                let mut time = self.world.resource_mut::<Time>();
+                let last_update = time.last_update().unwrap();
+                time.update_with_instant(last_update + duration);
             }
+
+            // Reset world-related change detection
+            self.world.clear_trackers();
+            assert!(!self.transform().is_changed());
+
+            // Tick system
+            system.run((), &mut self.world);
+
+            // Update events after system ticked, in case system emitted some events
+            let mut events = self.world.resource_mut::<Events<TweenCompleted>>();
+            events.update();
+        }
+
+        /// Get the animator for the transform.
+        pub fn animator(&self) -> &Animator<Transform> {
+            self.world
+                .entity(self.entity)
+                .get::<Animator<Transform>>()
+                .unwrap()
+        }
+
+        /// Get the transform component.
+        pub fn transform(&mut self) -> Mut<Transform> {
+            self.world.get_mut::<Transform>(self.entity).unwrap()
         }
+
+        /// Get the emitted event count since last tick.
+        pub fn event_count(&self) -> usize {
+            let events = self.world.resource::<Events<TweenCompleted>>();
+            events.get_reader().len(&events)
+        }
+    }
+
+    #[test]
+    fn change_detect_component() {
+        let tween = Tween::new(
+            EaseMethod::Linear,
+            Duration::from_secs(1),
+            TransformPositionLens {
+                start: Vec3::ZERO,
+                end: Vec3::ONE,
+            },
+        )
+        .with_completed_event(0);
+
+        let mut env = TestEnv::new(Animator::new(tween));
+
+        // After being inserted, components are always considered changed
+        let transform = env.transform();
+        assert!(transform.is_changed());
+
+        //fn nit() {}
+        //let mut system = IntoSystem::into_system(nit);
+        let mut system = IntoSystem::into_system(component_animator_system::<Transform>);
+        system.initialize(env.world_mut());
+
+        env.tick(Duration::ZERO, &mut system);
+
+        let animator = env.animator();
+        assert_eq!(animator.state, AnimatorState::Playing);
+        assert_eq!(animator.tweenable().times_completed(), 0);
+        let transform = env.transform();
+        assert!(transform.is_changed());
+        assert!(transform.translation.abs_diff_eq(Vec3::ZERO, 1e-5));
+
+        env.tick(Duration::from_millis(500), &mut system);
+
+        assert_eq!(env.event_count(), 0);
+        let animator = env.animator();
+        assert_eq!(animator.state, AnimatorState::Playing);
+        assert_eq!(animator.tweenable().times_completed(), 0);
+        let transform = env.transform();
+        assert!(transform.is_changed());
+        assert!(transform.translation.abs_diff_eq(Vec3::splat(0.5), 1e-5));
+
+        env.tick(Duration::from_millis(500), &mut system);
+
+        assert_eq!(env.event_count(), 1);
+        let animator = env.animator();
+        assert_eq!(animator.state, AnimatorState::Playing);
+        assert_eq!(animator.tweenable().times_completed(), 1);
+        let transform = env.transform();
+        assert!(transform.is_changed());
+        assert!(transform.translation.abs_diff_eq(Vec3::ONE, 1e-5));
+
+        env.tick(Duration::from_millis(100), &mut system);
+
+        assert_eq!(env.event_count(), 0);
+        let animator = env.animator();
+        assert_eq!(animator.state, AnimatorState::Playing);
+        assert_eq!(animator.tweenable().times_completed(), 1);
+        let transform = env.transform();
+        assert!(!transform.is_changed());
+        assert!(transform.translation.abs_diff_eq(Vec3::ONE, 1e-5));
     }
 }