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
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/bevy_animation/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ petgraph = { version = "0.6", features = ["serde-1"] }
ron = "0.8"
serde = "1"
blake3 = { version = "1.0" }
downcast-rs = "1.2.0"
derive_more = { version = "1", default-features = false, features = [
"error",
"from",
Expand Down
124 changes: 97 additions & 27 deletions crates/bevy_animation/src/animation_curves.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ use bevy_math::{
},
Quat, Vec3,
};
use bevy_reflect::{FromReflect, Reflect, Reflectable, TypePath};
use bevy_reflect::{FromReflect, Reflect, Reflectable};
use bevy_render::mesh::morph::MorphWeights;
use bevy_transform::prelude::Transform;

Expand All @@ -100,6 +100,7 @@ use crate::{
prelude::{Animatable, BlendInput},
AnimationEntityMut, AnimationEvaluationError,
};
use downcast_rs::{impl_downcast, Downcast};

/// A value on a component that Bevy can animate.
///
Expand Down Expand Up @@ -160,17 +161,83 @@ use crate::{
/// configured above).
///
/// [`AnimationClip`]: crate::AnimationClip
pub trait AnimatableProperty: Reflect + TypePath {
pub trait AnimatableProperty {
/// The type of the component that the property lives on.
type Component: Component;

/// The type of the property to be animated.
type Property: Animatable + FromReflect + Reflectable + Clone + Sync + Debug;
type Property: Animatable + Clone + Sync + Debug;

/// Given a reference to the component, returns a reference to the property.
///
/// If the property couldn't be found, returns `None`.
fn get_mut(component: &mut Self::Component) -> Option<&mut Self::Property>;
fn get_mut<'a>(&self, component: &'a mut Self::Component) -> Option<&'a mut Self::Property>;
}

/// A [`Component`] property that can be animated, defined by a function that reads the component and returns
/// the accessed field / property.
#[derive(Clone)]
pub struct AnimatedProperty<C, P, F: Fn(&mut C) -> &mut P> {
func: F,
marker: PhantomData<(C, P)>,
}

impl<C, P, F> AnimatableProperty for AnimatedProperty<C, P, F>
where
C: Component,
P: Animatable + Clone + Sync + Debug,
F: Fn(&mut C) -> &mut P,
{
type Component = C;

type Property = P;

fn get_mut<'a>(&self, component: &'a mut Self::Component) -> Option<&'a mut Self::Property> {
Some((self.func)(component))
}
}

impl<C, P, F: Fn(&mut C) -> &mut P + 'static> AnimatedProperty<C, P, F> {
/// Creates a new instance of [`AnimatedProperty`]
pub fn new(func: F) -> Self {
Self {
func,
marker: PhantomData,
}
}
}

/// A [`Component`] property that can be animated, defined by a function that reads the component and returns
/// either the accessed field / property, or [`None`] if it does not exist
#[derive(Clone)]
pub struct AnimatedPropertyOptional<C, P, F: Fn(&mut C) -> Option<&mut P>> {
func: F,
marker: PhantomData<(C, P)>,
}

impl<C, P, F> AnimatableProperty for AnimatedPropertyOptional<C, P, F>
where
C: Component,
P: Animatable + Clone + Sync + Debug,
F: Fn(&mut C) -> Option<&mut P>,
{
type Component = C;

type Property = P;

fn get_mut<'a>(&self, component: &'a mut Self::Component) -> Option<&'a mut Self::Property> {
(self.func)(component)
}
}

impl<C, P, F: Fn(&mut C) -> Option<&mut P> + 'static> AnimatedPropertyOptional<C, P, F> {
/// Creates a new instance of [`AnimatedPropertyOptional`]
pub fn new(func: F) -> Self {
Self {
func,
marker: PhantomData,
}
}
}

/// This trait collects the additional requirements on top of [`Curve<T>`] needed for a
Expand All @@ -187,12 +254,14 @@ impl<T, C> AnimationCompatibleCurve<T> for C where C: Curve<T> + Debug + Clone +
#[derive(Reflect, FromReflect)]
#[reflect(from_reflect = false)]
pub struct AnimatableCurve<P, C> {
/// The property selector, which defines what component to access and how to access
/// a property on that component.
pub property: P,

/// The inner [curve] whose values are used to animate the property.
///
/// [curve]: Curve
pub curve: C,
#[reflect(ignore)]
_phantom: PhantomData<P>,
}

/// An [`AnimatableCurveEvaluator`] for [`AnimatableProperty`] instances.
Expand All @@ -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.)

}

impl<P, C> AnimatableCurve<P, C>
Expand All @@ -218,22 +286,20 @@ where
/// valued in an [animatable property].
///
/// [animatable property]: AnimatableProperty::Property
pub fn from_curve(curve: C) -> Self {
Self {
curve,
_phantom: PhantomData,
}
pub fn new(property: P, curve: C) -> Self {
Self { property, curve }
}
}

impl<P, C> Clone for AnimatableCurve<P, C>
where
C: Clone,
P: Clone,
{
fn clone(&self) -> Self {
Self {
curve: self.curve.clone(),
_phantom: PhantomData,
property: self.property.clone(),
}
}
}
Expand All @@ -249,10 +315,10 @@ where
}
}

