Keyframe handles dont follow keyframes. #81813

New Issue

Luciano Muñoz Sessarego · 2020-10-18T20:34:14+02:00

2020-10-18 20:34:14 +02:00

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: f425f40c4e
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)

#81813.blend

**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: `f425f40c4e` 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) ![keyframes.gif](https://archive.blender.org/developer/F9013258/keyframes.gif) [#81813.blend](https://archive.blender.org/developer/F9016300/T81813.blend)

Luciano Muñoz Sessarego commented

2020-10-18 20:34:14 +02:00

Added subscriber: @LucianoMunoz

Luciano Muñoz Sessarego commented

2020-10-18 20:35:19 +02:00

Added subscriber: @wbmoss_dev

Sybren A. Stüvel commented

2020-10-19 16:15:38 +02:00

Added subscriber: @dr.sybren

Sybren A. Stüvel commented

2020-10-19 16:15:38 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Sybren A. Stüvel commented

2020-10-19 16:15:38 +02:00

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 0a66436fe5, 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.

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 0a66436fe5, 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.

Wayde Moss commented

2020-10-20 00:38:32 +02:00

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.

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.

Wayde Moss was assigned by Sybren A. Stüvel

2020-10-20 10:25:09 +02:00

Sybren A. Stüvel commented

2020-10-20 10:25:10 +02:00

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.

> 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.

Wayde Moss commented

2020-10-21 05:45:05 +02:00

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

> 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); ```

Sybren A. Stüvel commented

2020-11-10 11:11:32 +01:00

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.

blender-admin commented

2020-11-12 11:53:12 +01:00

This issue was referenced by dbbfba9428

This issue was referenced by dbbfba942867d907706222e3b0094cb18f517352

Sybren A. Stüvel commented

2020-11-12 11:58:16 +01:00

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.

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 commented

2020-11-12 12:05:04 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Sybren A. Stüvel closed this issue

2020-11-12 12:05:04 +01:00

Christoph Lendenfeld referenced this issue

2023-03-04 15:48:24 +01:00

Implementation: Modal Key Manipulation Operators #81785

Sign in to join this conversation.