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

AnimatedProperty and AnimatedPropertyOptional #16484

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cart
Copy link
Member

@cart cart commented Nov 23, 2024

Objective

Animating component properties requires too much boilerplate at the moment:

#[derive(Reflect)]
struct FontSizeProperty;

impl AnimatableProperty for FontSizeProperty {
    type Component = TextFont;

    type Property = f32;

    fn get_mut(component: &mut Self::Component) -> Option<&mut Self::Property> {
        Some(&mut component.font_size)
    }
}

animation_clip.add_curve_to_target(
    animation_target_id,
    AnimatableKeyframeCurve::new(
        [0.0, 0.5, 1.0, 1.5, 2.0, 2.5, 3.0]
            .into_iter()
            .zip([24.0, 80.0, 24.0, 80.0, 24.0, 80.0, 24.0]),
    )
    .map(AnimatableCurve::<FontSizeProperty, _>::from_curve)
    .expect("should be able to build translation curve because we pass in valid samples"),
);

Solution

This adds AnimatedProperty and AnimatedPropertyOptional, enabling the following:

animation_clip.add_curve_to_target(
    animation_target_id,
    AnimatableCurve::new(
        AnimatedProperty::new(|font: &mut TextFont| &mut font.font_size),
        AnimatableKeyframeCurve::new(
            [0.0, 0.5, 1.0, 1.5, 2.0, 2.5, 3.0]
                .into_iter()
                .zip([24.0, 80.0, 24.0, 80.0, 24.0, 80.0, 24.0]),
        )
        .expect(
            "should be able to build translation curve because we pass in valid samples",
        ),
    ),
);

This required reworking the internals a bit, namely stripping out a lot of the Reflect usage, as that implementation was fundamentally incompatible with the AnimatedProperty pattern. Reflect was being used in this context just to downcast traits. But we can get downcasting behavior without the Reflect requirement by implementing Downcast for AnimationCurveEvaluator.

@cart cart added the A-Animation Make things move and change over time label Nov 23, 2024
@cart cart added this to the 0.15 milestone Nov 23, 2024
@cart
Copy link
Member Author

cart commented Nov 23, 2024

I'd like to land this in 0.15 as it:

  1. Polishes up a feature being released in 0.15
  2. Is reasonably straightforward / risk-free

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 23, 2024
@@ -205,8 +274,7 @@ where
P: AnimatableProperty,
{
evaluator: BasicAnimationCurveEvaluator<P::Property>,
#[reflect(ignore)]
phantom: PhantomData<P>,
property: P,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically my only concern with this PR.

Since evaluators are cached and dispatched based on type IDs, the previous setup with marker structs guaranteed that all AnimatableCurves accessing the same property would map to evaluators with the same type, since they used the same marker type as input.

Now, since AnimatedProperty has a function-type generic F: Fn(&mut C) -> &mut P, I think there is a subtle footgun involving the instantiation of evaluators: for example, if you create two AnimatableCurves using AnimatablePropertys that are the same, but use closures to specify the property accessor, the two will map to distinct evaluator types, since the types (P here) will actually be distinct. As a result, the outputs of those curves will not blend correctly (one will just overwrite the other, I think).

In other words, the previous implementation effectively guaranteed that the property accessors would be type-identical, and that is no longer the case now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It might be enough to just warn about this and recommend defining the property in a large lexical scope and then reusing it in calls to AnimatableCurve::new; if anything, I'm mostly pointing out that the pattern described in the 'Solution' section of the PR description is subtly an anti-pattern.)

@viridia
Copy link
Contributor

viridia commented Nov 23, 2024

There are a few things that I would do differently here.

First, I would define a getter and setter rather than returning a mutable field. This would allow the property being animated to be a simplified abstraction of the underlying property. So for example, I could animate rotation_x as an angle instead of having to deal with a quat - the conversion from angle to quat could be done in the AnimatableProperty impl. Similarly, I could animate scale as a scalar affecting all dimensions instead of separate scale_x, scale_y, scale_z.

See also the discussion in #15725 - there's a difference between "keyframe" curves and "transition" curves (my terms, perhaps there's a better way of describing this), in that the animation target can change in mid-animation (like a button which inflates when hovered, but the mouse quickly enters and leaves, giving no time for the animation to complete). In thee cases, you want to avoid an ugly jump from resetting the animation back to start, and instead start the animation from whatever the current state is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants