Anim: bone collections, add 'solo' flag #117414

Manually merged
Sybren A. Stüvel merged 2 commits from dr.sybren/blender:anim/bone-collections-solo into main 2024-01-26 10:27:52 +01:00

Add the 'solo' flag to bone collections, effectively adding another layer of visibility controls.

Test Build

If there is any bone collection with this flag enabled, only this collection (and others with this flag enabled) will be visible.

In RNA, the following properties are exposed:

  • bone_collection.is_solo: writable property to manage the solo flag.
  • armature.is_solo_active: read-only property that is True when any bone collection has is_solo = True.
Solo OFF Solo ON
solo OFF solo ON

Question for reviewers:

  • What do you think of the GUI? I've used the already-existing SOLO_ON and SOLO_OFF icons. This makes the functionality consistent with the similar buttons in the animation editors, but I don't think the star is the best visualisation of the concept.
  • Are there any other places in the GUI where the "solo this" functionality should be exposed?
  • What shall we do with the 'Solo Visibility' operator? Shall we just keep that as-is, or remove it now that we have a true Solo flag?

Note that I've reused a previously-cleared armature flag, which is why some old versioning code also changed.

Add the 'solo' flag to bone collections, effectively adding another layer of visibility controls. **[Test Build](https://builder.blender.org/download/patch/PR117414)** If there is _any_ bone collection with this flag enabled, only this collection (and others with this flag enabled) will be visible. In RNA, the following properties are exposed: - `bone_collection.is_solo`: writable property to manage the solo flag. - `armature.is_solo_active`: read-only property that is `True` when any bone collection has `is_solo = True`. | Solo OFF | Solo ON | | - | - | | ![solo OFF](/attachments/80619eaa-2a19-413a-937a-86060a9c0ece) | ![solo ON](/attachments/a3fd42df-ba26-4771-8ed5-a05f9f6dc2d8) | -------------- Question for reviewers: - What do you think of the GUI? I've used the already-existing `SOLO_ON` and `SOLO_OFF` icons. This makes the functionality consistent with the similar buttons in the animation editors, but I don't think the star is the best visualisation of the concept. - Are there any other places in the GUI where the "solo this" functionality should be exposed? - What shall we do with the 'Solo Visibility' operator? Shall we just keep that as-is, or remove it now that we have a true Solo flag? Note that I've reused a previously-cleared armature flag, which is why some old versioning code also changed.
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-01-22 16:17:47 +01:00
Sybren A. Stüvel requested review from Demeter Dzadik 2024-01-22 16:17:59 +01:00
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-01-22 16:18:05 +01:00
Sybren A. Stüvel requested review from Christoph Lendenfeld 2024-01-22 16:20:15 +01:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR117414) when ready.
Sybren A. Stüvel added this to the 4.1 milestone 2024-01-22 16:22:50 +01:00
Member

Thanks so much for doing this (and so many other things) at my request!

What do you think of the GUI?

I agree it's good that it's consistent with other "Solo" concepts in Blender, even if all the stars are admittedly a bit heavy on the eyes.
A small nitpick though, it seems like on the "Solo Off" picture, the stars are grayed out. I don't think they should be? Graying out the eyes while Solo is on makes sense though.

Are there any other places in the GUI where the "solo this" functionality should be exposed?

I can't think of any!

What shall we do with the 'Solo Visibility' operator? Shall we just keep that as-is, or remove it now that we have a true Solo flag?

If people like this property-based Solo behaviour, then to me it makes sense to remove the old operator.

Thanks so much for doing this (and so many other things) at my request! > What do you think of the GUI? I agree it's good that it's consistent with other "Solo" concepts in Blender, even if all the stars are admittedly a bit heavy on the eyes. A small nitpick though, it seems like on the "Solo Off" picture, the stars are grayed out. I don't think they should be? Graying out the eyes while Solo is on makes sense though. > Are there any other places in the GUI where the "solo this" functionality should be exposed? I can't think of any! > What shall we do with the 'Solo Visibility' operator? Shall we just keep that as-is, or remove it now that we have a true Solo flag? If people like this property-based Solo behaviour, then to me it makes sense to remove the old operator.
First-time contributor

