Fix #104054: Symmetrize visible ebones when nothing selected #107902

Open
Denys Hsu wants to merge 5 commits from cgtinker/blender:symmetrize-unselected into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Contributor

Symmetrize does nothing when there are no bones selected (Silently exits). Expected behaviour would be that when nothing is selected, the armature gets symmetrized as if all bones are selected.

When all bones are selected, all visible, selectable bones are symmetrized. This makes sense from the user perspective and also should be expected when nothing has been selected.

Options to solve this problem:

  • (De)select bones depending on the selected direction if mirrored bones are available.
  • Select all, if nothing has been selected ("reuse the deselection code").

I decided to close my previous PR as we decided that refactoring the method before applying the change would be more readable.

Symmetrize does nothing when there are no bones selected (Silently exits). Expected behaviour would be that when nothing is selected, the armature gets symmetrized as if all bones are selected. When all bones are selected, all visible, selectable bones are symmetrized. This makes sense from the user perspective and also should be expected when nothing has been selected. Options to solve this problem: - (De)select bones depending on the selected direction if mirrored bones are available. - Select all, if nothing has been selected ("reuse the deselection code"). I decided to close my previous PR as we decided that refactoring the method before applying the change would be more readable.
Denys Hsu added 1 commit 2023-05-13 01:13:23 +02:00
Iliya Katushenock added this to the Animation & Rigging project 2023-05-13 09:17:23 +02:00
Iliya Katushenock requested review from Sybren A. Stüvel 2023-05-13 09:18:15 +02:00
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2023-05-15 11:54:54 +02:00

@cgtinker does this PR need any adjusting now that #106487 landed?

@cgtinker does this PR need any adjusting now that #106487 landed?
Author
Contributor

@cgtinker does this PR need any adjusting now that #106487 landed?

build it on top of #106487, should be good to go :)

> @cgtinker does this PR need any adjusting now that #106487 landed? build it on top of #106487, should be good to go :)
Sybren A. Stüvel added 1 commit 2023-06-12 12:47:09 +02:00
Sybren A. Stüvel added 3 commits 2023-06-12 12:51:57 +02:00
d4ad038bd2 Rename `is_relevant_selection` to `is_selection_relevant`
This makes things a little easier to read, as it's about answering the
question "is the selection relevant?", and not "is this the relevant
selection?"

No functional changes.
b91978b572 Change assignment of `is_selection_relevant` so it can be `const`
This makes it clear that `is_selection_relevant` will not change value
after its initialisation.

As this PR was already lingering for a while (sorry about that) I took the liberty to make a few changes myself to speed things up. What I did was:

  • Rename is_relevant_selection to is_selection_relevant so that it reads more like a sentence, and
  • Change initialisation of is_selection_relevant so it can be const.

So no functional changes, just a little adjustment of the code.

As this PR was already lingering for a while (sorry about that) I took the liberty to make a few changes myself to speed things up. What I did was: - Rename `is_relevant_selection` to `is_selection_relevant` so that it reads more like a sentence, and - Change initialisation of `is_selection_relevant` so it can be `const`. So no functional changes, just a little adjustment of the code.
Sybren A. Stüvel approved these changes 2023-06-12 12:55:36 +02:00
Sybren A. Stüvel left a comment
Member

The functional changes seem good to me, so let's land this!

The functional changes seem good to me, so let's land this!
Sybren A. Stüvel requested review from Sybren A. Stüvel 2023-06-12 12:58:35 +02:00

On second thought, probably better to create a test build and have some riggers try out the new functionality.

@blender-bot package

On second thought, probably better to create a test build and have some riggers try out the new functionality. @blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107902) when ready.

It's nice to me so far.
As mentionned during the meeting, the wrench on the root of a keyed item is white, even though the exposed channels of this item are all greyed out as it has no modifier.

Also, if a modifier is muted on a transform channel, the item's wrench, the main one, is greyed out, that's great.
01.jpg