impl<P, C> AnimationCurve for AnimatableCurve<P, C>
impl<P: Send + Sync + 'static, C> AnimationCurve for AnimatableCurve<P, C>
where
P: AnimatableProperty,
C: AnimationCompatibleCurve<P::Property>,
P: AnimatableProperty + Clone,
C: AnimationCompatibleCurve<P::Property> + Clone,
{
fn clone_value(&self) -> Box<dyn AnimationCurve> {
Box::new(self.clone())
Expand All @@ -269,7 +335,7 @@ where
fn create_evaluator(&self) -> Box<dyn AnimationCurveEvaluator> {
Box::new(AnimatableCurveEvaluator {
evaluator: BasicAnimationCurveEvaluator::default(),
phantom: PhantomData::<P>,
property: self.property.clone(),
})
}

Expand All @@ -280,7 +346,7 @@ where
weight: f32,
graph_node: AnimationNodeIndex,
) -> Result<(), AnimationEvaluationError> {
let curve_evaluator = (*Reflect::as_any_mut(curve_evaluator))
let curve_evaluator = curve_evaluator
.downcast_mut::<AnimatableCurveEvaluator<P>>()
.unwrap();
let value = self.curve.sample_clamped(t);
Expand All @@ -298,7 +364,7 @@ where

impl<P> AnimationCurveEvaluator for AnimatableCurveEvaluator<P>
where
P: AnimatableProperty,
P: AnimatableProperty + Send + Sync + 'static,
{
fn blend(&mut self, graph_node: AnimationNodeIndex) -> Result<(), AnimationEvaluationError> {
self.evaluator.combine(graph_node, /*additive=*/ false)
Expand All @@ -324,7 +390,9 @@ where
let mut component = entity.get_mut::<P::Component>().ok_or_else(|| {
AnimationEvaluationError::ComponentNotPresent(TypeId::of::<P::Component>())
})?;
let property = P::get_mut(&mut component)
let property = self
.property
.get_mut(&mut component)
.ok_or_else(|| AnimationEvaluationError::PropertyNotPresent(TypeId::of::<P>()))?;
*property = self
.evaluator
Expand Down Expand Up @@ -382,7 +450,7 @@ where
weight: f32,
graph_node: AnimationNodeIndex,
) -> Result<(), AnimationEvaluationError> {
let curve_evaluator = (*Reflect::as_any_mut(curve_evaluator))
let curve_evaluator = curve_evaluator
.downcast_mut::<TranslationCurveEvaluator>()
.unwrap();
let value = self.0.sample_clamped(t);
Expand Down Expand Up @@ -479,7 +547,7 @@ where
weight: f32,
graph_node: AnimationNodeIndex,
) -> Result<(), AnimationEvaluationError> {
let curve_evaluator = (*Reflect::as_any_mut(curve_evaluator))
let curve_evaluator = curve_evaluator
.downcast_mut::<RotationCurveEvaluator>()
.unwrap();
let value = self.0.sample_clamped(t);
Expand Down Expand Up @@ -576,7 +644,7 @@ where
weight: f32,
graph_node: AnimationNodeIndex,
) -> Result<(), AnimationEvaluationError> {
let curve_evaluator = (*Reflect::as_any_mut(curve_evaluator))
let curve_evaluator = curve_evaluator
.downcast_mut::<ScaleCurveEvaluator>()
.unwrap();
let value = self.0.sample_clamped(t);
Expand Down Expand Up @@ -704,7 +772,7 @@ where
weight: f32,
graph_node: AnimationNodeIndex,
) -> Result<(), AnimationEvaluationError> {
let curve_evaluator = (*Reflect::as_any_mut(curve_evaluator))
let curve_evaluator = curve_evaluator
.downcast_mut::<WeightsCurveEvaluator>()
.unwrap();

Expand Down Expand Up @@ -968,7 +1036,7 @@ where
/// mutated in the implementation of [`apply`].
///
/// [`apply`]: AnimationCurve::apply
pub trait AnimationCurve: Reflect + Debug + Send + Sync {
pub trait AnimationCurve: Debug + Send + Sync + 'static {
/// Returns a boxed clone of this value.
fn clone_value(&self) -> Box<dyn AnimationCurve>;

Expand Down Expand Up @@ -1031,7 +1099,7 @@ pub trait AnimationCurve: Reflect + Debug + Send + Sync {
/// translation keyframes. The stack stores intermediate values generated while
/// evaluating the [`crate::graph::AnimationGraph`], while the blend register
/// stores the result of a blend operation.
pub trait AnimationCurveEvaluator: Reflect {
pub trait AnimationCurveEvaluator: Downcast + Send + Sync + 'static {
/// Blends the top element of the stack with the blend register.
///
/// The semantics of this method are as follows:
Expand Down Expand Up @@ -1099,6 +1167,8 @@ pub trait AnimationCurveEvaluator: Reflect {
) -> Result<(), AnimationEvaluationError>;
}

impl_downcast!(AnimationCurveEvaluator);

/// A [curve] defined by keyframes with values in an [animatable] type.
///
/// The keyframes are interpolated using the type's [`Animatable::interpolate`] implementation.
Expand Down
Loading
Loading