Last keyframe of animation curve not sampled

[VVishion]

Bevy version

0.12

Bug

The current implementation for sampling the animation doesnt return the last keyframe of a curve.

bevy/crates/bevy_animation/src/lib.rs

Lines 681 to 685 in b4c33da

	let step_start = match curve
	.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

[tim-blackbird]

Judging by the code below (curve.keyframe_timestamps[step_start + 1]) it might be sampled partially, sometimes, but it could be missed completely if the time delta is higher than the difference between the last two keyframe's timestamps which is pretty bad.

[VVishion]

If .binary_search_by returns Ok(n) the entry was found. Here we continue if n is len() - 1 aka the last entry

[tim-blackbird]

Because the keyframes are sampled two at a time (n and n + 1) the last keyframe would get sampled.

But it does look like the final state after an animation has run would not be exactly equal to the last keyframe, which is pretty bad. (Assuming I've successfully grokked the code here)

[VVishion]

We can just return the last Keyframe if Ok(n) if n == len() - 1. No need for interpolation. Am I understanding you right?

Let t=last keyframe time, then .binary_search_by returns Ok(n) for which the code currently calls continue because n == len() -1 for this t

[tim-blackbird]

We can just return the last Keyframe if Ok(n) if n == len() - 1. No need for interpolation. Am I understanding you right?

Yeah, We should apply the last keyframe instead of continuing.

[VVishion]

My mistake. You were talking about the cases where t0 is before the last keyframe and t1 after and the case where t0 is before the keyframe before the last and t1 is after the last keyframe.

I only considered the case t=clip duration.

Currently, all those cases are not handled properly imo - but the first two cases are not trivial to solve

[VVishion]

I would scope this issue for t=clip duration, because thats an easy fix.

To give an idea why the other cases are worthy a separate discussion:

Lets consider two animations A and B:

A = [(t0, 0), (t1, 1), (t2, 2)]
B = [(t0, 0), (t3, 0)]
where t0 < t1 < t2 < t3

Under the assumption that finished curves are not contributing to the final result aka we are not holding the last pose.
If we blend A and B by 0.5 and sample them at t=t0 and on the next frame we sample at t=t3, then t1 nor t2 or any interpolation of these two is applied. Therefore v=0.

If we implement that if the curve finished and the last keyframe was not sampled exactly that we apply the last keyframe, so that the animated object always has the same pose after the animation finished, then we have the following scenario:

When we sample again at t=t0 and t=t3, then for A we sample at t2 for t=t3, because t2 is the last keyframe, then the blend of A and B would be v=2 * 0.5 + 0 * 0.5.
This differs from when we sample at t=t2 and then at t=t3, where v=0 * 1 (1 because the other curve isn't contributing anymore - see assumption).

There may be other edge cases and I dont know the consensus about holding the last pose.

edit: "v=0 * 1 (1 because the other curve isn't contributing anymore - see assumption)" should probably be "v=0 * 0.5 (the other curve isnt contributing anymore - see assumption)"