Anim: Ctrl-shift channel selection. #118429

Pratik Borhade · 2024-02-18T07:31:13+01:00

Pratik Borhade commented

2024-02-18 07:31:13 +01:00

New keymap (ctrl-shift LMB) to extend range selection without clearing
previous selection.
In click_select_channel_* functions, order of extend_range and extend
is interchanged.

Resolves #108448

New keymap (ctrl-shift LMB) to extend range selection without clearing previous selection. In `click_select_channel_*` functions, order of `extend_range` and `extend` is interchanged. Resolves #108448

Pratik Borhade added 1 commit 2024-02-18 07:31:24 +01:00

90d317bc0f WIP: Anim: Ctrl-shift channel selection.

Resolves #108448

Pratik Borhade added the

Module

Animation & Rigging

label 2024-02-18 07:31:52 +01:00

Pratik Borhade added this to the Animation & Rigging project 2024-02-18 07:31:56 +01:00

Pratik Borhade referenced this pull request

2024-02-18 07:32:25 +01:00

Make channel selection consistent with outliiner #108448

Pratik Borhade added 1 commit 2024-02-19 12:54:05 +01:00

2a2f99f6cf Include remaing channel types

Pratik Borhade changed title from ~~WIP: Anim: Ctrl-shift channel selection.~~ to Anim: Ctrl-shift channel selection.

2024-02-19 12:54:25 +01:00

Pratik Borhade requested review from Sybren A. Stüvel 2024-02-19 12:57:14 +01:00

Pratik Borhade requested review from Christoph Lendenfeld 2024-02-19 12:57:15 +01:00

Pratik Borhade requested review from Christian Rauch 2024-02-19 12:57:15 +01:00

Pratik Borhade reviewed 2024-02-19 12:58:28 +01:00

scripts/presets/keyconfig/keymap_data/blender_default.py

						
				@ -3599,3 +3601,2 @@

				         {"properties": [("extend", True)]}),

				        ("anim.channels_click", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "ctrl": True},

				         {"properties": [("children_only", True)]}),

				        # ("anim.channels_click", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "ctrl": True},

Pratik Borhade commented

2024-02-19 12:58:28 +01:00

I've commented this keymap. Does anyone have alternative keymap for children_only ? 🙂

I've commented this keymap. Does anyone have alternative keymap for `children_only` ? 🙂

Christoph Lendenfeld commented

2024-02-20 10:11:36 +01:00

I suggest Ctrl + Alt + LEFTMOUSE
This is also unused in the outliner so we might be able to introduce it there as well. Selecting the hierarchy can be quite useful.
I noticed though in the channels this isn't even working properly. It only works when selecting the FCurve group, not e.g. the action or the object channel. But that's for a different time to fix

I suggest Ctrl + Alt + LEFTMOUSE This is also unused in the outliner so we might be able to introduce it there as well. Selecting the hierarchy can be quite useful. I noticed though in the channels this isn't even working properly. It only works when selecting the FCurve group, not e.g. the action or the object channel. But that's for a different time to fix

Thomas Dinges removed review request for Christian Rauch 2024-02-19 13:25:53 +01:00

Christoph Lendenfeld approved these changes 2024-02-20 10:46:15 +01:00

Christoph Lendenfeld left a comment

I am unsure about the decision to turn the selectmode into a bitflag.
This can now have conflicting flags which makes things confusing.

I suggest moving the SELECT_INVERT flag into a separate boolean so you can keep the mode a simple enum class.

enum class ChannelSelectMode {
  SELECT_SINGLE,
  SELECT_CHILDREN,
  SELECT_RANGE,
}

That way you can also use switch statements.

I'll let @dr.sybren be the judge on this though because this bloats the PR a bit.

I am unsure about the decision to turn the `selectmode` into a bitflag. This can now have conflicting flags which makes things confusing. I suggest moving the `SELECT_INVERT` flag into a separate boolean so you can keep the mode a simple enum class. ``` enum class ChannelSelectMode { SELECT_SINGLE, SELECT_CHILDREN, SELECT_RANGE, } ``` That way you can also use `switch` statements. I'll let @dr.sybren be the judge on this though because this bloats the PR a bit.

