blender
/
blender
136
404
Fork 586
Code Issues 6.2k Pull Requests 752 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 +16 -21
2 changed files with 16 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 65b92186e9 - Show all commits

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

@ -498,35 +498,30 @@ void time_offset_fcurve_segment(FCurve *fcu, FCurveSegment *segment, const float
const BezTriple *first_key = &fcu->bezt[0];
const int fcu_x_range = last_key->vec[1][0] - first_key->vec[1][0];
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).
float delta_y;
const float fcu_y_range = last_key->vec[1][1] - first_key->vec[1][1];
const float first_key_x = first_key->vec[1][0];
/* If we operate directly on the fcurve there will be a feedback loop
* so we need to capture the "y" values on an array to then apply them on a second 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
float *y_values = MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");
Christoph Lendenfeld commented 2023-04-13 12:55:36 +02:00
Outdated
Review
Copy Link

forgot the . at the end of the comment

forgot the . at the end of the comment
y_values[segment->length];
Christoph Lendenfeld commented 2023-04-13 12:58:53 +02:00
Outdated
Review
Copy Link

this doesn't compile for me. C doesn't allow variable length arrays.

you'd need to do MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");
and at the end of the function MEM_freeN(y_values); so to not have a memory leak

this doesn't compile for me. C doesn't allow variable length arrays. you'd need to do `MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");` and at the end of the function `MEM_freeN(y_values);` so to not have a memory leak
AresDeveaux commented 2023-04-14 21:27:03 +02:00
Outdated
Review
Copy Link

Why can I compile it? do I need to set something I haven't for make?

When you say C doesn't allow variable length array, I'm not sure I follow. I thought by declaring an array with a fixed value, C could handle it. Isn't segment->length an unchanged value for that segment?

Also the slider currently allows an overshoot in the positive axis, but doesn't allow to go below 0.
When overshooting on positive x it doesn't loop infinitely as you'd expect

Didn't know we could overshoot. Where is that set in the code, and how can I test it on the UI?

In regard to the memory leak, should I be adding those types of lines to the other sliders too?

Why can I compile it? do I need to set something I haven't for `make`? When you say C doesn't allow variable length array, I'm not sure I follow. I thought by declaring an array with a fixed value, C could handle it. Isn't `segment->length` an unchanged value for that segment? ``` Also the slider currently allows an overshoot in the positive axis, but doesn't allow to go below 0. When overshooting on positive x it doesn't loop infinitely as you'd expect ``` Didn't know we could overshoot. Where is that set in the code, and how can I test it on the UI? In regard to the memory leak, should I be adding those types of lines to the other sliders too?
Christoph Lendenfeld commented 2023-04-15 19:36:17 +02:00
Outdated
Review
Copy Link

variable length arrays are a C99 feature, but I think Blender runs on a different standard. But honestly I can't find in the wiki where that is.

A lot of the slider features are controller by the slider itself. You can see the shortcuts in the Status Bar
ED_slider_allow_overshoot_set allows you to change the behaviour

variable length arrays are a C99 feature, but I think Blender runs on a different standard. But honestly I can't find in the wiki where that is. A lot of the slider features are controller by the slider itself. You can see the shortcuts in the Status Bar `ED_slider_allow_overshoot_set` allows you to change the behaviour
AresDeveaux commented 2023-04-18 04:13:44 +02:00
Outdated
Review
Copy Link

Interesting! I didn't see those before.

I tried overshoot in time offset slider and as you say, to the right it works as expected, but to the left stops at 0. I then tried the ease slider, the neighbor slider and the default slider, and all of them stop at 0. I could try to find the problem, but it seems the issue is not in my function but in the slider itself and that might be beyond my understanding at this point.

Interesting! I didn't see those before. I tried overshoot in `time offset slider` and as you say, to the right it works as expected, but to the left stops at 0. I then tried the `ease slider`, the `neighbor slider` and the `default slider`, and all of them stop at 0. I could try to find the problem, but it seems the issue is not in my function but in the slider itself and that might be beyond my understanding at this point.
for (int i = segment->start_index; i < segment->start_index + segment->length; i++) {
/* This simulates the fcu curve moving in time. */
float time = fcu->bezt[i].vec[1][0] + fcu_x_range * factor;
/* 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->vec[1][0]) {
int offset_frame = fcu->bezt[i].vec[1][0] - fcu_x_range;
time = offset_frame + fcu_x_range * factor;
delta_y = last_key->vec[1][1] - first_key->vec[1][1];
}
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 * factor;
delta_y = first_key->vec[1][1] - last_key->vec[1][1];
}
else {
delta_y = 0;
const float time = fcu->bezt[i].vec[1][0] + fcu_x_range * factor;
/* Need to normalize time to first_key to specify that as the wrapping point. */
float wrapped_time = fmod(time - first_key_x, fcu_x_range) + first_key_x;
float delta_y = fcu_y_range * (int)((time - first_key_x) / fcu_x_range);
if (time - first_key_x < 0) {
/* Offset negative values since the modulo at fmod(-0.1, 1) would produce -0.1 instead of
* 0.9, as we'd need it to. */
wrapped_time += fcu_x_range;
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()`.
/* Similarly, (int)-0.1 produces 0 while we need it to be -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.
delta_y -= fcu_y_range;
}
const float key_y_value = evaluate_fcurve(fcu, time) + delta_y;
const float key_y_value = evaluate_fcurve(fcu, wrapped_time) + delta_y;
const int index_from_zero = i - segment->start_index;
y_values[index_from_zero] = key_y_value;
}

4
source/blender/editors/space_graph/graph_slider_ops.c
View File

@ -1177,12 +1177,12 @@ void GRAPH_OT_time_offset(wmOperatorType *ot)
RNA_def_float_factor(ot->srna,
"factor",
0.5f,
0.0f,
-FLT_MAX,
FLT_MAX,
"Curve Bend",
"Control the bend of the curve",
0.0f,
-1.0f,
1.0f);
}