Keyframe handles dont follow keyframes.
Closed, ResolvedPublicBUG
Actions

Description

System Information
Operating system: Windows-10-10.0.18362-SP0 64 Bits
Graphics card: GeForce RTX 2070 with Max-Q Design/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 456.71

Blender Version
Broken: version: 2.91.0 Alpha, branch: master, commit date: 2020-10-17 21:36, hash: rBf425f40c4e17
Worked: never because it's a new feature

Short description of error
When Alt dragging a value or using "copy to selected" the keyframe values are updated to the correct values but if the keyframes are in "free" or "aligned" mode the bezier handles stay in place which is wrong behaviour.
There is NO case where you want to move the keyframes and have the beziers stay in the same absolute place

Exact steps for others to reproduce the error
Create an object.
Set several keyframes.
Select the keyframes
Press V and select free or aligned.
Use alt drag to slide the value of the active keyframe.
keyframes move but handles stay.
(look at the gif above as it shows the wrong and correct behaviour)

T81813.blend583 KBDownload

Revisions and Commits

rB Blender
	rBdbbfba942867 Fix T81813: Keyframe handles don't follow keyframes

Related Objects

Mentioned In: T81785: Implementation: Modal Key Manipulation Operators
Mentioned Here: rB0a66436fe5f2: Fix T76597: Support Keyframe: Copy To Selected

Event Timeline

Luciano Muñoz Sessarego (looch) created this task.Oct 18 2020, 8:34 PM

Luciano Muñoz Sessarego (looch) added a project: Animation & Rigging.

Luciano Muñoz Sessarego (looch) added a subscriber: Wayde Moss (GuiltyGhost).

I can confirm. Also when the keys are set to Auto Clamped or Auto, the handles don't move along when you drag the "Key Frame" property slider.

Update: This may be exposed by rB0a66436fe5f2: Fix T76597: Support Keyframe: Copy To Selected, but I don't think it's caused by it. In 2.90 release you can also drag the Key Frame property slider, and the handles stay put.

Sybren A. Stüvel (sybren) updated the task description. (Show Details)Oct 19 2020, 4:17 PM

Sybren A. Stüvel (sybren) updated the task description. (Show Details)Oct 19 2020, 4:47 PM

Sybren A. Stüvel (sybren) added a project: User Interface.

It looks like the rna for Keyframe Co value writes do not adjust handle values at all in (rna_fcurve.c) rna_FKeyframe_ctrlpoint_set(). When transforming through the GE, (transform.convert_graph.c) flushTransGraphData() will apply the expected offset to the handles. This is only necessary for Free and Aligned handle types. A call the fcurve_channel.update() is enough to fix it for Auto, Clamped, and Vector handle types. Fixing this within rna_FKeyframe_ctrlpoint_set() would affect any python scripts that assumed the existing behavior.

I can confirm. Also when the keys are set to Auto Clamped or Auto, the handles don't move along when you drag the "Key Frame" property slider.

I'm not sure why this happens though. Using the slider for the Value property properly updates Auto, Clamped, and Vector, handle types properly Value-wise. But dragging the slider for the Frame property doesn't properly update them. Using fcurve_channel.update() should be enough to fix the issue for these handle types. I assumed that call happens already but maybe not due to these issues so I need to look into it more later.

Fixing this within rna_FKeyframe_ctrlpoint_set() would affect any python scripts that assumed the existing behavior.

The RNA setter functions shouldn't do anything except just set the values. If other things are necessary, this should be done in an update function (or in some other place).

I need to look into it more later.

Thanks for looking into it.

I had some improvement for the "Key Frame" slider by adding this line:

diff --cc source/blender/editors/space_graph/graph_buttons.c
index a491ce29bd4,a491ce29bd4..20a23639678
--- a/source/blender/editors/space_graph/graph_buttons.c
+++ b/source/blender/editors/space_graph/graph_buttons.c
@@@ -430,6 -430,6 +430,7 @@@ static void graph_panel_key_properties(
                        0,
                        0,
                        NULL);
++      UI_but_func_set(but, graphedit_activekey_update_cb, fcu, bezt);
  
        uiItemL_respect_property_split(col, IFACE_("Value"), ICON_NONE);
        but = uiDefButR(block,

It doesn't solve the rest of the issues, though.

Sybren A. Stüvel (sybren) changed the subtype of this task from "Report" to "Bug".Oct 20 2020, 2:31 PM

Sybren A. Stüvel (sybren) moved this task from Backlog to Short Term on the Animation & Rigging board.

The RNA setter functions shouldn't do anything except just set the values. If other things are necessary, this should be done in an update function (or in some other place).

After asking in the the ui-module-team's blender chat, it seems there's no other way. Ultimately, we'd have to track the previous Co value manually outside of the UI system then apply that offset when the slider is modified. The patch below solves all the problems but it's also a bit of a loop hole. The patch effectively provides an alternative RNA property for keyframe.co where only the set() function differs. I haven't pushed a differential for it because I'm not sure if it's the "Blender way".

diff --git a/source/blender/editors/space_graph/graph_buttons.c b/source/blender/editors/space_graph/graph_buttons.c
index a491ce29bd4..6db68cd2a0b 100644
--- a/source/blender/editors/space_graph/graph_buttons.c
+++ b/source/blender/editors/space_graph/graph_buttons.c
@@ -423,13 +423,14 @@ static void graph_panel_key_properties(const bContext *C, Panel *panel)
                       but_max_width,
                       UI_UNIT_Y,
                       &bezt_ptr,
-                      "co",
+                      "ui_co",
                       0,
                       0,
                       0,
                       0,
                       0,
                       NULL);