source/blender/editors/animation/anim_channels_edit.cc

						
				@ -3494,4 +3504,1 @@

				    ANIM_anim_channels_select_set(ac, ACHANNEL_SETFLAG_EXTEND_RANGE);

				    animchannel_select_range(ac, ale);

				  }

				  else if (selectmode == -1) {

Christoph Lendenfeld commented

2024-02-20 10:31:01 +01:00

This PR also changes the behavior of children_only because this will now only work if no selection flags are set. Because of selectmode |= -1 further down.

This PR also changes the behavior of `children_only` because this will now only work if no selection flags are set. Because of `selectmode |= -1` further down.

Christoph Lendenfeld requested changes 2024-02-20 10:46:33 +01:00

Christoph Lendenfeld left a comment

oops wrong button

Sybren A. Stüvel requested changes 2024-02-20 10:53:39 +01:00

Sybren A. Stüvel left a comment

Thanks for the patch Pratik!

I think that these changes clearly show the limitations of Blender's existing code. For extra clarity: I'm pointing out problems in the existing code here, not yours, Pratik. Some of the code is over a decade old, and it shows.

There's plenty of short selectmode (instead of using the eEditKeyframes_Select enum type). Furthermore, there is a "special" value that's used that is not part of the enum (-1) that may or may not be interpreted differently by different parts of the code.
The eEditKeyframes_Select enum items have values that suggest they can be used as bitflags, but still the code that's interpreting them doesn't use them as such. Instead, direct comparison (selectmode == SELECT_xxx) is used.

Since in this PR the combinatory bitflags of selectmode are used much more, I think this warrants a bit of a bigger refactor, where selectmode is actually passed as eEditKeyframes_Select. For that to work, you have to add ENUM_OPERATORS(eEditKeyframes_Select, SELECT_EXTEND_RANGE); right after the enum eEditKeyframes_Select declaration.

@PratikPB2123 would you be up for that?

Thanks for the patch Pratik! I think that these changes clearly show the limitations of Blender's existing code. For extra clarity: I'm pointing out problems in the existing code here, not yours, Pratik. Some of the code is over a decade old, and it shows. - There's plenty of `short selectmode` (instead of using the `eEditKeyframes_Select` enum type). Furthermore, there is a "special" value that's used that is _not_ part of the enum (`-1`) that may or may not be interpreted differently by different parts of the code. - The `eEditKeyframes_Select` enum items have values that suggest they can be used as bitflags, but still the code that's interpreting them doesn't use them as such. Instead, direct comparison (`selectmode == SELECT_xxx`) is used. Since in this PR the combinatory bitflags of `selectmode` are used much more, I think this warrants a bit of a bigger refactor, where `selectmode` is actually passed as `eEditKeyframes_Select`. For that to work, you have to add `ENUM_OPERATORS(eEditKeyframes_Select, SELECT_EXTEND_RANGE);` right after the `enum eEditKeyframes_Select` declaration. @PratikPB2123 would you be up for that?

source/blender/editors/animation/anim_channels_edit.cc

						
				@ -3917,2 +3930,3 @@

				  int notifierFlags = 0;

				  short selectmode;

				  short selectmode = 0;

				  bool select_mode_set = false;

Sybren A. Stüvel commented

2024-02-20 10:28:38 +01:00

This bool doesn't seem necessary. Just initialise short selectmode = SELECT_EXTEND_RANGE and then overwrite that value when necessary.

This `bool` doesn't seem necessary. Just initialise `short selectmode = SELECT_EXTEND_RANGE` and then overwrite that value when necessary.

source/blender/editors/animation/anim_channels_edit.cc

						
				@ -3938,3 +3954,3 @@

				    /* this is a bit of a special case for ActionGroups only...

				     * should it be removed or extended to all instead? */

				    selectmode = -1;

				    selectmode |= -1;

Sybren A. Stüvel commented

2024-02-20 10:28:01 +01:00

|= -1 is a bit of a dubious operation. -1 is all-ones in two's complement, which means that you're OR-ing with all-ones, which means that you're effectively setting the value to -1. So why not just set the value to that directly?

`|= -1` is a bit of a dubious operation. `-1` is all-ones in two's complement, which means that you're OR-ing with all-ones, which means that you're effectively setting the value to `-1`. So why not just set the value to that directly?