Animation: Graph Editor Handle Selection #111143

Merged
Nathan Vegdahl merged 11 commits from nathanvegdahl/blender:select_handles_patch into main 2023-09-21 15:05:37 +02:00
4 changed files with 183 additions and 0 deletions

View File

@ -217,6 +217,16 @@ class GRAPH_MT_select(Menu):
props.extend = False
props.mode = 'RIGHT'
layout.separator()
props = layout.operator("graph.select_key_handles", text="Select Handles")
props.left_handle_action = 'SELECT'
props.right_handle_action = 'SELECT'
props.key_action = 'KEEP'
props = layout.operator("graph.select_key_handles", text="Select Key")
props.left_handle_action = 'DESELECT'
props.right_handle_action = 'DESELECT'
props.key_action = 'SELECT'
layout.separator()
layout.operator("graph.select_more")
layout.operator("graph.select_less")
nathanvegdahl marked this conversation as resolved Outdated

I think this should be 'DESELECT' because right now using this option you still have the key selected, meaning when using G to move it you move the key and not the handle.

I think this should be 'DESELECT' because right now using this option you still have the key selected, meaning when using G to move it you move the key and not the handle.

Good call.

But now that I think of it, do we even need the left/right versions in there at all? The primary use case is quickly selecting/deselecting both handles together. It's still good for the core operator to be flexible, so users with more niche use cases can do what they like. But for the provided menu items it might be best to just have "Select Handles" and "Select Keys".

Thoughts?