But if I manualy re-enable all modifiers at the transform channel level, the item's wrench is still greyed out.
02.jpg
you have to click th emain wrench to update it.

Hope it makes sense

It's nice to me so far. As mentionned during the meeting, the wrench on the root of a keyed item is white, even though the exposed channels of this item are all greyed out as it has no modifier. Also, if a modifier is muted on a transform channel, the item's wrench, the main one, is greyed out, that's great. ![01.jpg](/attachments/9de68a7b-f19e-49b1-8f22-9c35c3d9e601) But if I manualy re-enable all modifiers at the transform channel level, the item's wrench is still greyed out. ![02.jpg](/attachments/3720eece-5be8-4e6a-a06b-cab10b17838e) you have to click th emain wrench to update it. Hope it makes sense
28 KiB
30 KiB

@P2design I think you meant to reply to #106214 ?

@P2design I think you meant to reply to #106214 ?
Member

I think it's worth noting that Blender is very inconsistent when it comes to how operators that need a selection behave when they don't have a selection:

  • Symmetrize, Delete, Shade Smooth, presumably almost every operator: Fail silently (pretty sad)
  • Can't find any example other than my own add-ons: Operator is grayed out until something is selected (I like this)
  • Assign Collection: Fail loudly, throws an error that this operator NEEDS a selection (I like this a lot!)

But note that I also can't find any example of an operator that will fall back to operating on visible or some global dataset when there's no selection. So I think this patch is introducing another behaviour and exacerbates this inconsistency of Blender.

I would propose to throw an error instead, forcing the user to make a selection.

And I wish all other Blender operators behaved the same. (And I should start doing the same in my add-ons, maybe!)

I think it's worth noting that Blender is very inconsistent when it comes to how operators that need a selection behave when they don't have a selection: - Symmetrize, Delete, Shade Smooth, presumably almost every operator: Fail silently (pretty sad) - Can't find any example other than my own add-ons: Operator is grayed out until something is selected (I like this) - Assign Collection: Fail loudly, throws an error that this operator NEEDS a selection (I like this a lot!) But note that I also can't find any example of an operator that will fall back to operating on visible or some global dataset when there's no selection. So I think this patch is introducing another behaviour and exacerbates this inconsistency of Blender. I would propose to throw an error instead, forcing the user to make a selection. And I wish all other Blender operators behaved the same. (And I should start doing the same in my add-ons, maybe!)

Hi @dr.sybren , yes.
Sorry for the confusion ^_^'

Hi @dr.sybren , yes. Sorry for the confusion ^_^'

I think it's worth noting that Blender is very inconsistent when it comes to how operators that need a selection behave when they don't have a selection:

  • Symmetrize, Delete, Shade Smooth, presumably almost every operator: Fail silently (pretty sad)

This is indeed what I wanted to address.

  • Can't find any example other than my own add-ons: Operator is grayed out until something is selected (I like this)
  • Assign Collection: Fail loudly, throws an error that this operator NEEDS a selection (I like this a lot!)

I wish there was somewhat of a middle ground, where a menu could behave as the first option (grey out + explanation of WHY in the tooltip) and accessing the operator via a hotkey would use the 2nd option (because otherwise there is no way to show the WHY). That's not a change that this PR can introduce though.

But note that I also can't find any example of an operator that will fall back to operating on visible or some global dataset when there's no selection. So I think this patch is introducing another behaviour and exacerbates this inconsistency of Blender.

I would propose to throw an error instead, forcing the user to make a selection.

👍 I agree, consistency is important.

