blender
/
blender
136
404
Fork 586
Code Issues 6.2k Pull Requests 753 Projects 19 Wiki Activity

Animation: time offset slider #106520

Closed
AresDeveaux wants to merge 7 commits from AresDeveaux/blender:time_offset_slider into main
pull from: AresDeveaux/blender:time_offset_slider
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 6 Commits 7 Files Changed 6 +5 -23
1 changed files with 5 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 688a039711 - Show all commits

28
source/blender/editors/animation/keyframes_general.c
View File

@ -493,32 +493,14 @@ void ease_fcurve_segment(FCurve *fcu, FCurveSegment *segment, const float factor
void time_offset_fcurve_segment(FCurve *fcu, FCurveSegment *segment, const float factor)
Sybren A. Stüvel commented 2023-05-16 10:10:39 +02:00
Review
Copy Link

An offset usually is just, mathematically speaking, an addition of a constant value. So shifting all the keys into a certain direction. A comment below mentions "the wrapping point", which suggests that more is going on than just applying an offset.

This should be reflected in the name,. If that's not possible, at least it should be documented.

An offset usually is just, mathematically speaking, an addition of a constant value. So shifting all the keys into a certain direction. A comment below mentions "the wrapping point", which suggests that more is going on than just applying an offset. This should be reflected in the name,. If that's not possible, at least it should be documented.
{
const BezTriple *left_key = fcurve_segment_start_get(fcu, segment->start_index);
const float left_x = left_key->vec[1][0];
const BezTriple *right_key = fcurve_segment_end_get(fcu, segment->start_index + segment->length);
const float key_x_range = right_key->vec[1][0] - left_x;
/* Happens if there is only 1 key on the FCurve. Needs to be skipped because it
* would be a divide by 0. */
if (IS_EQF(key_x_range, 0.0f)) {
return;
}
/* The factor goes from 0 to 1, but for this tool it needs to go from -1 to 1. */
const float long_factor = factor * 2 - 1;
/* Two bookend keys of the fcurve are needed to be able to cycle the values. */
const BezTriple *last_key = &fcu->bezt[fcu->totvert - 1];
Sybren A. Stüvel commented 2023-05-16 10:07:59 +02:00
Review
Copy Link

The X-coordinate of keys is also a float, as keys can be placed on sub-frames as well (to control things like motion blur).

The X-coordinate of keys is also a `float`, as keys can be placed on sub-frames as well (to control things like motion blur).
const float last_key_x = last_key->vec[1][0];
const float last_key_y = last_key->vec[1][1];
const BezTriple *first_key = &fcu->bezt[0];
const float first_key_x = first_key->vec[1][0];
const float first_key_y = first_key->vec[1][1];
const int fcu_x_range = last_key_x - first_key_x;
const int fcu_x_range = last_key->vec[1][0] - first_key->vec[1][0];
float delta_y;
/* If we operate directly on the fcurve there will be a feedback loop
Christoph Lendenfeld commented 2023-04-20 12:23:14 +02:00
Outdated
Review
Copy Link

you don't need this line

you don't need this line
@ -532,15 +514,15 @@ void time_offset_fcurve_segment(FCurve *fcu, FCurveSegment *segment, const float
/* The values need to go back to the ones at the other end of the fcurve
* every time we get to the last or the first key. */
if (time > last_key_x) {
if (time > last_key->vec[1][0]) {
int offset_frame = fcu->bezt[i].vec[1][0] - fcu_x_range;
time = offset_frame + fcu_x_range * long_factor;
Sybren A. Stüvel commented 2023-05-16 14:06:19 +02:00
Review
Copy Link

(please read on to the end of the comment, I'm asking less of you than apparent at first sight)

This code (adjusting the behaviour of fmod to match what you need) shouldn't be here. The fact that you need to write it means that you need another modulo function that behaves correctly.

Another way of making the same point: 'computing the modulo in an always-positive manner' is not the same job as as 'offsetting an fcurve segment', and thus it belongs in a different function.

I think it would be best to add a function declaration to source/blender/blenlib/BLI_math_base.h, underneath mod_i():

MINLINE float mod_f_positive(float f, float n);

The implementation can then be added to source/blender/blenlib/intern/math_base_inline.c, again underneath mod_i(). Don't forget to make the parameters const in the function implementation (the surrounding code is old, and doesn't follow this new-ish style rule yet). Something like this likely works:

MINLINE float mod_f_positive(const float f, const float n)
{
  const float modulo = fmodf(f, n);
  if (modulo < 0) {
    return modulo + n;
  }
  return modulo;
}

Note that I've also used fmodf() here instead of fmod(). The latter works fine, but converts the float paramters to double, computes the double result, which is then put back into a float again. fmodf() just operates on float directly.

TL;DR: And yeah, then I figured that adding such a function would also require writing a unit test for it, and that's getting too much for this PR. So I just went ahead and implemented that. It's now in the main branch and you can replace the call to fmod() with mod_f_positive().

_(please read on to the end of the comment, I'm asking less of you than apparent at first sight)_ This code (adjusting the behaviour of `fmod` to match what you need) shouldn't be here. The fact that you need to write it means that you need another modulo function that behaves correctly. Another way of making the same point: 'computing the modulo in an always-positive manner' is not the same job as as 'offsetting an fcurve segment', and thus it belongs in a different function. I think it would be best to add a function declaration to `source/blender/blenlib/BLI_math_base.h`, underneath `mod_i()`: ```c MINLINE float mod_f_positive(float f, float n); ``` The implementation can then be added to `source/blender/blenlib/intern/math_base_inline.c`, again underneath `mod_i()`. Don't forget to make the parameters `const` in the function implementation (the surrounding code is old, and doesn't follow this new-ish style rule yet). Something like this likely works: ```c MINLINE float mod_f_positive(const float f, const float n) { const float modulo = fmodf(f, n); if (modulo < 0) { return modulo + n; } return modulo; } ``` Note that I've also used `fmodf()` here instead of `fmod()`. The latter works fine, but converts the `float` paramters to `double`, computes the `double` result, which is then put back into a `float` again. `fmodf()` just operates on `float` directly. **TL;DR:** And yeah, then I figured that adding such a function would also require writing a unit test for it, and that's getting too much for this PR. So I just went ahead and implemented that. It's now in the `main` branch and you can replace the call to `fmod()` with `mod_f_positive()`.
delta_y = last_key_y - first_key_y;
delta_y = last_key->vec[1][1] - first_key->vec[1][1];
Sybren A. Stüvel commented 2023-05-16 10:12:26 +02:00
Review
Copy Link

Use floor(x) if you want to always round down (instead of to zero). That way you don't have to correct the result of the computation.

Use `floor(x)` if you want to always round down (instead of to zero). That way you don't have to correct the result of the computation.
}
else if (time < first_key_x) {
else if (time < first_key->vec[1][0]) {
int offset_frame = fcu->bezt[i].vec[1][0] + fcu_x_range;
time = offset_frame + fcu_x_range * long_factor;
delta_y = first_key_y - last_key_y;
delta_y = first_key->vec[1][1] - last_key->vec[1][1];
}
else {
delta_y = 0;