Animation: time offset slider #106520

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

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
First-time contributor

New operator for the slider tool in the Graph Editor.

It shifts the value in time of selected keys. It achieves the same as moving the selected keys in time with the added advantage of keeping the keys in the same frame.

It uses the structure already created by @ChrisLend. The main functionality chancge is in 'keyframes_general.c', the rest for the most part just follows what hi did.

New operator for the slider tool in the Graph Editor. It shifts the value in time of selected keys. It achieves the same as moving the selected keys in time with the added advantage of keeping the keys in the same frame. It uses the structure already created by @ChrisLend. The main functionality chancge is in 'keyframes_general.c', the rest for the most part just follows what hi did.
AresDeveaux added 2 commits 2023-04-04 05:56:40 +02:00
Nate Rupsis added the
Module
Animation & Rigging
label 2023-04-06 20:59:23 +02:00
Nate Rupsis added this to the Animation & Rigging project 2023-04-06 20:59:31 +02:00
AresDeveaux changed title from WIP: Animation&Rigging: time_offset_slider to Animation: time offset slider 2023-04-10 05:35:02 +02:00
AresDeveaux added 1 commit 2023-04-11 01:25:18 +02:00
Christoph Lendenfeld requested changes 2023-04-13 12:59:30 +02:00
Christoph Lendenfeld left a comment
Member

needs a memory allocation. Currently it doesn't compile for me.

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

needs a memory allocation. Currently it doesn't compile for me. 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
@ -494,0 +504,4 @@
float delta_y;
/* 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*/

forgot the . at the end of the comment

forgot the . at the end of the comment
@ -494,0 +505,4 @@
/* 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*/
float y_values[segment->length];

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
Author
First-time contributor

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?

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
Author
First-time contributor

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.
Author
First-time contributor

I added the lines you suggested to avoid memory leaks, and when compiling I get this message in regard to one of the lines:

warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
  MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");

When I add MEM_freeN(y_values); at the end of the function, Blender crashes any time I try to use the slider.

I added the lines you suggested to avoid memory leaks, and when compiling I get this message in regard to one of the lines: ``` warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result] MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples"); ``` When I add `MEM_freeN(y_values);` at the end of the function, Blender crashes any time I try to use the slider.

I added the lines you suggested to avoid memory leaks, and when compiling I get this message in regard to one of the lines:

warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
  MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");

When I add MEM_freeN(y_values); at the end of the function, Blender crashes any time I try to use the slider.

sorry might have been unclear
the allocation needs to be like this
float *y_values = MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");

> I added the lines you suggested to avoid memory leaks, and when compiling I get this message in regard to one of the lines: > > ``` > warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result] > MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples"); > ``` > > When I add `MEM_freeN(y_values);` at the end of the function, Blender crashes any time I try to use the slider. sorry might have been unclear the allocation needs to be like this `float *y_values = MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");`
AresDeveaux added 2 commits 2023-04-18 04:59:11 +02:00
AresDeveaux requested review from Christoph Lendenfeld 2023-04-18 04:59:19 +02:00
Author
First-time contributor

I added the lines to avoid memory leaks.

Also, as stated previously I tried overshoot in time offset slider and stops at 0 as you said, but also all the other sliders (ease, blend to neighbor, blend to default). It seems to be an issue with the overshot tool of the slider or a setting I don't know about.

I added the lines to avoid memory leaks. Also, as stated previously I tried overshoot in `time offset slider` and stops at 0 as you said, but also all the other sliders (`ease`, `blend to neighbor`, `blend to default`). It seems to be an issue with the overshot tool of the slider or a setting I don't know about.
AresDeveaux added 1 commit 2023-04-18 07:17:23 +02:00
Author
First-time contributor

Good news!... The 0 limit when using overshoot in negative values is gone now.

Since the bidirectional slider worked so well addressing Sybren's note, I decided to use it on every slider because most of them are bidirectional (with the exception of push pull and blend to frame now that which only uses the current frame). In doing that I realized the 0 limit of the overshoot goes away when the slider is bidirectional. So maybe we need to change ease slider, blend to neighbor and blend to default.

It actually makes sense that when a slider is not bidirectional would not overshoot less than 0. For example, on push pull on 0 the curve becomes linear and you don't want it to go beyond that, but on the other direction, you do want to overshoot.

