Allow select range in animation editor #104565
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104565
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "PratikPB2123/blender:T103855-anim-select-extend"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This patch will add support to select all channels between active channel
and last-clicked channel without deselecting previous selection.
Shift
key isnow assigned for the range selection and
ctrl
key to extend the selection. Thischanges will make multi-selection similar as outliner tree-elements. New function is
created
animchannel_select_range
to handle the range selection of channels.Old Differential revision: https://archive.blender.org/developer/D17079
One thing I noticed in the code
But I like the functionality :)
@ -3192,0 +3220,4 @@
if (cursor->flag & FCURVE_ACTIVE) {
cursor->flag |= FCURVE_SELECTED;
return;
This early return statement would mean that
ANIM_animdata_freelist(&anim_data);
never gets called.It's probably better to use
break
insteadDone, replaced it with
break
looks good to me now :)
Please edit the description to explain what this means in practice. "similar to outliner" is a bit too vague for me.
It's a nice patch, but can do with a few clarifications.
@ -3192,0 +3194,4 @@
ListBase anim_data = anim_channels_for_selection(ac);
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
if (ale->type != ANIMTYPE_FCURVE) {
What's the reason to exclude non-FCurve channels here (like GreasePencil)?
@ -3192,0 +3198,4 @@
continue;
}
FCurve *fcu = (FCurve *)ale->data;
This should probably be extracted into a separate function, like
animchannel_select(bAnimListElem *channel)
, that can then handle the per-channel-type specification of how selection is supposed to work.Instead we could use
anim_channels_select_set
function to reset the flag as done in this diff: (check below messages)What do you think?
Please don't rely on external websites for code reviews. If you want to propose an alternative approach, either use a separate PR or just paste the diff in a comment here.
Sure. I thought that would make conversation less readable :/
@ -3192,0 +3205,4 @@
ANIM_animdata_freelist(&anim_data);
}
static void animchannel_select_range(bAnimContext *ac, FCurve *cursor)
This should get a comment to explain what it does, as it's not directly clear which range it selects.
@ -3192,0 +3208,4 @@
static void animchannel_select_range(bAnimContext *ac, FCurve *cursor)
{
ListBase anim_data = anim_channels_for_selection(ac);
bool selected = false;
I think
selected
is a bit of a confusing name. The fact that something was selected or not is secondary; this variable indicates whether the loop is inside the to-be-selected range.in_selection_range
would be better.@ -3192,0 +3212,4 @@
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
if (ale->type != ANIMTYPE_FCURVE) {
Same question as above: why only operate on FCurves?
@ -3192,0 +3218,4 @@
FCurve *fcu = (FCurve *)ale->data;
if (cursor->flag & FCURVE_ACTIVE) {
This condition will not change within the for-loop, so there is no need to keep checking it here. As a matter of fact, you can even do this before the call to
anim_channels_for_selection(ac)
and then immediately return.@ -3526,6 +3583,9 @@ static void ANIM_OT_channels_click(wmOperatorType *ot)
prop = RNA_def_boolean(ot->srna, "extend", false, "Extend Select", "");
RNA_def_property_flag(prop, PROP_SKIP_SAVE);
prop = RNA_def_boolean(ot->srna, "extend_range", false, "Extend Range", "");
This should get a tooltip, to explain that it selects from the active channel to the current/clicked/howeveryouwannacallit channel.
Hi @dr.sybren , thanks for the review. I'll update the patch in a day or two.
I had asked about this in design task: #103855-comment
Will include other channel types too. Which other channel types are expected to do range selection?
I think this should work for any channel type.
@dr.sybren , do you expect use of switch case for the selection logic of individual channel types in
animchannel_select_range
?Yes, as that's more readable than a chain of
if
/else if
/else
.@dr.sybren hi, did changes to PR description and updated the code too.
Also included the selection of gpencil channels.
BTW, some channels don't have
active
flags (example: mask, shape-key). Supporting range selection of these channels will be different than the Fcurve/Gpencil channel types.I can't seem to get this PR to work. I select a channel, then shift-click on another channel, but it just keeps those two selected. There's no range select. I tested in the dopesheet and the graph editor, with F-Curve channels.
I can't seem to get this PR to work. I select a channel, then shift-click on another channel, but it just keeps those two selected. There's no range select. I tested in the dopesheet and the graph editor, with F-Curve channels.Never mind, that was an issue on my end.
Just some minor remarks on the code, the rest looks good!
@ -3205,0 +3273,4 @@
else if (selectmode == SELECT_EXTEND_RANGE) {
animchannel_clear_selection(ac);
/* When active channel is being clicked again for range selection, only select the
* clicked/active channel. Otherwise call range selection function. */
Is there any reason to have a special case for this? I think it would be better to have
animchannel_select_range()
simply work well when the start and end of the range are the same channel.AFAICT this check is needed. Suppose only one channel is selected and then we
shift-select
the same channel,select_range
will select all channels below to that.To avoid this, I had added this condition in
animchannel_select_range
(now moved theif
condition here)@ -3205,0 +3274,4 @@
animchannel_clear_selection(ac);
/* When active channel is being clicked again for range selection, only select the
* clicked/active channel. Otherwise call range selection function. */
if ((fcu->flag & FCURVE_ACTIVE) == 0) {
If this code needs to stick around, flip the condition, and swap the
if
andelse
blocks. That way you can doThis avoids the extra negation of 'is this FCurve active-flag NOT set?' in combination with an
else
.@ -3280,0 +3361,4 @@
animchannel_clear_selection(ac);
/* When active channel is being clicked again for range selection, only select the
* clicked/active channel. Otherwise call range selection function. */
if ((gpl->flag & FCURVE_ACTIVE) == 0) {
Same questions/remarks as above.
@ -3285,3 +3376,3 @@
/* change active layer, if this is selected (since we must always have an active layer) */
if (gpl->flag & GP_LAYER_SELECT) {
if ((gpl->flag & GP_LAYER_SELECT) && (selectmode != SELECT_EXTEND_RANGE)) {
Add the same note here as above, that
SELECT_EXTEND_RANGE
is excluded here to give the same behaviour as the Outliner.@ -3193,0 +3224,4 @@
case ANIMTYPE_FCURVE:
case ANIMTYPE_NLACURVE: {
FCurve *fcu = (FCurve *)ale->data;
FCurve *cursor = (FCurve *)cursor_elem->data;
There is no guarantee that
cursor_elem
is actually anFCurve
. In the Dopesheet you can have a mixture of different channel types. Same for the cast below --ale->type
dose not say anything aboutcursor_elem
.@ -3193,0 +3236,4 @@
if (in_selection_range) {
fcu->flag |= FCURVE_SELECTED;
}
} break;
No
break
outside the braces, see the style guide.@ -3205,0 +3273,4 @@
else if (selectmode == SELECT_EXTEND_RANGE) {
animchannel_clear_selection(ac);
/* When active channel is being clicked again for range selection, only select the
* clicked/active channel. Otherwise call range selection function. */
This behaviour should be part of
animchannel_select_range()
. Otherwise the caller has to know about this particular behaviour, which is basically stems from an implementation detail ofanimchannel_select_range()
. It creates strong coupling, which is undesired.@ -3193,0 +3224,4 @@
case ANIMTYPE_FCURVE:
case ANIMTYPE_NLACURVE: {
FCurve *fcu = (FCurve *)ale->data;
FCurve *cursor = (FCurve *)cursor_elem->data;
As far as I understand,
cursor_elem
will always be of typeFcurve
.We're passing
cursor_elem/AnimListElem
toanimchannel_select_range
from the selection function of individual channel type (for Fcurves:click_select_channel_fcurve
). In that functionAnimListElem
is Fcurve.I hope my explanation is clear.
@ -3205,0 +3273,4 @@
else if (selectmode == SELECT_EXTEND_RANGE) {
animchannel_clear_selection(ac);
/* When active channel is being clicked again for range selection, only select the
* clicked/active channel. Otherwise call range selection function. */
This change was part of
animchannel_select_range
but you recommended to move it outside :)To take the discussion into the main conversation:
It's not. A few lines further down you have:
so there it is assumed to be of type
bGPDlayer *
.My point is that you make this decision based on
ale->type
and notcursor_elem->type
.Try adding
BLI_assert(ale->type == cursor_elem->type);
in the loop, and test on the attached file. Click on theX Location
channel, then shift-click on theFills
channel. This should select the entire range, which combines FCurves and Grease Pencil channels.Something like this could avoid the need to check for the 'clicked on the active element' before calling
animchannel_select_range()
:@ -3193,0 +3221,4 @@
bool in_selection_range = false;
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
bool is_cursor_elem = (ale->data == cursor_elem->data);
This can be
const
Thanks for the alternative solution. Tested and this change works well
I've been digging some more into the animation channel API, and I think the code in this PR can be simplified if we use that instead. Not as far as I would like, because that API also has its shortcomings, but at least we can improve things :)
The select-range loop can be written like this:
That way the whole "select the channel" code is using
ANIM_channel_setting_set()
, and theswitch
is only used to get the active channel. Getting the active channel isn't possible with a simple API call (yet) so that's why I left it in the switch.Thanks, this solution is more clean and works good.
But it is also selecting anim-elements of type group, object, etc. that lies within the range. Is this expected? I mean if someone wants to select and then delete channels from different groups with "range select", they will not be able to delete particular channels, they might end up with all channels deleted from the group.
Either we can keep the old logic
Or we'd need to exclude group/object elements from being selected with new logic.
That's a good point, and thanks for the clear demo videos. I'll discuss this in today's module meeting.
If the decision is to keep the old behaviour, I think it's nicer to explicitly test for this, and then still use the generic API to actually select things.
Hi, any updates on this?
Thanks for the poke. Yes, this was discussed in last week's module meeting.
The consensus was:
Is this something you could implement?
Sure, will do.
Thanks for the changes, the FCurve channel selection works fine.
I discovered two issues though.
When selecting groups, e.g. this
it also selects the FCurve channels inbetween. The idea was that the selection would stay on the level it is on and not go down or up.
Second, when you have two objects with animation (turn off "only show selected") it will also select all channels from the other object. In fact it seems to just not work properly with channel selection that are not bones
see the described issues in the previous comment
ok, so in above case, only selection of groups within range is expected? I was not in the meeting so I misinterpreted Sybren's message (I consider to include all channels+groups when group is shift clicked). In short, which elements to select should depend on the shift-clicked element, right?
I've not understood this problem really. A short video might help.
Yeah that's the idea
So it took me a while to replicate this on second go. Quite obscure.
But video is there
Is this an issue with this PR? Just asking, because there are already quite a few issues with selection syncing between channels, and if it's not specific to this PR, I'd rather land this one and solve the rest some other time ;-)
Not sure, never encountered this issue.
I found one problem ( [i] ) while trying to replicate the above issue. Included the fix for it.
[i] calling range selection when no active element is present
I cannot reproduce the previous issue anymore, so from my side this is good to go
oops ctrl-enter makes a comment
officially green ticked from me now
Some minor things.
@ -215,3 +215,3 @@
}
static void select_pchan_for_action_group(bAnimContext *ac, bActionGroup *agrp, bAnimListElem *ale)
static void select_pchan_for_action_group(bAnimContext *ac, bActionGroup *agrp, bAnimListElem *ale, const bool change_active)
Document what the
change_active
parameter does. Booleans are notoriously tricky to understand.@ -3027,6 +3028,224 @@ static int click_select_channel_scene(bAnimListElem *ale,
return (ND_ANIMCHAN | NA_SELECTED);
}
/* Before selecting the range, check if any active element is present. */
Document what a function does, not when it should be called. The latter can be included, but should be secondary, and not the first thing documented.
"check" → "return whether". In #104949 the word "check" was used as "ensure", but here it seems to have another meaning.
Probably a better function name would be
animchannel_has_active_of_type
.@ -3028,2 +3029,4 @@
}
/* Before selecting the range, check if any active element is present. */
static bool animchannel_active_check(bAnimContext *ac, eAnim_ChannelType type)
const eAnim_ChannelType type
@ -3030,0 +3085,4 @@
is_active_found |= gpl->flag & GP_LAYER_ACTIVE;
break;
}
default:
Remove the
default
clause (also in the otherswitch
es) so that the compiler can warn if there are any enum elements that don't have a correspondingcase
.@ -3030,0 +3088,4 @@
default:
break;
}
}
Add this here:
as once the active element is found, the loop can stop.
@ -3030,0 +3153,4 @@
ANIM_animdata_freelist(&anim_data);
}
/* Select channels that lies between active channel and last clicked channel. */
I think the "last clicked channel" is actually the
cursor_elem
parameter? If so, don't reference UI interaction here, as it's irrelevant to the functionality of this function how thatcursor_elem
was passed.As an illustration of possible use it's fine to mention this, but then it should be in a second sentence and not the primary definition of what the code does.
@ -3030,0 +3161,4 @@
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
bool is_active_elem = false;
bool is_selection_allowed = ale->type == cursor_elem->type;
const
@ -3030,0 +3162,4 @@
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
bool is_active_elem = false;
bool is_selection_allowed = ale->type == cursor_elem->type;
const bool is_cursor_elem = (ale->data == cursor_elem->data);
Consistency: either use parentheses aroung the
A == B
expressions, or don't. But don't use different styles for neighbouring lines.@ -3030,0 +3169,4 @@
continue;
}
switch (ale->type) {
This switch is a copy of the logic above in the
animchannel_active_check()
function. Both basically ask 'isale
the active one?'Move the duplicated code into a function of its own, and use that from both places. Since it's so generic, it should probably be something like
bool ANIM_is_active_channel(bAnimListElem *ale)
and be declared inED_anim_api.h
.@ -357,7 +357,7 @@ void ED_pose_bone_select_tag_update(struct Object *ob);
/**
* Utility method for changing the selection status of a bone.
*/
Document what
change_active
means.@ -358,3 +358,3 @@
* Utility method for changing the selection status of a bone.
*/
void ED_pose_bone_select(struct Object *ob, struct bPoseChannel *pchan, bool select);
void ED_pose_bone_select(struct Object *ob, struct bPoseChannel *pchan, bool select, const bool change_active);
Remove
const
, it has no meaning in declarations of pass-by-value parameters.@ -217,1 +217,3 @@
static void select_pchan_for_action_group(bAnimContext *ac, bActionGroup *agrp, bAnimListElem *ale)
bool ANIM_is_active_channel(bAnimListElem *ale)
{
bool is_active_found = false;
You don't need this
bool
variable. Since it's only set once, never changes, and then is used to return a value, you can replace all theis_active_found = ...
withreturn ...
.@ -218,0 +263,4 @@
is_active_found = gpl->flag & GP_LAYER_ACTIVE;
break;
}
return is_active_found;
This line is never reached, and the function has no
return
statement for when there is no matchingcase
.Just end the function with
return false;
@ -3030,0 +3106,4 @@
return is_active_found;
}
static void animchannel_clear_selection(bAnimContext *ac)
This function is now very similar to
anim_channels_select_set()
, except that that one also handles some other types, for exampleANIMTYPE_SCENE
andANIMTYPE_NLATRACK
.If there is still need for this function, document well what the differences are, and why these differences are relevant.
Also the
switch
should have acase
for all items ineAnim_ChannelType
. Without that, a compiler can complain that there are missing items (which is a good mechanic to detect issues when a new enum item is added, so silencing the warning with adefault:
case is usually not recommended).Right. I mentioned this earlier about replacing
animchannel_clear_selection
with the existing function: #104565 (comment) :)make sense to add switch case for other channels when clearing selection flag.
But do you agree about replacing clear_selection function with
anim_channels_select_set
?Yeah, for sure! 👍
Thanks, replaced it with the existing function (with minor changes).
@ -3030,0 +3172,4 @@
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
bool is_active_elem = false;
const bool is_selection_allowed = (ale->type == cursor_elem->type);
const bool is_cursor_elem = (ale->data == cursor_elem->data);
This can be moved down below the
if (!is_selection_allowed)
block, because it's only used after that block.@ -3030,0 +3179,4 @@
continue;
}
is_active_elem = ANIM_is_active_channel(ale);
is_active_elem
isn't used before this point, so just declare it asconst bool is_active_elem = ANIM_is_active_channel(ale);
@ -110,3 +110,3 @@
if (select) {
pchan->bone->flag |= BONE_SELECTED;
arm->act_bone = pchan->bone;
arm->act_bone = change_active ? pchan->bone : arm->act_bone;
I think a no-op assignment when
change_active == false
is error-prone. Better to use a bit more code and write:Same below.
@ -218,0 +258,4 @@
bGPDlayer *gpl = (bGPDlayer *)ale->data;
return gpl->flag & GP_LAYER_ACTIVE;
}
return false;
This
return
is still in the wrong place...981cb0a3de
8e3bb1f6e0\o/ woohoo!
@ -218,0 +258,4 @@
bGPDlayer *gpl = (bGPDlayer *)ale->data;
return gpl->flag & GP_LAYER_ACTIVE;
}
/* These channel types do not have active flags. */
👍 now it's instantly clear that these were not forgotten.
Merged, thanks for reviewing :)
I believe this was already documented here.
Yes, @ChrisLend had updated the manual:
36fdeffef4