Page MenuHome

Add variability to "Bounce" interpolation
Needs ReviewPublic

Authored by Daniel Stewart (mpc823) on Mar 31 2020, 6:00 PM.

Details

Summary

This patch adds user definable parameters to the Bounce keyframe & Grease Pencil interpolation. It adds both the number of bounces and the relative height of each bounce ("bounciness" in [0.0 - 1.0] range) to be set for a Bounce keyframe interpolation. The general strategy is the same as the previous implementation, which had hard-coded values which resulted in 3 bounces with a "bounciness" of 0.5. This patch adds two variables to the interpolation interface for Bounce interpolation - "Bounces" (number of bounces) and "Bounciness" (the percentage height each bounce should be from the previous bounce).

Diff Detail

Repository
rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 21 2020, 3:57 PM

Please use clang-format before creating patches: https://wiki.blender.org/wiki/Tools/ClangFormat

There seems to be an off-by-one error with the number of bounces.

I'd expect this to have zero bounces


and this to have one bounce.

source/blender/blenlib/intern/easing.c
131

This does not compile for me on linux, because of the implicit conversion from int to float. Just use (float)i. Same below.

This revision now requires changes to proceed.Apr 21 2020, 3:57 PM

Changed the number of bounces to start at 0 instead of 1. Added a cast to allow compilation on Linux. And ran clang-format via Visual Studio.

Sergey Sharybin (sergey) requested changes to this revision.Apr 22 2020, 10:29 AM

I think there is missing code in the do_versions to keep existing files which use this feature working. Because of this marking as Requested Changes.

Personally, I feel weird about having bounce as an interpolation. Is not something I am aware of animators actually use and, if anything, it should have been done as a modifier. From all attempts to use it for anything it always behaves weirdly.

@Sybren A. Stüvel (sybren) , do you mind making a call here? Think you have more bigger picture overview.

This revision now requires changes to proceed.Apr 22 2020, 10:29 AM

Also not keen on this.

I tried using this patch and found the settings difficult to control usefully.

I'd rather have this kind of feature in f-curve modifiers.

I'll look at the do_versions, as I wasn't aware of that.

As far as usefulness, I'm surprised at that. At it's defaults, it behaves the same as it otherwise would. But given the requests for this feature when the bounciness interpolation was first added, I thought making the bounce interpolation as adjustable as the other Robert Penner interpolations would be advantageous to animators looking for quick background/secondary motion. (Which I've used myself.) It seemed that perhaps the only reason it wasn't adjustable was because the original instantiation was hard-coded and no one took the time to make it adjustable. I personally find having the adjustments available make the bounce interpolation more useful.

Added do_version support.

But I've realized I have no idea where the Grease Pencil Interpolation code is. I've searched the codebase numerous times and can't find where it actually makes the UI for the GP interpolation strokes. So although the core code appears to be there for adding parameters to bounciness, I don't know where in the code to make it actually show up in the UI. Any pointers would be greatly appreciated.

Daniel Stewart (mpc823) edited the summary of this revision. (Show Details)

Added the "num_bounces" and "bounciness" to the UI for Grease Pencil Interpolation.

I am unable to apply this patch to master on Windows.

Sybren A. Stüvel (sybren) requested changes to this revision.Jun 18 2020, 11:37 AM

I think this is a nice feature to have.

The interpolation type can be set per key, so I don't see how this functionality could be moved to an FCurve modifier. Unless the entire FCurve would get the same bouncyness, or unless the FCurve can keep track of parameters per key, that is, and I'm not a fan of either.

There are already properties to influence the elasticity of an elastic interpolation, so I don't see why this would be objectionable for bounce interpolation.

source/blender/blenlib/intern/easing.c
106

Don't use single-letter variables.

122–123

This changes time from actual time (which I assume is in frames) to a percentage of time. This should not be done with the same variable.

124–125

"width", "increment", and "interval" all mean the same thing: a space between two numbers. Please choose names that reflect their meaning better.

What is the difference between "interval" and "total_interval"? Why is the width expressed in squared units? Or is the rest also the result of squaring, but because 1.0² = 1.0 this is invisible?

129

It's rather unclear what this expression means, and why the loop can stop here. Also, what is here inside the if body is exactly what is executed after the for loop, so instead of repeating the code you could just use a break statement here.

131

This expression is quite complex now, and also has some unneeded parentheses. At least remove the ones around (width * time), as it actually hides the fact that time is squared here. I know, this was also used in the previous code, and no I'm not happy about that either.

It's also copied below, which is also not a good idea. If code is to be reused, place it in a function and call that from both places.

source/blender/blenloader/intern/versioning_280.c
753–754 ↗(On Diff #24501)

No need to declare everything at the top.

754 ↗(On Diff #24501)

Don't use single-letter variables.

1678 ↗(On Diff #24501)

2.90 should not be handled before 2.83, and probably also not in versioning_280.c.

There is an (almost) empty code block at the end of the function marked "Versioning code until next subversion bump goes here." Just put the versioning code there, and when a new version bump happens someone will take care of putting it in the right place.

source/blender/makesrna/intern/rna_fcurve.c
2080

If this is a percentage, replace PROP_NONE with PROP_PERCENTAGE so that it actually shows as a percentage in the UI.

2083

"should have" is inappropriately hedging. "Percentage of the energy that is passed from each bounce to the next" would be better.

This revision now requires changes to proceed.Jun 18 2020, 11:37 AM
source/blender/blenlib/intern/easing.c
122–123

Yes, this is the percentage of time. I'll add a new variable.

124–125

The width is essentially the coefficient (m) in the parabolic expression of y = m * x^2 + c. It so happens that the coefficient m can be expressed in this manner. I'll try to simplify this calculation and add some comments explaining the calculation. Essentially, the code is simply finding which parabola to calculate and then calculating the value at the current time position.

129

I'm refactoring to avoid the iterative loop.

131

I'm refactoring this code to not perform the calculations iteratively. This should simplify the code a bit and remove the extra calculation outside of the loop. The downside is the calculations appear a bit complicated, using log() and pow() functions to determine which "bounce" (parabola) should be calculated.

source/blender/makesrna/intern/rna_fcurve.c
2080

Thanks for telling me about that. I didn't realize the use of PROP_PERCENTAGE.

Removed iterative approach to calculating the bounces. Moved the versioning support to the versioning_290.c file.

Daniel Stewart (mpc823) marked 7 inline comments as done.

This addresses the comments made concerning the previous patch. I refactored the code such that I no longer use an iterative process to determine which bounce we are calculating nor how high the bounce should be. Moved the versioning code to versioning_290.c (hopefully in the right spot!).