> I think it's worth noting that Blender is very inconsistent when it comes to how operators that need a selection behave when they don't have a selection: > - Symmetrize, Delete, Shade Smooth, presumably almost every operator: Fail silently (pretty sad) This is indeed what I wanted to address. > - Can't find any example other than my own add-ons: Operator is grayed out until something is selected (I like this) > - Assign Collection: Fail loudly, throws an error that this operator NEEDS a selection (I like this a lot!) I wish there was somewhat of a middle ground, where a menu could behave as the first option (grey out + explanation of WHY in the tooltip) and accessing the operator via a hotkey would use the 2nd option (because otherwise there is no way to show the WHY). That's not a change that this PR can introduce though. > But note that I also can't find any example of an operator that will fall back to operating on visible or some global dataset when there's no selection. So I think this patch is introducing another behaviour and exacerbates this inconsistency of Blender. > > I would propose to throw an error instead, forcing the user to make a selection. :+1: I agree, consistency is important.
Author
Contributor

I think it's worth noting that Blender is very inconsistent when it comes to how operators that need a selection behave when they don't have a selection:

  • Symmetrize, Delete, Shade Smooth, presumably almost every operator: Fail silently (pretty sad)

This is indeed what I wanted to address.

  • Can't find any example other than my own add-ons: Operator is grayed out until something is selected (I like this)
  • Assign Collection: Fail loudly, throws an error that this operator NEEDS a selection (I like this a lot!)

I wish there was somewhat of a middle ground, where a menu could behave as the first option (grey out + explanation of WHY in the tooltip) and accessing the operator via a hotkey would use the 2nd option (because otherwise there is no way to show the WHY). That's not a change that this PR can introduce though.

But note that I also can't find any example of an operator that will fall back to operating on visible or some global dataset when there's no selection. So I think this patch is introducing another behaviour and exacerbates this inconsistency of Blender.

I would propose to throw an error instead, forcing the user to make a selection.

👍 I agree, consistency is important.

Based on the meeting nodes (22.06) and the comment I guess the PR should be closed? (To keep operator behaviour consistent)

> > I think it's worth noting that Blender is very inconsistent when it comes to how operators that need a selection behave when they don't have a selection: > > - Symmetrize, Delete, Shade Smooth, presumably almost every operator: Fail silently (pretty sad) > > This is indeed what I wanted to address. > > > - Can't find any example other than my own add-ons: Operator is grayed out until something is selected (I like this) > > - Assign Collection: Fail loudly, throws an error that this operator NEEDS a selection (I like this a lot!) > > I wish there was somewhat of a middle ground, where a menu could behave as the first option (grey out + explanation of WHY in the tooltip) and accessing the operator via a hotkey would use the 2nd option (because otherwise there is no way to show the WHY). That's not a change that this PR can introduce though. > > > But note that I also can't find any example of an operator that will fall back to operating on visible or some global dataset when there's no selection. So I think this patch is introducing another behaviour and exacerbates this inconsistency of Blender. > > > > I would propose to throw an error instead, forcing the user to make a selection. > > :+1: I agree, consistency is important. > Based on the meeting nodes (22.06) and the comment I guess the PR should be closed? (To keep operator behaviour consistent)
Member

I think final decision lies with Sybren, but if you ask me, yes, I would either leave the operator the way it is, or add graying out / error throwing behaviours to improve the situation in a different way.

I think final decision lies with Sybren, but if you ask me, yes, I would either leave the operator the way it is, or add graying out / error throwing behaviours to improve the situation in a different way.

Since this whole thing got started with the operator silently doing nothing, I think leaving the way it is is not an option. Disabling the operator & having a clear explanation as to why in the tooltip would be good, I think. To me that would be preferred over appearing functional but then still causing an error when you try to use it.

Since this whole thing got started with the operator silently doing nothing, I think leaving the way it is is not an option. Disabling the operator & having a clear explanation as to why in the tooltip would be good, I think. To me that would be preferred over appearing functional but then still causing an error when you try to use it.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
This pull request has changes conflicting with the target branch.
  • source/blender/editors/armature/armature_add.c

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u symmetrize-unselected:cgtinker-symmetrize-unselected
git checkout cgtinker-symmetrize-unselected
Sign in to join this conversation.
No reviewers
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
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
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
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 Assignees
5 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#107902
No description provided.