What do you think of the GUI? I've used the already-existing SOLO_ON and SOLO_OFF icons. This makes the functionality consistent with the similar buttons in the animation editors, but I don't think the star is the best visualisation of the concept.

There's been a long discussion about this same topic when trying to find a "solo" icon for the NLA. From what I've seen, apparently there isn't an industry standard icon for solo-ing. Certain video editor for effects uses a white circle (quite unintuitive, imo, since I didn't know what it did until being told about it). To me, a solo-ing icon would be composed of several shapes (3-4 circles, for example) which would be filled. And when "solo-ed", only one of those shapes would remain filled and the rest would turn empty.

Are there any other places in the GUI where the "solo this" functionality should be exposed?

VSE layers, outliner (I guess it would be like making an object local), modifiers (3d, but also VSE), nodes (maybe? It's kinda like previewing a single node's effect), NLA layers, curve channels (from the graph editor).

> What do you think of the GUI? I've used the already-existing SOLO_ON and SOLO_OFF icons. This makes the functionality consistent with the similar buttons in the animation editors, but I don't think the star is the best visualisation of the concept. There's been a long discussion about this same topic when trying to find a "solo" icon for the NLA. From what I've seen, apparently there isn't an industry standard icon for solo-ing. Certain video editor for effects uses a white circle (quite unintuitive, imo, since I didn't know what it did until being told about it). To me, a solo-ing icon would be composed of several shapes (3-4 circles, for example) which would be filled. And when "solo-ed", only one of those shapes would remain filled and the rest would turn empty. > Are there any other places in the GUI where the "solo this" functionality should be exposed? VSE layers, outliner (I guess it would be like making an object local), modifiers (3d, but also VSE), nodes (maybe? It's kinda like previewing a single node's effect), NLA layers, curve channels (from the graph editor).
Author
Member

There's been a long discussion about this same topic when trying to find a "solo" icon for the NLA. From what I've seen, apparently there isn't an industry standard icon for solo-ing. Certain video editor for effects uses a white circle (quite unintuitive, imo, since I didn't know what it did until being told about it). To me, a solo-ing icon would be composed of several shapes (3-4 circles, for example) which would be filled. And when "solo-ed", only one of those shapes would remain filled and the rest would turn empty.

@EosFoxx also made a design for the NLA solo buttons:

e1e40b72481a438395a397e53d38145b4f491085

Those are a huge improvement over the "favourite" star, but too specific to use for bone collections as well.

If we're going to change/add icons, it should be done in collaboration with the UI module. Unfortunately they are also busy, and we already have a bunch of PRs open.

Are there any other places in the GUI where the "solo this" functionality should be exposed?

VSE layers, outliner (I guess it would be like making an object local), modifiers (3d, but also VSE), nodes (maybe? It's kinda like previewing a single node's effect), NLA layers, curve channels (from the graph editor).

I wasn't thinking this broadly ;-) I meant whether the control over solo'ing bone collections should be available in other spots as well. To illustrate, assigning bones to collections can be done in the Armature properties panel, but also in the 3D viewport (M/Shift+M hotkeys).

> There's been a long discussion about this same topic when trying to find a "solo" icon for the NLA. From what I've seen, apparently there isn't an industry standard icon for solo-ing. Certain video editor for effects uses a white circle (quite unintuitive, imo, since I didn't know what it did until being told about it). To me, a solo-ing icon would be composed of several shapes (3-4 circles, for example) which would be filled. And when "solo-ed", only one of those shapes would remain filled and the rest would turn empty. @EosFoxx also [made a design for the NLA solo buttons](https://devtalk.blender.org/t/2023-06-13-animation-rigging-module-meeting/29855): ![e1e40b72481a438395a397e53d38145b4f491085](/attachments/08acc8fe-0d69-44bc-8197-cda853264c7e) Those are a huge improvement over the "favourite" star, but too specific to use for bone collections as well. If we're going to change/add icons, it should be done in collaboration with the UI module. Unfortunately they are also busy, and we already have a bunch of PRs open. > > Are there any other places in the GUI where the "solo this" functionality should be exposed? > > VSE layers, outliner (I guess it would be like making an object local), modifiers (3d, but also VSE), nodes (maybe? It's kinda like previewing a single node's effect), NLA layers, curve channels (from the graph editor). I wasn't thinking this broadly ;-) I meant whether the control over solo'ing bone collections should be available in other spots as well. To illustrate, assigning bones to collections can be done in the Armature properties panel, but also in the 3D viewport (M/Shift+M hotkeys).

As much as I dislike the Star icon, I think for consistency it's good to use it here. This would also mean there is more reason to replace it in the future.

As much as I dislike the Star icon, I think for consistency it's good to use it here. This would also mean there is more reason to replace it in the future.
First-time contributor

As much as I dislike the Star icon, I think for consistency it's good to use it here. This would also mean there is more reason to replace it in the future.

Yes, I agree on this.

@dr.sybren Ah, sorry, I misunderstood. My bad. I guess it'd be nice having it in the outliner, although I think it would be odd having a solo button for bone but not for anything else.

> As much as I dislike the Star icon, I think for consistency it's good to use it here. This would also mean there is more reason to replace it in the future. Yes, I agree on this. @dr.sybren Ah, sorry, I misunderstood. My bad. I guess it'd be nice having it in the outliner, although I think it would be odd having a solo button for bone but not for anything else.
Author
Member

Hmmm Gitea ate up my response to Demeter.

A small nitpick though, it seems like on the "Solo Off" picture, the stars are grayed out. I don't think they should be? Graying out the eyes while Solo is on makes sense though.

That's intentional, to make it easier to see that there is no "soloing" enabled anywhere. A greyed out eye just indicates "this setting doesn't matter", which can have two reasons:

  • Something is solo'ed
  • A parent bone collection is marked invisible, implicitly hiding its children.

If people like this property-based Solo behaviour, then to me it makes sense to remove the old operator.

👍

Hmmm Gitea ate up my response to Demeter. > A small nitpick though, it seems like on the "Solo Off" picture, the stars are grayed out. I don't think they should be? Graying out the eyes while Solo is on makes sense though. That's intentional, to make it easier to see that there is no "soloing" enabled anywhere. A greyed out eye just indicates "this setting doesn't matter", which can have two reasons: - Something is solo'ed - A parent bone collection is marked invisible, implicitly hiding its children. > If people like this property-based Solo behaviour, then to me it makes sense to remove the old operator. :+1:
Member

to make it easier to see that there is no "soloing" enabled anywhere.

Hmmm, that does makes sense, but grayed out usually means "you can't click this", so it's also a bit confusing. I don't really have a good solution that avoids that while also communicating whether soloing is currently active or not, hmm... But if we have to choose between soloing potentially not being indicated (if the solo'd collection is off-screen currently), vs. appearing like it's not available, I would still go for the former, so, no graying. What do you think?

> to make it easier to see that there is no "soloing" enabled anywhere. Hmmm, that does makes sense, but grayed out usually means "you can't click this", so it's also a bit confusing. I don't really have a good solution that avoids that while also communicating whether soloing is currently active or not, hmm... But if we have to choose between soloing potentially not being indicated (if the solo'd collection is off-screen currently), vs. appearing like it's not available, I would still go for the former, so, no graying. What do you think?
Author
Member

@Mets this is how it would look without the greying of the stars (so with just the greying of the eyes):

@Mets this is how it would look without the greying of the stars (so with just the greying of the eyes): <video src="/attachments/c0f1d96f-6dac-4823-bb27-32e0375fdb25" title="recording-2024-01-23_17.42.38.mp4" controls></video>

I prefer the non-grayed out stars.
To me, if they are grayed out it indicates that clicking it won't change anything.

I prefer the non-grayed out stars. To me, if they are grayed out it indicates that clicking it won't change anything.
Member

Yep, if people don't have strong feelings for the opposite, I wouldn't gray out the stars. There may not be a perfect indication of solo-ing being active, but I think that's not the end of the world; It only happens when the solo'd collection gets collapsed or scrolls away, and even then, grayed out eye icons of top level collections can still suggest that you have something solo'd. Hopefully this results in the least possible confusion.

Yep, if people don't have strong feelings for the opposite, I wouldn't gray out the stars. There may not be a perfect indication of solo-ing being active, but I think that's not the end of the world; It only happens when the solo'd collection gets collapsed or scrolls away, and even then, grayed out eye icons of top level collections can still suggest that you have something solo'd. Hopefully this results in the least possible confusion.
Author
Member

No prob! It's just the removal of a tiny bit of code. Will do that when I'm back in the office :)

No prob! It's just the removal of a tiny bit of code. Will do that when I'm back in the office :)
Sybren A. Stüvel reviewed 2024-01-23 20:27:08 +01:00
@ -246,0 +249,4 @@
/* Solo icon. */
{
uiLayout *solo_sub = uiLayoutRow(sub, true);
uiLayoutSetActive(solo_sub, is_solo_active);
Author
Member

Note for the reviewers: it's these two lines that'll be removed for the "stop greying out the stars" thing. The uiItemR call below will use sub instead of solo_sub.

Note for the reviewers: it's these two lines that'll be removed for the "stop greying out the stars" thing. The `uiItemR` call below will use `sub` instead of `solo_sub`.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel reviewed 2024-01-23 20:27:55 +01:00
@ -234,15 +234,27 @@ class BoneCollectionItem : public AbstractTreeViewItem {
uiItemL(sub, "", icon);
}
const bool is_solo_active = armature_.flag & ARM_BCOLL_SOLO_ACTIVE;
Author
Member

This will be moved into the /* Visibility eye icon. */ section, as it's only used there (after removal of The Greying of the Stars™)

This will be moved into the `/* Visibility eye icon. */` section, as it's only used there (after removal of The Greying of the Stars™)
dr.sybren marked this conversation as resolved
First-time contributor

Typically, DAWs (digital audio workstations) use an "S" for the solo button. It is always placed next to an "M" for mute, and is pretty much universal to my knowledge (except for Audacity, which uses the full text "Solo"). In Blender it has to make sense for armature layers, NLA tracks, possibly sequencer strips, etc. so finding an icon fitting all those contexts is not obvious. For these reasons I suggest aligning with the audio world.

armcolS.png

Typically, DAWs (digital audio workstations) use an "S" for the solo button. It is always placed next to an "M" for mute, and is pretty much universal to my knowledge (except for Audacity, which uses the full text "Solo"). In Blender it has to make sense for armature layers, NLA tracks, possibly sequencer strips, etc. so finding an icon fitting all those contexts is not obvious. For these reasons I suggest aligning with the audio world. ![armcolS.png](/attachments/f6175d53-24ab-43b5-88d4-42a6c904f45c)
Christoph Lendenfeld reviewed 2024-01-23 23:10:29 +01:00
Christoph Lendenfeld left a comment
Member

haven't tested it yet, just went through the code

@HadrienBrissaud we discussed this with the UI team a while ago and the reasoning against it was that this doesn't work across all languages. In some, solo doesn't have an s and in some the letter s doesn't exist. So the rule is to stay away from letters as icons

I think it's a good discussion to have but maybe for a different PR since this is more about adding the functionality of soloing bone layers

haven't tested it yet, just went through the code @HadrienBrissaud we discussed this with the UI team a while ago and the reasoning against it was that this doesn't work across all languages. In some, solo doesn't have an s and in some the letter s doesn't exist. So the rule is to stay away from letters as icons I think it's a good discussion to have but maybe for a different PR since this is more about adding the functionality of soloing bone layers
@ -795,0 +812,4 @@
void ANIM_armature_refresh_solo_active(bArmature *armature)
{
bool any_bcoll_solo = false;
for (const BoneCollection *other_bcoll : armature->collections_span()) {

I assume you split this function off from ANIM_armature_bonecoll_solo_set
Now there is only one BoneCollection so this can be called bcoll

I assume you split this function off from `ANIM_armature_bonecoll_solo_set` Now there is only one `BoneCollection` so this can be called `bcoll`
dr.sybren marked this conversation as resolved
@ -944,13 +990,13 @@ static bool any_bone_collection_visible(const ListBase /*BoneCollectionRef*/ *co
/* TODO: these two functions were originally implemented for armature layers, hence the armature
* parameters. These should be removed at some point. */

The armature parameter is now in use, so I think this comment needs updating

The armature parameter is now in use, so I think this comment needs updating
dr.sybren marked this conversation as resolved
Christoph Lendenfeld approved these changes 2024-01-25 15:57:16 +01:00
Christoph Lendenfeld left a comment
Member

works well in testing, couldn't find any issues.

The only thing that bothered me, which isn't part of this PR though, is that F2 doesn't rename the bone collection.

works well in testing, couldn't find any issues. The only thing that bothered me, which isn't part of this PR though, is that F2 doesn't rename the bone collection.
Nathan Vegdahl requested changes 2024-01-25 16:22:15 +01:00
Nathan Vegdahl left a comment
Member

Over-all looking good. Just one concern about how these changes affect the clarity of meaning of an existing function.

Over-all looking good. Just one concern about how these changes affect the clarity of meaning of an existing function.
@ -939,0 +979,4 @@
}
}
else {
if (bcoll->is_visible_effectively()) {
Member

Given the name of is_visible_effectively(), I would expect it to already account for whether things are soloed or not, rather than needing the branch on is_solo_active here.

I think either the name of the function should change, or it should be kept up-to-date to account for all things that affect the effective visibility of a collection.

Given the name of `is_visible_effectively()`, I would expect it to already account for whether things are soloed or not, rather than needing the branch on `is_solo_active` here. I think either the name of the function should change, or it should be kept up-to-date to account for all things that affect the effective visibility of a collection.
dr.sybren marked this conversation as resolved
Author
Member

I'll reply here, because I'm about to force-push, which will disconnect the notes from the sources.

Given the name of is_visible_effectively(), I would expect it to already account for whether things are soloed or not, rather than needing the branch on is_solo_active here.

I think either the name of the function should change, or it should be kept up-to-date to account for all things that affect the effective visibility of a collection.

I've split up this PR into two commits, and intend on landing it as such:

  1. Rename BoneCollection::is_visible_effectively to is_visible_with_ancestors. This should clarify its functionality.
  2. Add the 'solo' functionality, including a new function ANIM_armature_bonecoll_is_visible_effectively() that actually checks for the effective visibility (including solo flag). This is now also called in the RNA getter for the bcoll.is_visible_effectively property.
I'll reply here, because I'm about to force-push, which will disconnect the notes from the sources. > Given the name of `is_visible_effectively()`, I would expect it to already account for whether things are soloed or not, rather than needing the branch on `is_solo_active` here. > > I think either the name of the function should change, or it should be kept up-to-date to account for all things that affect the effective visibility of a collection. I've split up this PR into two commits, and intend on landing it as such: 1. Rename `BoneCollection::is_visible_effectively` to `is_visible_with_ancestors`. This should clarify its functionality. 2. Add the 'solo' functionality, including a new function `ANIM_armature_bonecoll_is_visible_effectively()` that *actually* checks for the effective visibility (including solo flag). This is now also called in the RNA getter for the `bcoll.is_visible_effectively` property.
Sybren A. Stüvel force-pushed anim/bone-collections-solo from e368473ff4 to 87619ca717 2024-01-25 17:14:36 +01:00 Compare
Nathan Vegdahl approved these changes 2024-01-25 17:20:53 +01:00
Nathan Vegdahl left a comment
Member

Looks good to me!

Looks good to me!
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel force-pushed anim/bone-collections-solo from 87619ca717 to e3e7381610 2024-01-26 10:21:58 +01:00 Compare
Sybren A. Stüvel manually merged commit a7f41fc938 into main 2024-01-26 10:27:52 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117414
No description provided.