Animation: blend to infinity slider #106517
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#106517
Loading…
Reference in New Issue
No description provided.
Delete Branch "AresDeveaux/blender:blend_to_infinity_slider"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
New operator for the slider tool in the Graph Editor.
It blends selected keys to the slant of neighboring ones. It is used to push the segment closer to the values of the next or previous pose.
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.
First of all thanks for splitting it up. Much easier to review this way.
And you got a lot of things right. Sybren will be very happy you've added
const
where possible :DI've added a few notes where I think things can be improved
Also I am not sure about the name "Blend to Infinity"
It's more of a "Blend to Linear Extrapolation", but I see how that is a bit long.
We should discuss that in the next Blender meeting.
Some housekeeping:
Some more info here: https://wiki.blender.org/wiki/Style_Guide/Commit_Messages
@ -494,0 +498,4 @@
const float left_y = left_key->vec[1][1];
const BezTriple *right_key = fcurve_segment_end_get(fcu, segment->start_index + segment->length);
const float right_x = right_key->vec[1][0];
I think instead of splitting the vector into its two components it would be better to keep it as a vector and use the functions in
math_vector_inline.c
This will potentially simplify your code quite a lot.
@ -494,0 +514,4 @@
const float beyond_right_x = beyond_right_key->vec[1][0];
const float beyond_right_y = beyond_right_key->vec[1][1];
const float key_x_range = right_x - left_x;
I don't think you need this. At least I can't see it being used other than the check for 0.0f
If it is required that there is more than 1 key selected, update the comment because there is no potential divide by 0
@ -494,0 +560,4 @@
float new_x_delta = 0;
float new_y_delta = 0;
float refe = 0;
generally it is better to not shorten variables. I assume this means
reference
I think it would be even better to be more concrete and call it
reference_y
Also I think this, including the
if(slider_right_side)
, can be moved outside of the for loop.It seems to not change within the for loop
@ -494,0 +562,4 @@
float new_y_delta = 0;
float refe = 0;
/* This new deltas are used to determin the relationship between the current key and the
These new deltas are used to determine...
@ -494,0 +573,4 @@
refe = left_key->vec[1][1];
}
/* we use compound rule of 3 to find the "Y" delta we are missing using the other deltas we
We ...
comments start with capital letters.
WIP: Animation&Rigging: blend_to_infinity_sliderto Animation: blend to infinity sliderThanks for the feedback. I tried to address most of them. I keep forgetting to run
make format
and have additional commit just for that (I'll keep trying to make that work in VS Code preferences)I've been looking at the code properly now.
I noticed you have a potential "index out of bounds" issue.
fcurve_segment_start_get
already gets you the key with index 1 left of the selection. If you then pass itsegment->start_index -1
it could try to get index -1.I think I need to rename that function. It doesn't really reflect what it's doing
some more notes:
you only need
const bool slider_right_side
and you can then simplify the if elseyou can simplify the code even more by storing a reference key directly like this
that way a lot of the logic can move out of the if else block and the one in the for loop can be removed completely
Thanks so much for the feedback. I have a couple of thoughts.
I understood what
fcurve_segment_start_get
gets me (no need to change the name), but I do need not only the key next to the segment, I also need one beyond to know the slope of the 2 keys before and after the selection. If we focus just on the left side for a moment, instead of getting one more key to the left so I have theleft_key
and thebeyond_left_key
to work with, I could use the 'left_key' and the first key of the segment to determine the slope, but it would be the only slider that doesn't affect the first (and last) key of the segment (let me know if you prefer that).I still have not checked if there are functions on the file you referred to me before that could simplify the code but, with the formulas as they are right now, I do need 2 keys to calculate the slope.
In regard to the "index out of bound" you mentioned, you are right. I thought that could happen and I added an "if statement" to check if there are no keys before.
but now I realize I should have done that before trying to do
segment->start_index - 1
. I could rearrange the code so these lines are beforeIn regard to the simplification inside the loop, I don't see how I could do it as you say.
Yes, I will store the key, not the "y" value, but I still need the
new_x_delta
for this formula:new_y_delta = new_x_delta * y_delta / x_delta
.I'm sure I must be missing something. I guess, what I'm not understanding is how could I find the slope without having that extra value I'm using in the code?
yeah you need the
new_x_delta
, but you don't need it to be in an if else, if you've saved the key.new_x_delta = fcu->bezt[i].vec[1][0] - reference_key->vec[1][0];
Edit: I see you've saved the reference_key inside the for loop. You can move that outside, because it will never change within the function call
About the two keys you need to the left of the selected keys. You are right you need them. I think it would be good to explicitly check for that and issue a warning to the user if it happens. You can do that with
WM_report(RPT_WARNING, "Message!");
in
graph_slider_ops.c
Thanks. I didn't understand it before. That is actually clever. I will take a look.
On a related note, I was addressing the notes on another slider and as you said before, I have been confused about names all along.
On this line:
because the name of the function implies that it will return the start of the segment and I knew that
left_key
is what I call the left_neighbor key in my addon, I then assumed the start of the segment was the left neighboring key. So I had the index shifted by 1 in my head. Now I have to go to every slider (including this one) and make sure everything is correct.Because of that, I'm re-reading everything you said to make sure I followed.
that's why I mentioned I need to change the name of the function
a segment is defined as a continual selection of keyframes
yet the function
fcurve_segment_start_get
doesn't actually return the start of the segment, but the key before thatIt needs to be called something like
segment_left_neighbor_bezt_get
I simplified the code using your suggestions and even found other places to implement the same to simplify it even more.
I added the warning but it is not working right at the moment. I added it to the
blend_to_infinity_graph_keys
because that is where I found the data for the if statement, but obviously, that function is refreshing as the slider is updated.By the way, I am awful at coming up with messages, so any suggestion is appreciated.
As I'm trying to understand more how the functions work in
graph_slider_ops.c
, I realize there must be lines of code in that file I just left there because that is what the template I used had, but maybe are not needed for particular sliders. I don't completely have a grasp yet, but I'm just letting you know so you are on the look for that.getting there. only some small cleanup left to do :)
@ -494,0 +510,4 @@
if (factor >= 0.5) {
/* Stop the function if there is no key beyond the the right neighboring one. */
if (segment->start_index + segment->length >= fcu->totvert - 1) {
this can also be clearer.
segment->start_index + segment->length > fcu->totvert
I tried this change as well as the next one and it doesn't work as it should. Remember, we need 2 keys before and after the segment, the way you suggest just account for one. If I don't prevent not having 2 keys at each side of the segment the tool goes crazy.
@ -494,0 +520,4 @@
else {
/* Stop the function if there is no key beyond the left neighboring one. */
if (segment->start_index <= 1) {
This would be clearer if you checked explicitly to 0
segment->start_index == 0
the start index will never be smaller than 0
I am glad you didn't listen to me here, because I just realized what I suggested is wrong 🤦
@ -494,0 +528,4 @@
beyond_key = fcu->bezt[segment->start_index - 2];
}
y_delta = beyond_key.vec[1][1] - reference_key->vec[1][1];
since this is now outside of the if else, you can to
const float y_delta = ...
and then remove the lines 503 and 504
@ -494,0 +536,4 @@
return;
}
float new_x_delta;
this can move into the for loop and you can then call it
const float new_x_delta = ...
@ -494,0 +539,4 @@
float new_x_delta;
float new_y_delta;
if (factor >= 0.5) {
this code can be moved to the first
if else
@ -1063,0 +1076,4 @@
LISTBASE_FOREACH (FCurveSegment *, segment, &segments) {
if (segment->start_index + segment->length >= fcu->totvert - 1 ||
segment->start_index <= 1) {
WM_report(RPT_WARNING, "You need at least 2 keys to eider side of the selection.");
the conditions can also be simplified like above
you can also
continue
after the warning because the code cannot run anywayyour suggestion of adding continue to the condition for the warning works, but made me realize 2 things:
1.- I need to split the warning into two possibilities. One when we don't have two reference keys on the left, and another if we don't have two reference keys on the right. As it is now it locks the function even if one of the sides has all it needs (I added this)
2.- If we put continue on the condition in graph_slider_ops.c we don't need the same if statements in `keyframes_general.c' since it will never be executed. (I didn't delete them until you take a look at it)
By the way, doesn't it bother you the warning is blinking? I'm thinking it's doing that because of where I put it. I think it is refreshing as the function is called back when the slider is modified, but don't know in what function I should put it to avoid it.
Hey, Sorry I didn't check for those obvious changes I should have done when I took some lines outside from the loops.
I changed the slider to be bidirectional and deleted some extra lines that were not needed. I still have not deleted the
if
statements that test for the neighboring keys needed for the tool, just waiting for your confirmation. But they should not be needed now that the newif
statements with the warnings ingraph_slider_ops.c
make them redundant.the function
blend_to_infinity_fcurve_segment
should return a bool I thinkThat way you don't need to have the if statements twice and the function in
graph_slider_ops.c
doesn't need to concern itself with the limitations of this specific function. It just needs to know if it ran or not@ -1063,0 +1076,4 @@
LISTBASE_FOREACH (FCurveSegment *, segment, &segments) {
if (factor >= 0){
if (segment->start_index + segment->length >= fcu->totvert - 1) {
WM_report(RPT_WARNING, "You need at least 2 keys to the right side of the selection.");
Let's have the warning outside of the for-loop so it's only displayed once per modal cycle
so make a
bool all_segments_valid = true
and where the warning is now, set it to false
then after the 2 loops do a
if(!all_segments_valid)
Hopefully I did it right.
yep looks correct
About returning false when
x_delta
is 0This would make the error message inaccurate, because the issue isn't that there aren't enough keys to the sides of the selection.
But I wouldn't worry about it. It is so unlikely that
x_delta
is 0 that I think it doesn't need to be reported to the user explicitly@ -494,0 +523,4 @@
/* Avoids dividing by 0. */
if (x_delta == 0) {
return true;
shouldn't this return false?
also now that I think about it, I am not sure x_delta can ever be 0. It would only be 0 if the beyond key and the reference key are the same, which they cannot be since you are getting different indices.
Hm ok writing this now, you can have the curve in a state where two keys are at the same point in time. Shouldn't happen, but could...
Anyway, the function should return true if it ran and false if it didn't. Since you are exiting early here, I think it should return false.
You are right, it should be "false"
as of this PR #107402: Refactor: Replace move_key with BKE_fcurve_keyframe_move_value_with_handles, the
move_key
function no longer exists.it needs to replaced with
BKE_fcurve_keyframe_move_value_with_handles
also the function
blend_to_infinity_draw_status_header
can be replaced withcommon_draw_status_header(C, gso, "Blend to Infinity Keys")
this feature has been merged with this PR #110567: Animation: Match Slope slider
Pull request closed