blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 777 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 +75 -28
3 changed files with 75 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 8bb64f0083 - Show all commits

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

@ -221,6 +221,7 @@ class GRAPH_MT_select(Menu):
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")

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

@ -80,10 +80,11 @@ enum eGraphKeys_LeftRightSelect_Mode {
};
/* Defines for handle selection. */
enum eGraphKey_HandleSelect_Mode {
GRAPHKEYS_HANDLESEL_LR = 0,
enum eGraphKey_HandleSelect_Side {
GRAPHKEYS_HANDLESEL_BOTH = 0,
GRAPHKEYS_HANDLESEL_L,
GRAPHKEYS_HANDLESEL_R,
GRAPHKEYS_HANDLESEL_KEY,
};
/* defines for column-select mode */

97
source/blender/editors/space_graph/graph_select.cc
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"
@ -2068,13 +2070,14 @@ void GRAPH_OT_clickselect(wmOperatorType *ot)
/* Defines for handle select tool. */
static const EnumPropertyItem prop_graphkeys_leftright_handle_select_types[] = {
{GRAPHKEYS_HANDLESEL_LR, "BOTH", 0, "Select Both Handles", ""},
{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},
};
static short bezt_sel_left_handles(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
static short bezt_sel_left_handles(KeyframeEditData * /* ked */, BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_SEL_IDX(bezt, 0);
@ -2083,7 +2086,7 @@ static short bezt_sel_left_handles(KeyframeEditData *UNUSED(ked), BezTriple *bez
return 0;
}
static short bezt_sel_right_handles(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
static short bezt_sel_right_handles(KeyframeEditData * /* ked */, BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_DESEL_IDX(bezt, 0);
@ -2092,7 +2095,7 @@ static short bezt_sel_right_handles(KeyframeEditData *UNUSED(ked), BezTriple *be
return 0;
}
static short bezt_sel_both_handles(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
static short bezt_sel_both_handles(KeyframeEditData * /* ked */, BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_SEL_IDX(bezt, 0);
@ -2101,7 +2104,17 @@ static short bezt_sel_both_handles(KeyframeEditData *UNUSED(ked), BezTriple *bez
return 0;
}
static short bezt_desel_keys(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
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);
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!
}
return 0;
}
static short bezt_desel_keys(KeyframeEditData * /*ked */, BezTriple *bezt)
{
if (BEZT_ISSEL_ANY(bezt)) {
BEZT_DESEL_IDX(bezt, 1);
@ -2109,18 +2122,25 @@ static short bezt_desel_keys(KeyframeEditData *UNUSED(ked), BezTriple *bezt)
return 0;
}
static void graphkeys_de_select_handles(
bAnimContext *ac,
const short mode, // prop_graphkeys_leftright_handle_select_types
const bool deselect_keyframe)
/*
* 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
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".
* 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);
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. */
@ -2128,7 +2148,7 @@ static void graphkeys_de_select_handles(
continue;
}
switch (mode) {
case GRAPHKEYS_HANDLESEL_LR: {
case GRAPHKEYS_HANDLESEL_BOTH: {
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`.
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_both_handles, NULL);
break;
}
@ -2140,8 +2160,11 @@ static void graphkeys_de_select_handles(
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);
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`
}
}
if (deselect_keyframe) {
if (deselect_keyframe && (mode != GRAPHKEYS_HANDLESEL_KEY)) {
ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_desel_keys, NULL);
}
}
@ -2159,10 +2182,11 @@ static int graphkeys_select_handles_exec(bContext *C, wmOperator *op)
return OPERATOR_CANCELLED;
}
short leftright = RNA_enum_get(op->ptr, "mode");
bool deselect_keyframe = RNA_boolean_get(op->ptr, "deselect_keyframe");
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. */
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.
graphkeys_de_select_handles(&ac, leftright, deselect_keyframe);
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);
@ -2170,25 +2194,46 @@ static int graphkeys_select_handles_exec(bContext *C, wmOperator *op)
return OPERATOR_FINISHED;
}
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;
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.
row = uiLayoutRow(layout, false);
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.
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);
}
}
void GRAPH_OT_select_handles(wmOperatorType *ot)
{
/* identifiers */
ot->name = "Select Handles";
ot->idname = "GRAPH_OT_select_handles";
ot->description = "(De)Select handles of based on the active selection";
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;
ot->prop = RNA_def_enum(ot->srna,
"mode",
prop_graphkeys_leftright_handle_select_types,
GRAPHKEYS_HANDLESEL_LR,
"Mode",
"(De)Select handles based on selection");
ot->prop = RNA_def_boolean(ot->srna, "deselect_keyframe", 1, "Deselect Keyframes", "");
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");
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-09-15 15:08:40 +02:00
Outdated
Review
Copy Link

"Selects ... based on the given selection" is not the clearest explanation. Suggestion:

Of the selected keys & handles, change the selection to only the handles (left/right/both) or just the keys themselves.

"Selects ... based on the given selection" is not the clearest explanation. Suggestion: > Of the selected keys & handles, change the selection to only the handles (left/right/both) or just the keys themselves.
Nathan Vegdahl commented 2023-09-18 17:10:36 +02:00
Outdated
Review
Copy Link

Since the functionality has become a bit more flexible, I changed the description. Would love to have your eyes on it again.

Since the functionality has become a bit more flexible, I changed the description. Would love to have your eyes on it again.
RNA_def_boolean(ot->srna,
"deselect_keyframe",
1,
"Deselect Keyframes",
"Deselect keyframes or preserve the given keyframe selection");
}