AnimatedField and Rework Evaluators

[cart]

Objective

Animating component fields 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 AnimatedField and an animated_field! macro, enabling the following:

animation_clip.add_curve_to_target(
    animation_target_id,
    AnimatableCurve::new(
        animated_field!(TextFont::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 AnimatedField 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.

This also reworks "evaluator identity" to support either a (Component / Field) pair, or a TypeId. This allows properties to reuse evaluators, even if they have different accessor methods. The "contract" here is that for a given (Component / Field) pair, the accessor will return the same value. Fields are identified by their Reflect-ed field index. The (TypeId, usize) is prehashed and cached to optimize for lookup speed.

This removes the built-in hard-coded TranslationCurve / RotationCurve / ScaleCurve in favor of AnimatableField.

[cart]

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

[mweatherley]

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.

[mweatherley]

(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.)

[cart]

Hmm yeah nice catch. Current behavior doesn't feel shippable, but I think it points to a more fundamental problem. Theres an odd coupling right now between evaluators and "property accessor code".

Seems like we should decouple evaluator identity from a statically defined AnimationProperty type, and instead tie it to the id of the component combined with the id of the property/field being accessed. I think we'll want to do that anyway to support things like ReflectedAnimatableProperty::<Vec3>::new(TypeId::of::<Transform>(), "translation").

[cart]

I'll start playing with that.

[cart]

This would probably also allow making arbitrary transform field animation (via things like AnimatableProperty) compatible with a custom TransformCurve impl. Rather than using the #15665 approach, which relies on everything using the same custom impls.

[viridia]

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.

[cart]

@viridia

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.

I like this concept / I agree that it would increase the flexibility here. But its also a larger rework of the current approach, whereas this PR is intended to be a surgical change to improve the usability of the current approach. Not something I'd want to change in this PR for 0.15

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.

Ditto for this.

[cart]

Just pushed some changes:

1. Reworked evaluator identity. Evaluators can now either be a (Component / Field) pair, or a TypeId. This allows properties to reuse evaluators, even if they have different accessor methods. The "contract" here is that for a given (Component / Field) pair, the accessor will return the same value. Fields are identified by their Reflect-ed field index. The (TypeId, usize) is prehashed and cached to optimize for lookup speed.
2. Reworked AnimatableProperty to make it box-able / type-eraseable.
3. Renamed AnimatedProperty to AnimatedField to better encapsulate the meaning of the type
4. Added animated_field!(Transform::translation)` macro. This removes the need to manually define the Fn to access the field, allows us to make future get/set internals non-breaking in these cases, and allows us to abstract over field-name-accesses (which require string lookups / unwraps). We still get compile-time protection here because of the generated field-access Fn, which will produce compiler errors if the field does not exist.
5. Removes built-in hard-code TranslationCurve / RotationCurve / ScaleCurve in favor of AnimatableField.

[github-actions]

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

[alice-i-cecile]

I like the simplification here, and I think this is a reasonable path forward. DreamerTalin's points above are interesting and reasonable, but definitely out of scope for now!

[mbrea-c]

I'm also in favor of the new API. My only concern is that by losing Reflect on evaluators we make sampling animation curves outside of the built-in bevy_animation flow will be even harder, from what I can see having to resort to std::mem::transmute-ing into copied types or maybe trying to write custom evaluators to override the existing (I'll dig into this more tomorrow to see if I missed something). I documented my own use-case for that in #16395, which I can update to bring in line with this new API; I'd feel much more comfortable about this if we also bring in some changes to open up the sampling of animation curves, be it my linked PR or something else entirely.

That said, there's not many applications for "sampling outside of bevy_animation" flow other than third-party animation crates, and it's not really something that was supported before 0.15 either, so it's completely understandable if we don't block on that (just mentioning here to bring some visibility to the issue).

[mweatherley]

I'm also in favor of the new API. My only concern is that by losing Reflect on evaluators we make sampling animation curves outside of the built-in bevy_animation flow will be even harder [...]

I am extremely sympathetic to this, but I also think that improving the story for access to animation curves requires enough design that it's unlikely to make 0.15; I'm also skeptical, I think, that Reflect is necessarily the right thing for working dynamically with these long-term in this case.

(From my perspective, the intent with how things work right now was that the level things would be "shareable" was with Curve<T> rather than AnimationCurve per se.)

[mbrea-c]

I am extremely sympathetic to this, but I also think that improving the story for access to animation curves requires enough design that it's unlikely to make 0.15;

That's fair, it's probably better to leave it for 0.16

I'm also skeptical, I think, that Reflect is necessarily the right thing for working dynamically with these long-term in this case.

Yeah, I completely agree. I never considered it a long term solution, just an unintended "escape hatch" that could be used as a workaround for the missing animation curve sampling/access APIs

(From my perspective, the intent with how things work right now was that the level things would be "shareable" was with Curve<T> rather than AnimationCurve per se.)

Makes sense, the more I think about it the more I agree that it'd be the most "natural" API. Maybe we can continue discussion in #15664 after 0.15?

[cart]

I'm also in favor of the new API. My only concern is that by losing Reflect on evaluators we make sampling animation curves outside of the built-in bevy_animation flow will be even harder, from what I can see having to resort to std::mem::transmute-ing into copied types or maybe trying to write custom evaluators to override the existing (I'll dig into this more tomorrow to see if I missed something). I documented my own use-case for that in #16395, which I can update to bring in line with this new API; I'd feel much more comfortable about this if we also bring in some changes to open up the sampling of animation curves, be it my linked PR or something else entirely.

Yeah I also think this is a worthwhile scenario to cover and agree that Reflect was the wrong way to cover it. We should build actual APIs that enable the scenarios we want.

[cart]

For 0.15, I think we should move forward with this and then sort out how to cover these scenarios in 0.16

[cart]

(docs need a bit of attention ... working on that now)