Good news!... The 0 limit when using overshoot in negative values is gone now. Since the bidirectional slider worked so well addressing Sybren's note, I decided to use it on every slider because most of them are bidirectional (with the exception of `push pull` and `blend to frame` now that which only uses the current frame). In doing that I realized the 0 limit of the `overshoot` goes away when the slider is bidirectional. So maybe we need to change `ease slider`, `blend to neighbor` and `blend to default`. It actually makes sense that when a slider is not bidirectional would not overshoot less than 0. For example, on `push pull` on 0 the curve becomes linear and you don't want it to go beyond that, but on the other direction, you do want to overshoot.
Christoph Lendenfeld requested changes 2023-04-20 12:23:23 +02:00
Christoph Lendenfeld left a comment
Member

one more improvement, in order for the overshoot to wrap endlessly we can modify the code to this

  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];

  for (int i = segment->start_index; i < segment->start_index + segment->length; i++) {
    /* This simulates the fcu curve moving in time. */
    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;
      /* Similarly, (int)-0.1 produces 0 while we need it to be -1. */
      delta_y -= fcu_y_range;
    }
    ...
  }

the if statement in there annoys me but I couldn't think of a way to get rid of it.

one more improvement, in order for the overshoot to wrap endlessly we can modify the code to this ``` 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]; for (int i = segment->start_index; i < segment->start_index + segment->length; i++) { /* This simulates the fcu curve moving in time. */ 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; /* Similarly, (int)-0.1 produces 0 while we need it to be -1. */ delta_y -= fcu_y_range; } ... } ``` the if statement in there annoys me but I couldn't think of a way to get rid of it.
@ -494,0 +503,4 @@
/* 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. */
float *y_values = MEM_callocN(sizeof(float) * segment->length, "Time Offset Samples");
y_values[segment->length];

you don't need this line

you don't need this line
Author
First-time contributor

While trying to understand what you are doing in those lines, I went back to one comment you had a while back and could not understand what you meant.

When overshooting on positive x it doesn't loop infinitely as you'd expect

Everything was working just fine on my end, and then I realized it was working because I have a cycle modifier (just to better see what the curve is suppose to do before and after) apparently the modifier was helping the operator. I now deleted the modifier and I see what you mean.

While trying to understand what you are doing in those lines, I went back to one comment you had a while back and could not understand what you meant. ``` When overshooting on positive x it doesn't loop infinitely as you'd expect ``` Everything was working just fine on my end, and then I realized it was working because I have a cycle modifier (just to better see what the curve is suppose to do before and after) apparently the modifier was helping the operator. I now deleted the modifier and I see what you mean.
AresDeveaux added 1 commit 2023-04-21 04:57:13 +02:00
AresDeveaux requested review from Christoph Lendenfeld 2023-04-21 04:57:25 +02:00
Christoph Lendenfeld approved these changes 2023-04-21 10:42:49 +02:00
Christoph Lendenfeld left a comment
Member

no more complaints :)

no more complaints :)
Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-04-21 10:43:10 +02:00
Sybren A. Stüvel requested changes 2023-05-16 14:09:21 +02:00
Sybren A. Stüvel left a comment
Member

A few minor remarks. Most importantly, the PR description and the time_offset_fcurve_segment function name only mention 'offset', but the code seems to wrap as well. Please make sure the PR description covers the functionality entirely, as it'll also be used as commit message, and to fill out the release notes, user manual, etc.

A few minor remarks. Most importantly, the PR description and the `time_offset_fcurve_segment` function name only mention 'offset', but the code seems to wrap as well. Please make sure the PR description covers the functionality entirely, as it'll also be used as commit message, and to fill out the release notes, user manual, etc.
@ -491,6 +491,50 @@ void ease_fcurve_segment(FCurve *fcu, FCurveSegment *segment, const float factor
/* ---------------- */
void time_offset_fcurve_segment(FCurve *fcu, FCurveSegment *segment, const float factor)

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.
@ -494,0 +497,4 @@
const BezTriple *last_key = &fcu->bezt[fcu->totvert - 1];
const BezTriple *first_key = &fcu->bezt[0];
const int fcu_x_range = last_key->vec[1][0] - first_key->vec[1][0];

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).
@ -494,0 +516,4 @@
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;

(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()`.
@ -494,0 +517,4 @@
/* 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;
/* Similarly, (int)-0.1 produces 0 while we need it to be -1. */

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.
Christoph Lendenfeld self-assigned this 2023-07-27 18:58:05 +02:00

this has been landed here: #110540: Anim: Time Offset Slider

this has been landed here: [#110540: Anim: Time Offset Slider](https://projects.blender.org/blender/blender/pulls/110540)

Pull request closed

Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#106520
No description provided.