Good call. But now that I think of it, do we even need the left/right versions in there at all? [The primary use case](https://projects.blender.org/blender/blender/issues/103802#issuecomment-936441) is quickly selecting/deselecting both handles together. It's still good for the core operator to be flexible, so users with more niche use cases can do what they like. But for the provided menu items it might be best to just have "Select Handles" and "Select Keys". Thoughts?

In the context of this issue it makes sense to only have "Select Handles" and "Select Keys"

When talking about animation in general it could make sense to have a shortcut to only select handles on one side. But since the functionality exists it is reasonably easy to expose in a GUI. Maybe even a pie menu. But we can leave that for the future I think

In the context of this issue it makes sense to only have "Select Handles" and "Select Keys" When talking about animation in general it could make sense to have a shortcut to only select handles on one side. But since the functionality exists it is reasonably easy to expose in a GUI. Maybe even a pie menu. But we can leave that for the future I think

Yeah. I also checked with one of the animators here at the studio, and he said he would use Select Handles and Select Keys, but couldn't think of a common use for left/right. Of course, that's just a sample size of one. But I think it makes sense to keep things minimal now, and like you said we can always add more things and explore other ways to expose this in the future.

Yeah. I also checked with one of the animators here at the studio, and he said he would use Select Handles and Select Keys, but couldn't think of a common use for left/right. Of course, that's just a sample size of one. But I think it makes sense to keep things minimal now, and like you said we can always add more things and explore other ways to expose this in the future.

View File

@ -70,6 +70,7 @@ void GRAPH_OT_select_linked(struct wmOperatorType *ot);
void GRAPH_OT_select_more(struct wmOperatorType *ot);
void GRAPH_OT_select_less(struct wmOperatorType *ot);
void GRAPH_OT_select_leftright(struct wmOperatorType *ot);
void GRAPH_OT_select_key_handles(struct wmOperatorType *ot);
void GRAPH_OT_clickselect(struct wmOperatorType *ot);
/* defines for left-right select tool */
@ -79,6 +80,14 @@ enum eGraphKeys_LeftRightSelect_Mode {
GRAPHKEYS_LRSEL_RIGHT,
};
/* Defines for key/handles selection. */
enum eGraphKey_SelectKeyHandles_Action {
GRAPHKEYS_KEYHANDLESSEL_SELECT = 0,
GRAPHKEYS_KEYHANDLESSEL_DESELECT,
/* Leave the selection status as-is. */
GRAPHKEYS_KEYHANDLESSEL_KEEP,
};
/* defines for column-select mode */
enum eGraphKeys_ColumnSelect_Mode {
GRAPHKEYS_COLUMNSEL_KEYS = 0,

View File

@ -447,6 +447,7 @@ void graphedit_operatortypes()
WM_operatortype_append(GRAPH_OT_select_more);
WM_operatortype_append(GRAPH_OT_select_less);
WM_operatortype_append(GRAPH_OT_select_leftright);
WM_operatortype_append(GRAPH_OT_select_key_handles);
/* editing */
WM_operatortype_append(GRAPH_OT_snap);

View File

@ -30,6 +30,8 @@
#include "BKE_fcurve.h"
#include "BKE_nla.h"
#include "UI_interface_c.hh"
#include "UI_resources.hh"
#include "UI_view2d.hh"
#include "ED_anim_api.hh"
@ -2064,3 +2066,164 @@ void GRAPH_OT_clickselect(wmOperatorType *ot)
}
/** \} */
/* Key/handles selection */
/* Defines for key/handles select tool. */
static const EnumPropertyItem prop_graphkeys_select_key_handles_actions[] = {
{GRAPHKEYS_KEYHANDLESSEL_SELECT, "SELECT", 0, "Select", ""},
{GRAPHKEYS_KEYHANDLESSEL_DESELECT, "DESELECT", 0, "Deselect", ""},
{GRAPHKEYS_KEYHANDLESSEL_KEEP, "KEEP", 0, "Keep", "Leave as is"},
{0, NULL, 0, NULL, NULL},
};
/**
* Select/deselect different parts (e.g. left/right handles) of already-selected keys.
*
* The *_action parameters determine what action to take on each part of
* a key: select, deselect, or keep (do nothing).
*
* \param left_handle_action: selection action to perform on left handles.
* \param key_action: selection action to perform on the keys themselves.
* \param right_handle_action: selection action to perform on right handles.
*/
static void graphkeys_select_key_handles(
bAnimContext *ac,
const enum eGraphKey_SelectKeyHandles_Action left_handle_action,
const enum eGraphKey_SelectKeyHandles_Action key_action,
const enum eGraphKey_SelectKeyHandles_Action right_handle_action)
{
ListBase anim_data = {NULL, NULL};
const eAnimFilter_Flags filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_CURVE_VISIBLE |
ANIMFILTER_FCURVESONLY | ANIMFILTER_NODUPLIS);
ANIM_animdata_filter(
ac, &anim_data, filter, ac->data, static_cast<eAnimCont_Types>(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;
}
for (int i = 0; i < fcu->totvert; i++) {
BezTriple *bezt = &fcu->bezt[i];
nathanvegdahl marked this conversation as resolved Outdated

Personally I am not a fan of pointer incrementing. But I will leave this up to you

Personally I am not a fan of pointer incrementing. But I will leave this up to you

Ah, fair point. I basically just copy-pasted this from elsewhere. I agree that just using the index directly makes more sense here. Thanks for the catch!

Ah, fair point. I basically just copy-pasted this from elsewhere. I agree that just using the index directly makes more sense here. Thanks for the catch!
if (!BEZT_ISSEL_ANY(bezt)) {
continue;
}
switch (left_handle_action) {
case GRAPHKEYS_KEYHANDLESSEL_SELECT:
BEZT_SEL_IDX(bezt, 0);
break;
case GRAPHKEYS_KEYHANDLESSEL_DESELECT:
BEZT_DESEL_IDX(bezt, 0);
break;
case GRAPHKEYS_KEYHANDLESSEL_KEEP:
/* Do nothing. */
break;
}
switch (key_action) {
nathanvegdahl marked this conversation as resolved Outdated

Suggestion: "whether keyframes should" → "whether the keys themselves should".

Suggestion: "whether keyframes should" → "whether the keys themselves should".
case GRAPHKEYS_KEYHANDLESSEL_SELECT:
BEZT_SEL_IDX(bezt, 1);
break;
case GRAPHKEYS_KEYHANDLESSEL_DESELECT:
BEZT_DESEL_IDX(bezt, 1);
break;
case GRAPHKEYS_KEYHANDLESSEL_KEEP:
/* Do nothing. */
break;
}
switch (right_handle_action) {
case GRAPHKEYS_KEYHANDLESSEL_SELECT:
BEZT_SEL_IDX(bezt, 2);
break;
case GRAPHKEYS_KEYHANDLESSEL_DESELECT:
BEZT_DESEL_IDX(bezt, 2);
break;
case GRAPHKEYS_KEYHANDLESSEL_KEEP:
/* Do nothing. */
break;
}
nathanvegdahl marked this conversation as resolved Outdated

Remove the curly braces, they are only necessary when introducing new variables inside the case.

Remove the curly braces, they are only necessary when introducing new variables inside the `case`.
}
}
/* Cleanup */
ANIM_animdata_freelist(&anim_data);
}
static int graphkeys_select_key_handles_exec(bContext *C, wmOperator *op)
{
bAnimContext ac;
/* Get editor data. */
if (ANIM_animdata_get_context(C, &ac) == 0) {
nathanvegdahl marked this conversation as resolved Outdated

Missing break

Missing `break`
return OPERATOR_CANCELLED;
}
const eGraphKey_SelectKeyHandles_Action left_handle_action =
static_cast<eGraphKey_SelectKeyHandles_Action>(RNA_enum_get(op->ptr, "left_handle_action"));
const eGraphKey_SelectKeyHandles_Action key_action =
static_cast<eGraphKey_SelectKeyHandles_Action>(RNA_enum_get(op->ptr, "key_action"));
const eGraphKey_SelectKeyHandles_Action right_handle_action =
static_cast<eGraphKey_SelectKeyHandles_Action>(RNA_enum_get(op->ptr, "right_handle_action"));
graphkeys_select_key_handles(&ac, left_handle_action, key_action, right_handle_action);
WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_SELECTED, NULL);
return OPERATOR_FINISHED;
}
static void graphkeys_select_key_handles_ui(bContext * /* C */, wmOperator *op)
{
uiLayout *layout = op->layout;
uiLayout *row;
row = uiLayoutRow(layout, false);
uiItemR(row, op->ptr, "left_handle_action", UI_ITEM_NONE, NULL, ICON_NONE);
nathanvegdahl marked this conversation as resolved Outdated

You don't need to explain what the function does at the call site of the function. If it needs explanation, choose a better function name.

You don't need to explain what the function does at the call site of the function. If it needs explanation, choose a better function name.
row = uiLayoutRow(layout, false);
uiItemR(row, op->ptr, "right_handle_action", UI_ITEM_NONE, NULL, ICON_NONE);
row = uiLayoutRow(layout, false);
uiItemR(row, op->ptr, "key_action", UI_ITEM_NONE, NULL, ICON_NONE);
}
void GRAPH_OT_select_key_handles(wmOperatorType *ot)
{
/* identifiers */
ot->name = "Select Key / Handles";
ot->idname = "GRAPH_OT_select_key_handles";
ot->description =
"For selected keyframes, select/deselect any combination of the key itself and its handles";
nathanvegdahl marked this conversation as resolved Outdated

const, and move this line down to where keyframe_selection is actually used.

`const`, and move this line down to where `keyframe_selection` is actually used.
/* callbacks */
nathanvegdahl marked this conversation as resolved Outdated

Declare row here, and not at the start of the function. The days of AncientC are over.

Declare `row` here, and not at the start of the function. The days of AncientC are over.
ot->poll = graphop_visible_keyframes_poll;
ot->exec = graphkeys_select_key_handles_exec;
ot->ui = graphkeys_select_key_handles_ui;
/* flags */
ot->flag = OPTYPE_UNDO | OPTYPE_REGISTER;
RNA_def_enum(ot->srna,
"left_handle_action",
prop_graphkeys_select_key_handles_actions,
GRAPHKEYS_KEYHANDLESSEL_SELECT,
"Left Handle",
"Effect on the left handle");
RNA_def_enum(ot->srna,
"right_handle_action",
prop_graphkeys_select_key_handles_actions,
GRAPHKEYS_KEYHANDLESSEL_SELECT,
"Right Handle",
"Effect on the right handle");
RNA_def_enum(ot->srna,
"key_action",
prop_graphkeys_select_key_handles_actions,
GRAPHKEYS_KEYHANDLESSEL_KEEP,
"Key",
"Effect on the key itself");
}