Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translation values still being changed after Animation played "Once" finished #33

Closed
jakyle opened this issue Jul 17, 2022 · 4 comments · Fixed by #71
Closed

Translation values still being changed after Animation played "Once" finished #33

jakyle opened this issue Jul 17, 2022 · 4 comments · Fixed by #71
Labels
bevy-blocked Issue blocked by the need for a change/fix in Bevy itself bug Something isn't working
Milestone

Comments

@jakyle
Copy link

jakyle commented Jul 17, 2022

Whenever I use the TransformPositionLens with TweeningType::Once, even after the animation is completed, the entitities transform seems to be "changed" still. I know this because I have another system listening on transform Change and it is triggering every frame.

#[derive(Component)]
pub struct ImTweeningHere;

pub fn move_player_on_click(
    mut commands: Commands,
    mouse_input: Res<Input<MouseButton>>,
    grid_dimensions: Res<GridDimensions>,
    mouse_map_pos: Option<Res<MouseMapPos>>,
    player: Query<(Entity, &Transform), (With<Player>, Without<ImTweeningHere>)>,
) {
    if let (true, Some(mouse_map_pos)) = (
        mouse_input.just_pressed(MouseButton::Left),
        mouse_map_pos.as_deref(),
    ) {
        if let Ok((entity, player_transform)) = player.get_single() {
            let end_position = grid_dimensions.grid_loc_to_vec2(
                (mouse_map_pos.0.x / grid_dimensions.tile_area).floor(),
                (mouse_map_pos.0.y / grid_dimensions.tile_area).floor(),
            );

            let tween = Tween::new(
                EaseFunction::CircularInOut,
                TweeningType::Once,
                std::time::Duration::from_millis(500),
                TransformPositionLens {
                    start: player_transform.translation,
                    end: Transform::from_translation(end_position.extend(player_transform.translation.z))
                        .translation,
                },
            )
            .with_completed_event(true, 0);

            let animator = Animator::new(tween);
            commands
                .entity(entity)
                .insert(animator)
                .insert(ImTweeningHere);
        }
    }
}

the above system triggers the animation on map click, when the animation completes, the entities Transform still fires off

fn player_transforming(
    player: Query<&Transform, (With<Player>, Changed<Transform>)>,
) {
    if let Ok(transforming) = player.get_single() {
        info!("player is transforming!: {transforming:?}");
    }
}

as a work around, I manually remove the AnimatorTransform on tween complete

fn movement_completed(mut commands: Commands, mut on_tween_complete: EventReader<TweenCompleted>) {
    for evt in on_tween_complete.iter() {
        if evt.user_data == 0 {
            commands
                .entity(evt.entity)
                .remove::<ImTweeningHere>()
                .remove::<Animator<Transform>>();
        }
    }
}

However, I am not sure if this is the intended behavior. do we need to remove the Animator on animation complete. I expect that the intended behavior when using a TweeningType of Once is the component will be removed on animation complete for the entity and we use the TweenCompleted event for other stuff, like my use case here to remove my own component.

@djeedai
Copy link
Owner

djeedai commented Jul 22, 2022

The intended design is never to auto-remove components; it's too hard to guess how the user wants to use the component. The auto-remove can be implemented by the user.

For the Changed issue, tick() takes &mut T instead of Mut<T> which is why Changed triggers all the time. This is a bug.

@djeedai
Copy link
Owner

djeedai commented Jul 25, 2022

I tried to fix, this needs a Bevy change to be fixed cleanly. The change went in already (bevyengine/bevy#5438) so this can be fixed in next version when we upgrade to Bevy 0.8 (which is imminent).

@jakyle
Copy link
Author

jakyle commented Jul 25, 2022

great, thanks for the reply :) and thanks for looking into this

@djeedai djeedai added the bug Something isn't working label Aug 3, 2022
@djeedai djeedai added this to the Bevy 0.8 milestone Aug 3, 2022
@djeedai djeedai pinned this issue Aug 3, 2022
@djeedai djeedai added the bevy-blocked Issue blocked by the need for a change/fix in Bevy itself label Sep 21, 2022
djeedai added a commit that referenced this issue Nov 6, 2022
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.

TODO : CHANGELOG + CHANGE DETECTION TESTS

Fixes #33
djeedai added a commit that referenced this issue Nov 8, 2022
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
djeedai added a commit that referenced this issue Nov 9, 2022
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
@djeedai
Copy link
Owner

djeedai commented Nov 9, 2022

@jakyle sorry for the delay, that took a while. Bevy 0.8 did fix what I needed, but also introduced other blockers for this issue, so I had to get help from the ECS team to figure out how to fix and test this. Change detection should work as expected now, and I've added a test for components, but feel free to reopen if there's still an issue.

@djeedai djeedai unpinned this issue Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bevy-blocked Issue blocked by the need for a change/fix in Bevy itself bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants