Animation: Graph Editor Handle Selection #108142

Closed
Denys Hsu wants to merge 3 commits from cgtinker/blender:select_handles_patch into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
4 changed files with 189 additions and 0 deletions

View File

@ -193,6 +193,12 @@ class GRAPH_MT_select(Menu):
props.extend = False
props.mode = 'RIGHT'
layout.separator()
layout.operator("graph.select_handles", text="Select Handles").mode = 'BOTH'
layout.operator("graph.select_handles", text="Select Handles Left").mode = 'LEFT'
layout.operator("graph.select_handles", text="Select Handles Right").mode = 'RIGHT'
layout.operator("graph.select_handles", text="Select Keyframes").mode = 'KEY'
layout.separator()
layout.operator("graph.select_more")
layout.operator("graph.select_less")

View File

@ -74,6 +74,14 @@ enum eGraphKeys_LeftRightSelect_Mode {
GRAPHKEYS_LRSEL_RIGHT,
};
/* Defines for handle selection. */
enum eGraphKey_HandleSelect_Side {
GRAPHKEYS_HANDLESEL_BOTH = 0,
GRAPHKEYS_HANDLESEL_L,
GRAPHKEYS_HANDLESEL_R,
GRAPHKEYS_HANDLESEL_KEY,
};
/* defines for column-select mode */
enum eGraphKeys_ColumnSelect_Mode {
GRAPHKEYS_COLUMNSEL_KEYS = 0,
@ -124,6 +132,7 @@ void GRAPH_OT_sound_bake(struct wmOperatorType *ot);
void GRAPH_OT_smooth(struct wmOperatorType *ot);
void GRAPH_OT_euler_filter(struct wmOperatorType *ot);
void GRAPH_OT_select_handles(struct wmOperatorType *ot);
void GRAPH_OT_handle_type(struct wmOperatorType *ot);
void GRAPH_OT_interpolation_type(struct wmOperatorType *ot);
void GRAPH_OT_extrapolation_type(struct wmOperatorType *ot);

View File

@ -443,6 +443,8 @@ void graphedit_operatortypes(void)
WM_operatortype_append(GRAPH_OT_select_less);
WM_operatortype_append(GRAPH_OT_select_leftright);
WM_operatortype_append(GRAPH_OT_select_handles);
/* editing */
WM_operatortype_append(GRAPH_OT_snap);
WM_operatortype_append(GRAPH_OT_equalize_handles);

View File

@ -29,6 +29,9 @@
#include "BKE_fcurve.h"
#include "BKE_nla.h"
#include "UI_interface.h"
#include "UI_resources.h"
#include "UI_view2d.h"
#include "ED_anim_api.h"
@ -2035,3 +2038,172 @@ void GRAPH_OT_clickselect(wmOperatorType *ot)
}
/** \} */
/* Handle selection */
/* Defines for handle select tool. */
static const EnumPropertyItem prop_graphkeys_leftright_handle_select_types[] = {
{GRAPHKEYS_HANDLESEL_BOTH, "BOTH", 0, "Select Both Handles", ""},
{GRAPHKEYS_HANDLESEL_L, "LEFT", 0, "Select Left Handles", ""},
{GRAPHKEYS_HANDLESEL_R, "RIGHT", 0, "Select Right Handles", ""},
{GRAPHKEYS_HANDLESEL_KEY, "KEY", 0, "Select Keyframes", ""},
{0, NULL, 0, NULL, NULL},
cgtinker marked this conversation as resolved

make that KeyframeEditData *UNUSED(ked) for all those functions
This removes the warning you get when compiling

make that `KeyframeEditData *UNUSED(ked)` for all those functions This removes the warning you get when compiling
};
static short bezt_sel_left_handles(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_SEL_IDX(bezt, 0);
BEZT_DESEL_IDX(bezt, 2);
}
return 0;
}
static short bezt_sel_right_handles(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_DESEL_IDX(bezt, 0);
BEZT_SEL_IDX(bezt, 2);
}
return 0;
}
static short bezt_sel_both_handles(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_SEL_IDX(bezt, 0);
BEZT_SEL_IDX(bezt, 2);
}
return 0;
}
static short bezt_sel_keys(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_DESEL_IDX(bezt, 0);
BEZT_SEL_IDX(bezt, 1);
BEZT_DESEL_IDX(bezt, 2);
}
return 0;
}
static short bezt_desel_keys(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_DESEL_IDX(bezt, 1);
cgtinker marked this conversation as resolved

can be const int filter = ...

can be `const int filter = ...`
}
return 0;
}
cgtinker marked this conversation as resolved

Add BLI_assert(ale->type, ANIMTYPE_FCURVE) before the cast, just to have a little extra check that the cast is actually valid.

Add `BLI_assert(ale->type, ANIMTYPE_FCURVE)` before the cast, just to have a little extra check that the cast is actually valid.
cgtinker marked this conversation as resolved

this can be LISTBASE_FOREACH(bAnimListElem *, ale, &anim_data)
you then also don't need to have bAnimListElem *ale; before

this can be `LISTBASE_FOREACH(bAnimListElem *, ale, &anim_data)` you then also don't need to have `bAnimListElem *ale;` before
/*
* Select keyframe handles left/right/both or keys based on the given selection.
*
* mode: Determines which handles to select - left, right, or both.
* deselect_keyframe: Specifies whether keyframes should be deselected (true) or if the current
* selection should be preserved (false).
*/
static void graph_keys_select_handles(bAnimContext *ac,
const enum eGraphKey_HandleSelect_Side mode,
const bool deselect_keyframe)
{
ListBase anim_data = {NULL, NULL};
const int filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_CURVE_VISIBLE | ANIMFILTER_FCURVESONLY |
ANIMFILTER_NODUPLIS);
ANIM_animdata_filter(ac, &anim_data, filter, ac->data, ac->datatype);
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
BLI_assert(ale->type & ANIMTYPE_FCURVE);
FCurve *fcu = (FCurve *)ale->key_data;
/* Only continue if F-Curve has keyframes. */
if (fcu->bezt == NULL) {
continue;
}
switch (mode) {
case GRAPHKEYS_HANDLESEL_BOTH: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_both_handles, NULL);
break;
}
case GRAPHKEYS_HANDLESEL_R: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_right_handles, NULL);
break;
}
case GRAPHKEYS_HANDLESEL_L: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_left_handles, NULL);
break;
}
case GRAPHKEYS_HANDLESEL_KEY: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_keys, NULL);
}
}
if (deselect_keyframe && (mode != GRAPHKEYS_HANDLESEL_KEY)) {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_desel_keys, NULL);
}
}
/* Cleanup */
ANIM_animdata_freelist(&anim_data);
}
static int graphkeys_select_handles_exec(bContext *C, wmOperator *op)
{
bAnimContext ac;
/* Get editor data. */
if (ANIM_animdata_get_context(C, &ac) == 0) {
return OPERATOR_CANCELLED;
}
const short leftright = RNA_enum_get(op->ptr, "mode");
const bool deselect_keyframe = RNA_boolean_get(op->ptr, "deselect_keyframe");
/* Perform selection changes. */
cgtinker marked this conversation as resolved

you don't need to assign the RNA_def to anything
so it can just be RNA_def... instead of ot->prop = RNA_def

now I didn't know the operator type had a prop. And I am not sure what it is used for.
Maybe also something for sybren to clear up

you don't need to assign the RNA_def to anything so it can just be `RNA_def...` instead of `ot->prop = RNA_def` now I didn't know the operator type had a prop. And I am not sure what it is used for. Maybe also something for sybren to clear up

It's documented as:

  /**
   * Default rna property to use for generic invoke functions.
   * menus, enum search... etc. Example: Enum 'type' for a Delete menu.
   *
   * When assigned a string/number property,
   * immediately edit the value when used in a popup. see: #UI_BUT_ACTIVATE_ON_INIT.
   */
  PropertyRNA *prop;

So this means that it can be used for certain interactions as a special property.

@ChrisLend is right in that here the assignment is unnecessary. And if there is a property that should be marked as that special one property, it should only be assigned once, and not assigned and then immediately be overwritten again.

It's documented as: ```c /** * Default rna property to use for generic invoke functions. * menus, enum search... etc. Example: Enum 'type' for a Delete menu. * * When assigned a string/number property, * immediately edit the value when used in a popup. see: #UI_BUT_ACTIVATE_ON_INIT. */ PropertyRNA *prop; ``` So this means that it can be used for certain interactions as a special property. @ChrisLend is right in that here the assignment is unnecessary. And if there is a property that should be marked as that special one property, it should only be assigned once, and not assigned and then immediately be overwritten again.
graph_keys_select_handles(&ac, leftright, deselect_keyframe);
/* Set notifier that keyframe selection has changed. */
WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_SELECTED, NULL);
cgtinker marked this conversation as resolved

"Select handles based on selection" is too vague.

"Select handles based on selection" is too vague.
return OPERATOR_FINISHED;
}
static void graphkeys_select_handles_ui(bContext *UNUSED(C), wmOperator *op)
{
uiLayout *layout = op->layout;
uiLayout *row;
bool keyframe_selection = RNA_enum_get(op->ptr, "mode") == GRAPHKEYS_HANDLESEL_KEY;
row = uiLayoutRow(layout, false);
uiItemR(row, op->ptr, "mode", 0, NULL, ICON_NONE);
if (!keyframe_selection) {
uiItemR(layout, op->ptr, "deselect_keyframe", 0, NULL, ICON_NONE);
}
}
void GRAPH_OT_select_handles(wmOperatorType *ot)
{
/* identifiers */
ot->name = "Select Handles";
ot->idname = "GRAPH_OT_select_handles";
ot->description = "Select keyframe handles left/right/both or keys based on the given selection";
/* callbacks */
ot->poll = graphop_visible_keyframes_poll;
ot->exec = graphkeys_select_handles_exec;
ot->ui = graphkeys_select_handles_ui;
/* flags */
ot->flag = OPTYPE_UNDO | OPTYPE_REGISTER;
RNA_def_enum(
ot->srna,
"mode",
prop_graphkeys_leftright_handle_select_types,
GRAPHKEYS_HANDLESEL_BOTH,
"Mode",
"Selects corresponding keyframe handles (left, right, both or keys) based on the given "
"selection");
RNA_def_boolean(ot->srna,
"deselect_keyframe",
1,
"Deselect Keyframes",
"Deselect keyframes or preserve the given keyframe selection");
}