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).
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.
This does not compile for me on linux, because of the implicit conversion from int to float. Just use (float)i. Same below.
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.
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.
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.
Don't use single-letter variables.
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.
"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?
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.
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.
|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.
If this is a percentage, replace PROP_NONE with PROP_PERCENTAGE so that it actually shows as a percentage in the UI.
"should have" is inappropriately hedging. "Percentage of the energy that is passed from each bounce to the next" would be better.
Yes, this is the percentage of time. I'll add a new variable.
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.
I'm refactoring to avoid the iterative loop.
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.
Thanks for telling me about that. I didn't realize the use of PROP_PERCENTAGE.
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!).