+      UI_but_func_set(but, graphedit_activekey_update_cb, fcu, bezt);
 
       uiItemL_respect_property_split(col, IFACE_("Value"), ICON_NONE);
       but = uiDefButR(block,
@@ -441,7 +442,7 @@ static void graph_panel_key_properties(const bContext *C, Panel *panel)
                       but_max_width,
                       UI_UNIT_Y,
                       &bezt_ptr,
-                      "co",
+                      "ui_co",
                       1,
                       0,
                       0,
@@ -450,8 +451,6 @@ static void graph_panel_key_properties(const bContext *C, Panel *panel)
                       NULL);
       UI_but_func_set(but, graphedit_activekey_update_cb, fcu, bezt);
       UI_but_unit_type_set(but, unit);
-
-      UI_but_func_set(but, graphedit_activekey_update_cb, fcu, bezt);
     }
 
     /* previous handle - only if previous was Bezier interpolation */
diff --git a/source/blender/makesrna/intern/rna_fcurve.c b/source/blender/makesrna/intern/rna_fcurve.c
index 1c5e3688312..5808e7d47bc 100644
--- a/source/blender/makesrna/intern/rna_fcurve.c
+++ b/source/blender/makesrna/intern/rna_fcurve.c
@@ -463,6 +463,27 @@ static void rna_FKeyframe_ctrlpoint_set(PointerRNA *ptr, const float *values)
   bezt->vec[1][1] = values[1];
 }
 
+static void rna_FKeyframe_ctrlpoint_ui_set(PointerRNA *ptr, const float *values)
+{
+  BezTriple *bezt = (BezTriple *)ptr->data;
+
+  const float frame_delta = values[0] - bezt->vec[1][0];
+  const float value_delta = values[1] - bezt->vec[1][1];
+
+  /** To match the behavior of transforming the keyframe Co using the Graph Editor
+   * (transform_convert_graph.c) flushTransGraphData(), we will also move the handles by
+   * the same amount as the Co delta. */
+
+  bezt->vec[0][0] += frame_delta;
+  bezt->vec[0][1] += value_delta;
+
+  bezt->vec[1][0] = values[0];
+  bezt->vec[1][1] = values[1];
+
+  bezt->vec[2][0] += frame_delta;
+  bezt->vec[2][1] += value_delta;
+}
+
 /* ****************************** */
 
 static void rna_FCurve_RnaPath_get(PointerRNA *ptr, char *value)
@@ -2098,6 +2119,18 @@ static void rna_def_fkeyframe(BlenderRNA *brna)
   RNA_def_property_ui_text(prop, "Control Point", "Coordinates of the control point");
   RNA_def_property_update(prop, NC_ANIMATION | ND_KEYFRAME | NA_EDITED, "rna_Keyframe_update");
 
+  prop = RNA_def_property(
+      srna, "ui_co", PROP_FLOAT, PROP_COORDS); /* keyframes are dimensionless */
+  RNA_def_property_array(prop, 2);
+  RNA_def_property_float_funcs(
+      prop, "rna_FKeyframe_ctrlpoint_get", "rna_FKeyframe_ctrlpoint_ui_set", NULL);
+  RNA_def_property_ui_text(
+      prop,
+      "Control Point",
+      "Coordinates of the control point. Note: Changing this value also updates the handles "
+      "similar to using the graph editor transform operator");
+  RNA_def_property_update(prop, NC_ANIMATION | ND_KEYFRAME | NA_EDITED, "rna_Keyframe_update");
+
   prop = RNA_def_property(
       srna, "handle_right", PROP_FLOAT, PROP_COORDS); /* keyframes are dimensionless */
   RNA_def_property_array(prop, 2);

Hans Goudey (HooglyBoogly) moved this task from Backlog to Bugs on the User Interface board.Oct 21 2020, 10:18 PM

Julian Eisel (Severin) moved this task from Bugs to Responsibility of Other Project/Module on the User Interface board.Oct 26 2020, 8:14 PM

Christoph Lendenfeld (ChrisLend) mentioned this in T81785: Implementation: Modal Key Manipulation Operators.Nov 5 2020, 5:24 PM

I agree that it's a bit weird to have two properties, but if it's the clearest way that's possible right now, I don't see a big objection.

One thing I noticed just now, I think co_ui would be a better name for the property than ui_co. When ordering the properties alphabetically the two properties will be next to each other and easier to find. It'll also help people when they use code completion in the Python console, because co<tab> will list both co and co_ui.

Sybren A. Stüvel (sybren) closed this task as Resolved by committing rBdbbfba942867: Fix T81813: Keyframe handles don't follow keyframes.Nov 12 2020, 12:05 PM

Sybren A. Stüvel (sybren) added a commit: rBdbbfba942867: Fix T81813: Keyframe handles don't follow keyframes.

Keyframe handles dont follow keyframes.Closed, ResolvedPublicBUGActions

Description

Revisions and Commits

Related Objects

Event Timeline

Keyframe handles dont follow keyframes.
Closed, ResolvedPublicBUG
Actions