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

Make cubic_interpolate() consider key time in animation #63602

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jul 29, 2022

Fixes #60154.

In the previous implementation, cubic_interpolate() did not consider time, resulting in clunky transitions when the key interval was not constant. The following images show animations with the same keyframes, but the left one is the previous cubic_interpolate() without time(velocity) consideration and the right one is this PR with time(velocity) consideration.

Desktop 2022 07 29 - 06 46 45_1

By providing time as an argument, it allows for smoother movements. Also, based on previous implementations, the default values of the argument are -1.0, 0.0, 1.0, 2.0. The result of this argument will be the same as the previous implementation, but if there are performance concerns in Curve or Gradient classes, it may want to separate the functions.

1_5_1.mp4
1_6_1.mp4

If anyone don't want overshoot, simply adding more plots will solve the problem, but if the demand is too much, we can consider implementing makima interpolate (#57952) or adding a function to AnimationTrack that switches between the previous implementation and the new one.

@TokageItLab TokageItLab added this to the 4.0 milestone Jul 29, 2022
@TokageItLab TokageItLab requested a review from a team July 29, 2022 00:47
@TokageItLab TokageItLab requested review from a team as code owners July 29, 2022 00:47
@TokageItLab TokageItLab requested a review from fire July 29, 2022 01:01
@TokageItLab TokageItLab force-pushed the cubic-interp-time branch 4 times, most recently from b788a81 to a0a6cf6 Compare July 29, 2022 08:40
@TokageItLab TokageItLab marked this pull request as draft July 29, 2022 09:41
@TokageItLab TokageItLab force-pushed the cubic-interp-time branch 2 times, most recently from 272e897 to 81b79b3 Compare July 29, 2022 09:50
@TokageItLab TokageItLab marked this pull request as ready for review July 29, 2022 10:23
@TokageItLab TokageItLab force-pushed the cubic-interp-time branch 3 times, most recently from 0765919 to 9bccc0e Compare July 29, 2022 19:13
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

The previous problems with non default arguments have been resolved. Approved.

We are not solving overshooting here.

@reduz
Copy link
Member

reduz commented Aug 18, 2022

I would not replace the existing function to be honest, since you don't always need to use it in time and this is more costly and complex.
I would add a new function cubic_interpolate_in_time() instead, which IMO makes more sense.

@TokageItLab
Copy link
Member Author

Ok, I'll modify this PR later.

@TokageItLab TokageItLab marked this pull request as draft August 18, 2022 09:25
@TokageItLab TokageItLab force-pushed the cubic-interp-time branch 2 times, most recently from 74ebbfd to 4d3e129 Compare August 19, 2022 03:48
@TokageItLab TokageItLab marked this pull request as ready for review August 19, 2022 03:48
@TokageItLab TokageItLab requested a review from a team as a code owner August 19, 2022 03:48
@TokageItLab
Copy link
Member Author

image

@reduz Changed this PR implementation to add cubic_interpolate_in_time() as a new interpolation type.

@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cubic_interpolate() does not consider the distance between keyframes
5 participants