Details

Reviewers

Sebastian Parborg (zeddb)
Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Commits

rBde8e13036ba4: Armature Make/Clear Parent: Grey out options that don't do anything

Summary

In armature edit mode, the Make/Clear Parent operators don't do anything in various cases, but only one of these cases was previously indicated, and it was indicated by hiding the option completely instead of graying it out.

Clear Parent (Alt+P):

"Clear Parent" option always showed up, even when none of the selected bones had a parent.
"Disconnect Bone" option always showed up, even when use_connected on all selected bones was already false.

Make Parent (Ctrl+P):

"Keep Offset" option didn't show up when all selected bones' parent was already the active bone. This was correct, and this patch tries to make all behaviours consistent with this.
"Connected" option always showed up, even when all selected bones' parent was already the active bone, and they all had use_connect set to True.

With this patch all options show up all the time, but in cases where they would do nothing, they will be grayed out.

Diff Detail

Repository

rB Blender

Branch

make_clear_parent (branched from master)

Build Status

Buildable 5428
Build 5428: arc lint + arc unit

Event Timeline

Demeter Dzadik (Mets) created this revision.Oct 20 2019, 12:25 AM

no changes, just using Arcanist instead of manual upload, to see the if the "Context not available." thing on phabricator still happens.

(Edit: It doesn't! Why?)

Harbormaster completed remote builds in B5424: Diff 19129.Oct 20 2019, 12:32 AM

Demeter Dzadik (Mets) edited reviewers, added: Sybren A. Stüvel (sybren); removed: Sergey Sharybin (sergey).Oct 20 2019, 12:52 AM

flip Set Parent order for consistency (so Connected/Disconnect is always the 2nd option)

Harbormaster completed remote builds in B5425: Diff 19130.Oct 20 2019, 1:00 AM

No longer handle case when use_connect=True but parent=NULL. With D6101 that case should no longer be possible.

Harbormaster completed remote builds in B5428: Diff 19133.Oct 20 2019, 1:43 AM

Ideally the options should be grayed out instead of hidden, but I can't figure out if/how that would be possible.

Edit: I think this would be possible only if these 2 operators with 2 types each would be split up into 4 separate operators?

In D6100#140508, @Demeter Dzadik (Mets) wrote:

Ideally the options should be grayed out instead of hidden, but I can't figure out if/how that would be possible.

I'm not too familiar with the UI code, but I think it's possible by adding a row, and then adding the menu item to that row. You can then set the row to disabled. Check out other uses of uiLayoutRow() and uiLayoutSetEnabled() in the code for examples.

Use uiLayoutRow() and grey out items instead of hiding them. And it only took 4 short months. How time flies!

Demeter Dzadik (Mets) retitled this revision from Armature Make/Clear Parent: Only display valid options to Armature Make/Clear Parent: Grey out options that don't do anything.Jun 29 2020, 10:44 PM

Demeter Dzadik (Mets) edited the summary of this revision. (Show Details)

Sebastian Parborg (zeddb) added a project: Animation & Rigging.Oct 13 2020, 2:33 PM

Sebastian Parborg (zeddb) added a reviewer: Animation & Rigging.

The patch works well, just some minor notes about the code.

source/blender/editors/armature/armature_relations.c
1011–1012	The auto-formatter will cause line wrapping. Better to put the comment above the variable declaration. Really nice to have those comments!
1016	`actbone` is no longer used and can be removed.
1018–1025	Nesting can be heavily reduced here, by flipping conditions and `continue`ing early. if (!EBONE_EDITABLE(ebone) \|\| !(ebone->flag & BONE_SELECTED)) { continue; } if (ebone->parent == NULL) { continue; } enable_clear = true; if (ebone->flag & BONE_CONNECTED) { enable_disconnect = true; break; } And yes, this is inconsistent with the above function that this was copied from -- I'm stricter for new code than for modifications on already-existing code ;-)