Page MenuHome

UI: Add shortcuts for modifier panels
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Mon, Jun 15, 7:20 PM.
Tokens
"Burninate" token, awarded by CAEL."Like" token, awarded by jorsh."Love" token, awarded by MantasKuginis."Love" token, awarded by weasel."Love" token, awarded by jfmatheu."Love" token, awarded by Geronimooo."Like" token, awarded by zeauro.

Details

Summary

This adds shortcuts to the modifier panels.

Each panel can now store an RNA pointer, and a new function is added to get the custom data for the panel under the cursor.
This custom data could be used to refactor the "List Panel System" to generalize it and integrate it further with RNA.

Other related improvements: D8037, D8040, D8042

I would add this functionality to constraints, grease pencil modifiers and grease pencil effects in separate commits.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Mon, Jun 15, 7:20 PM
Hans Goudey (HooglyBoogly) created this revision.
  • Use checkmark for apply modifier button

Hi, what about "Apply", I think that more frequent and important. must click twice in new drag menu. thanks

Yes. There should be more shortcuts.

Currently, if a user wants to apply all modifiers of stack at once ; he has to go into Object menu > Convert to sub-menu > Mesh from Curve/Meta/Surf/Text.
That is funny but that should not be the case at all.

A clean UI should exposed same things as things exposed by Modifiers Tools addon.
That is a community addon.
https://docs.blender.org/manual/en/2.83/addons/interface/modifier_tools.html

But it really only provides basic things to manage modifier stack that should be there by default.
An Apply All button, a Delete All button, a Viewport Vis button to disable/enable all modifiers in viewport and a Toggle Stack button to close it entirely or open it entirely.

You may not be a big fan of buttons. Those features could be added to menu and have shortcuts.
But there should be present by default and discoverable without the need to enable an addon.

Generally, +1! As discussed before, I agree it would be nice to have more utility operators like "Apply All", "Move to First", "Move Up", etc. They can go into the new menu with the down arrow (EDIT: oh, there's D8040 already :) ), although we should probably also consider adding these things to the context menu.


For the implementation, for newer code we usually let operators act on the UI context. So they check which button is "active", i.e. under the cursor. See usages of UI_context_active_but_get() or UI_context_active_but_prop_get() for example. We could do the same for panels: OBJECT_OT_modifier_remove would use the UI context if no modifier name or CTX pointer was set, checking if the hovered panel is a modifier panel.

The benefit of this is that the shortcut becomes configurable. Users can also disable it if it gets in the way for whatever reason. And I think you don't need the UI_ITEM_O_HAS_SHORTCUT flag then (or maybe you do...), it should detect that automatically.

I'm not against this current solution though, it seems quite fine too.

The only issue really is that it can be easy to accidentally duplicate modifiers if you try to add drivers with the D key. Luckily using D over widgets swallows the input so it doesn't both add a driver *and* duplicate, but if you miss the widget, it will then duplicate. Similar issue for the shortcut to reset values. IMO, the X key is too easy to accidentally hit without any warning, which is why it pops up a confirmation popup in the viewport. For the Delete key we don't do that, because it's sufficiently out of the way to accidentally hit.

A solution to this could be to use the Delete key for deleting immediately only, and adding a popup for the X key, and using Shift-D to duplicate, both of which are sufficiently different from those other widget keys.

... OBJECT_OT_modifier_remove would use the UI context if no modifier name or CTX pointer was set, checking if the hovered panel is a modifier panel.

I'm not sure that fits so well here. That seems like tying this specific implementation of the UI a bit too strongly with a general operator. I saw most uses of the functions you mentioned in interface code.
For one, the code to check if the mouse is in a panel exists, but it's local to the panel code. And the check for a "modifier panel" would have to be the panel context string and the INSTANCED flag. It just seems a bit hacky.

Also, if we added a shortcut to the whole operator and it displayed automatically, it would display everywhere it was added, not just here in the properties region, right?

If you felt strongly I'm happy to pursue it, but I'm personally not convinced it's a large enough improvement.

In addition to what I just said, it looks like panels don't store any data about whether they are below the mouse like buttons.

  • Add a report for deleting modifiers
  • Add ctrl-A to apply
This revision is now accepted and ready to land.Tue, Jun 16, 5:49 PM

I don't think it would be that hacky to make this UI context based. The operator could use the name to check if a panel is a modifier one. I also think it makes sense to have a Panel.runtime.customdata and reference the modifier from there. The operator could simply check if that pointer is in an object's modifier list. That would actually allow us to improve the panel-matching a bit.
I'm also starting to think it makes sense to have a ui_context_invoke() callback for operators. There are a few further occasions where I think this would be useful.
For the shortcut display issue I also see simple ways to solve this.

Anyway, like I said this approach is fine too. So I won't delay this further. Code looks fine.

  • Version with editable keymap with operators called directly (no custom panel handler)

@Julian Eisel (Severin) I was going to commit this but I wasn't satisfied with the idea that could be better. So this version does what you suggest, and indeed, it's not too hacky! Except for some stupid temporary stuff.

It works for delete so far but I've barely tested it. Just uploading a patch so I can work on this
a bit this weekend on my laptop.

Hans Goudey (HooglyBoogly) retitled this revision from UI: Add shortcuts for modifier panels to [WIP] UI: Add shortcuts for modifier panels.Fri, Jun 19, 11:59 PM
  • Improvements, working completely now
Hans Goudey (HooglyBoogly) retitled this revision from [WIP] UI: Add shortcuts for modifier panels to UI: Add shortcuts for modifier panels.Mon, Jun 22, 10:19 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
source/blender/editors/object/object_modifier.c
1068

I'm looking for a way to check if a RNA pointer is a modifier. But the pointers are "refined" when I create them. (RNA_Modifier becomes RNA_BooleanModifier).

Is there a way to avoid this refining so I can avoid a giant switch statement? If not I will manually set the type back to &RNA_Modifier when I create the pointers.

  • Remove unintentional formatting changes
  • Properly check if custom data is a modifier type
  • Solve custom data memory leak on Blender exit
Julian Eisel (Severin) requested changes to this revision.EditedMon, Jun 29, 2:30 PM

This crashes for me when trying to load factory settings.

Will test again once the crash is fixed. Code looks fine.

This revision now requires changes to proceed.Mon, Jun 29, 2:30 PM

This crashes for me when trying to load factory settings.

I can't seem to trigger this! Could you provide more detail?

  • Fix crash when reloading factory preferences

I just forgot to clear the panel listbase after freeing its contents..

  • Add reports for modifier delete and apply

Glad you looked into the other approach!
This is quite nice :)

This revision is now accepted and ready to land.Mon, Jun 29, 8:22 PM
This revision was automatically updated to reflect the committed changes.