Skip to content

Fix last keyframe #10842

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.

Sign up for GitHub

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

Closed
A-Walrus wants to merge 1 commit into from
Closed

Fix last keyframe #10842

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 46 additions & 25 deletions crates/bevy_animation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,39 @@ fn apply_animation(
parents: &Query<(Has<AnimationPlayer>, Option<&Parent>)>,
children: &Query<&Children>,
) {
/// Set transform to exactly the value of some keyframe.
fn set_keyframe(
curve: &VariableCurve,
transform: &mut Mut<'_, Transform>,
weight: f32,
morphs: &mut Result<Mut<'_, MorphWeights>, bevy_ecs::query::QueryEntityError>,
keyframe: usize,
) {
match &curve.keyframes {
Keyframes::Rotation(keyframes) => {
transform.rotation = transform.rotation.slerp(keyframes[keyframe], weight);
}
Keyframes::Translation(keyframes) => {
transform.translation = transform.translation.lerp(keyframes[keyframe], weight);
}
Keyframes::Scale(keyframes) => {
transform.scale = transform.scale.lerp(keyframes[keyframe], weight);
}
Keyframes::Weights(keyframes) => {
if let Ok(morphs) = morphs {
let target_count = morphs.weights().len();
lerp_morph_weights(
morphs.weights_mut(),
get_keyframe(target_count, keyframes, keyframe)
.iter()
.copied(),
weight,
);
}
}
}
}

if let Some(animation_clip) = animations.get(&animation.animation_clip) {
// We don't return early because seek_to() may have been called on the animation player.
animation.update(
Expand Down Expand Up @@ -648,31 +681,13 @@ fn apply_animation(
};
// SAFETY: As above, there can't be other AnimationPlayers with this target so this fetch can't alias
let mut morphs = unsafe { morphs.get_unchecked(target) };

for curve in curves {
let len = curve.keyframe_timestamps.len();

// Some curves have only one keyframe used to set a transform
if curve.keyframe_timestamps.len() == 1 {
match &curve.keyframes {
Keyframes::Rotation(keyframes) => {
transform.rotation = transform.rotation.slerp(keyframes[0], weight);
}
Keyframes::Translation(keyframes) => {
transform.translation =
transform.translation.lerp(keyframes[0], weight);
}
Keyframes::Scale(keyframes) => {
transform.scale = transform.scale.lerp(keyframes[0], weight);
}
Keyframes::Weights(keyframes) => {
if let Ok(morphs) = &mut morphs {
let target_count = morphs.weights().len();
lerp_morph_weights(
morphs.weights_mut(),
get_keyframe(target_count, keyframes, 0).iter().copied(),
weight,
);
}
}
}
if len == 1 {
set_keyframe(curve, &mut transform, weight, &mut morphs, 0);
continue;
}

Expand All @@ -682,10 +697,16 @@ fn apply_animation(
.keyframe_timestamps
.binary_search_by(|probe| probe.partial_cmp(&animation.seek_time).unwrap())
{
Ok(n) if n >= curve.keyframe_timestamps.len() - 1 => continue, // this curve is finished
Ok(n) if n >= curve.keyframe_timestamps.len() - 1 => {
set_keyframe(curve, &mut transform, weight, &mut morphs, len - 1);
continue;
} // this curve is finished
Ok(i) => i,
Err(0) => continue, // this curve isn't started yet
Err(n) if n > curve.keyframe_timestamps.len() - 1 => continue, // this curve is finished
Err(n) if n > curve.keyframe_timestamps.len() - 1 => {
Copy link
Contributor

@VVishion VVishion Dec 2, 2023

Choose a reason for hiding this comment

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

Since Err(n) is returned when no exact match was found this means that we sample after the last keyframe for n > len - 1. This curve should not contribute to the final pose by default in my opinion. The way most are familiar with this is probably an option "hold". I would continue here and special case it later when this option is implemented.
The rest looks fine

Copy link
Contributor

@VVishion VVishion Dec 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I should add curves not contributing anymore is essentially only relevant when blending

Copy link
Contributor

@VVishion VVishion Dec 2, 2023

Choose a reason for hiding this comment

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

I gave it another thought and my suggestion doesnt result in the last keyframe being applied when we sample with t > curve duration. Im going to give it some more thought for something bullet-proof.

My current stance is that the last keyframe should be sampled when t(i - 1) < curve duration and t(i) > curve duration, but not when t(i - 1) > curve duration and t(i) > curve duration (unless we are holding the animation).
This means we need to keep track of when an animation was last sampled and whether it was sampled the last time the player ran.

Where t(i - 1) is the last time we sampled and t(i) is the time we currently sample.

Copy link
Contributor

@VVishion VVishion Dec 2, 2023

Choose a reason for hiding this comment

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

See #10832 (comment)

set_keyframe(curve, &mut transform, weight, &mut morphs, len - 1);
continue;
} // this curve is finished
Err(i) => i - 1,
};
let ts_start = curve.keyframe_timestamps[step_start];
Expand Down