blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 779 Projects 19 Wiki Activity

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
Conversation 5 Commits 11 Files Changed 4 +108 -107
3 changed files with 108 additions and 107 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 47e50a8383 - Show all commits

20
scripts/startup/bl_ui/space_graph.py
View File

@ -218,10 +218,22 @@ class GRAPH_MT_select(Menu):
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'
props = layout.operator("graph.select_handles", text="Select Handles")
props.left_handle_action = 'SELECT'
props.right_handle_action = 'SELECT'
props.k_actioney = 'KEEP'
props = layout.operator("graph.select_handles", text="Select Handles Left")
props.left_handle_action = 'SELECT'
props.right_handle_action = 'DESELECT'
props.key_action = 'KEEP'
props = layout.operator("graph.select_handles", text="Select Handles Right")
props.left_handle_action = 'DESELECT'
props.right_handle_action = 'SELECT'
props.key_action = 'KEEP'
nathanvegdahl marked this conversation as resolved Outdated
Christoph Lendenfeld commented 2023-09-21 10:32:21 +02:00
Outdated
Review
Copy Link

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.
Nathan Vegdahl commented 2023-09-21 12:48:50 +02:00
Outdated
Review
Copy Link

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?
Christoph Lendenfeld commented 2023-09-21 13:02:48 +02:00
Outdated
Review
Copy Link

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
👍 1
Nathan Vegdahl commented 2023-09-21 14:03:23 +02:00
Outdated
Review
Copy Link

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.
props = layout.operator("graph.select_handles", text="Select Keyframes")
props.left_handle_action = 'DESELECT'
props.right_handle_action = 'DESELECT'
props.key_action = 'SELECT'
layout.separator()
layout.operator("graph.select_more")

10
source/blender/editors/space_graph/graph_intern.h
View File

@ -80,11 +80,11 @@ enum eGraphKeys_LeftRightSelect_Mode {
};
/* Defines for handle selection. */
enum eGraphKey_HandleSelect_Side {
GRAPHKEYS_HANDLESEL_BOTH = 0,
GRAPHKEYS_HANDLESEL_L,
GRAPHKEYS_HANDLESEL_R,
GRAPHKEYS_HANDLESEL_KEY,
enum eGraphKey_HandleSelect_Action {
GRAPHKEYS_HANDLESEL_SELECT = 0,
GRAPHKEYS_HANDLESEL_DESELECT,
/* Leave the selection status as-is. */
GRAPHKEYS_HANDLESEL_KEEP,
};
/* defines for column-select mode */

185
source/blender/editors/space_graph/graph_select.cc
View File

@ -2069,69 +2069,27 @@ 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", ""},
static const EnumPropertyItem prop_graphkeys_handle_select_actions[] = {
{GRAPHKEYS_HANDLESEL_SELECT, "SELECT", 0, "Select", ""},
{GRAPHKEYS_HANDLESEL_DESELECT, "DESELECT", 0, "Deselect", ""},
{GRAPHKEYS_HANDLESEL_KEEP, "KEEP", 0, "Keep", "Leave as-is"},
{0, NULL, 0, NULL, NULL},
};
static short bezt_sel_left_handles(KeyframeEditData * /* 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 * /* 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 * /* 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 * /* 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 * /*ked */, BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_DESEL_IDX(bezt, 1);
}
return 0;
}
/*
* Select keyframe handles left/right/both or keys based on the given selection.
/**
* Select/deselect different parts (e.g. left/right handles) of already-selected keys.
*
* 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).
* 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 graph_keys_select_handles(bAnimContext *ac,
const enum eGraphKey_HandleSelect_Side mode,
const bool deselect_keyframe)
const enum eGraphKey_HandleSelect_Action left_handle_action,
const enum eGraphKey_HandleSelect_Action key_action,
const enum eGraphKey_HandleSelect_Action right_handle_action)
{
ListBase anim_data = {NULL, NULL};
@ -2147,26 +2105,50 @@ static void graph_keys_select_handles(bAnimContext *ac,
if (fcu->bezt == NULL) {
continue;
}
switch (mode) {
case GRAPHKEYS_HANDLESEL_BOTH: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_both_handles, NULL);
break;
int i = 0;
BezTriple *bezt = fcu->bezt;
for (; i < fcu->totvert; bezt++, i++) {
if (!BEZT_ISSEL_ANY(bezt)) {
nathanvegdahl marked this conversation as resolved Outdated
Christoph Lendenfeld commented 2023-09-21 14:35:11 +02:00
Outdated
Review
Copy Link

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
Nathan Vegdahl commented 2023-09-21 14:44:45 +02:00
Outdated
Review
Copy Link

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!
continue;
}
case GRAPHKEYS_HANDLESEL_R: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_right_handles, NULL);
break;
switch (left_handle_action) {
case GRAPHKEYS_HANDLESEL_SELECT:
BEZT_SEL_IDX(bezt, 0);
break;
case GRAPHKEYS_HANDLESEL_DESELECT:
BEZT_DESEL_IDX(bezt, 0);
break;
case GRAPHKEYS_HANDLESEL_KEEP:
/* Do nothing. */
break;
}
case GRAPHKEYS_HANDLESEL_L: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_left_handles, NULL);
break;
switch (key_action) {
case GRAPHKEYS_HANDLESEL_SELECT:
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:01:45 +02:00
Outdated
Review
Copy Link

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

Suggestion: "whether keyframes should" → "whether the keys themselves should".
BEZT_SEL_IDX(bezt, 1);
break;
case GRAPHKEYS_HANDLESEL_DESELECT:
BEZT_DESEL_IDX(bezt, 1);
break;
case GRAPHKEYS_HANDLESEL_KEEP:
/* Do nothing. */
break;
}
case GRAPHKEYS_HANDLESEL_KEY: {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_keys, NULL);
switch (right_handle_action) {
case GRAPHKEYS_HANDLESEL_SELECT:
BEZT_SEL_IDX(bezt, 2);
break;
case GRAPHKEYS_HANDLESEL_DESELECT:
BEZT_DESEL_IDX(bezt, 2);
break;
case GRAPHKEYS_HANDLESEL_KEEP:
/* Do nothing. */
break;
}
}
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:02:49 +02:00
Outdated
Review
Copy Link

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`.
if (deselect_keyframe && (mode != GRAPHKEYS_HANDLESEL_KEY)) {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_desel_keys, NULL);
}
}
/* Cleanup */
@ -2182,13 +2164,15 @@ static int graphkeys_select_handles_exec(bContext *C, wmOperator *op)
return OPERATOR_CANCELLED;
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:02:54 +02:00
Outdated
Review
Copy Link

Missing break

Missing `break`
}
const eGraphKey_HandleSelect_Side leftright = static_cast<eGraphKey_HandleSelect_Side>(
RNA_enum_get(op->ptr, "mode"));
const bool deselect_keyframe = RNA_boolean_get(op->ptr, "deselect_keyframe");
/* Perform selection changes. */
graph_keys_select_handles(&ac, leftright, deselect_keyframe);
const eGraphKey_HandleSelect_Action left_handle_action =
static_cast<eGraphKey_HandleSelect_Action>(RNA_enum_get(op->ptr, "left_handle_action"));
const eGraphKey_HandleSelect_Action key_action = static_cast<eGraphKey_HandleSelect_Action>(
RNA_enum_get(op->ptr, "key_action"));
const eGraphKey_HandleSelect_Action right_handle_action =
static_cast<eGraphKey_HandleSelect_Action>(RNA_enum_get(op->ptr, "right_handle_action"));
graph_keys_select_handles(&ac, left_handle_action, key_action, right_handle_action);
/* Set notifier that keyframe selection has changed. */
WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_SELECTED, NULL);
return OPERATOR_FINISHED;
@ -2199,13 +2183,12 @@ static void graphkeys_select_handles_ui(bContext * /* 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", UI_ITEM_NONE, NULL, ICON_NONE);
if (!keyframe_selection) {
uiItemR(layout, op->ptr, "deselect_keyframe", UI_ITEM_NONE, NULL, ICON_NONE);
}
uiItemR(row, op->ptr, "left_handle_action", UI_ITEM_NONE, NULL, ICON_NONE);
row = uiLayoutRow(layout, false);
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:03:35 +02:00
Outdated
Review
Copy Link

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.
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_handles(wmOperatorType *ot)
@ -2213,7 +2196,8 @@ 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";
ot->description =
"Select/deselect the different parts of keys that have at least one part already selected";
/* callbacks */
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:06:12 +02:00
Outdated
Review
Copy Link

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.
ot->poll = graphop_visible_keyframes_poll;
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:06:31 +02:00
Outdated
Review
Copy Link

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.
@ -2223,17 +2207,22 @@ void GRAPH_OT_select_handles(wmOperatorType *ot)
/* 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");
RNA_def_enum(ot->srna,
"left_handle_action",
prop_graphkeys_handle_select_actions,
GRAPHKEYS_HANDLESEL_SELECT,
"Left Handle",
"How to affect the selection of the left handle");
RNA_def_enum(ot->srna,
"right_handle_action",
prop_graphkeys_handle_select_actions,
GRAPHKEYS_HANDLESEL_SELECT,
"Right Handle",
"How to affect the selection of the right handle");
RNA_def_enum(ot->srna,
"key_action",
prop_graphkeys_handle_select_actions,
GRAPHKEYS_HANDLESEL_KEEP,
"Key",
"How to affect the selection of the key itself");
}