Details

Reviewers

Maniphest Tasks

T89560: Insert key dropdown on keymap doesn't show keying sets anymore

Summary

It was not possible to assign a shortcut to menu items in the insert key-frame menu going back to version 2.7x.
Doing so would replace the current key that opens the insert keyframe menu
(I-key by default), instead of binding a key to insert a key-frame for the keying-set referenced by the menu item.

Now each menu item can be bound to a key or added to the "Quick Favorites" menu, directly inserting a key-frame for the corresponding keying-set.

Note that users must use the operator anim.keyframe_insert_by_name when setting up key-shortcuts as anim.keyframe_insert is only intended to launch the menu.

Keymap Editor:

When editing these key-map items in the key-map editor, the keying-set identifier must be used.
At the moment the key-map editor doesn't support showing a drop-down list.
The identifiers can be used from the tool-tip or the info editor.

Details:

Use ANIM_OT_keyframe_insert_by_name instead of ANIM_OT_keyframe_insert_menu for the insert keyframe popup menu to resolve the following issues binding keys to keying sets:

The index of the keying set isn't stable (adding/removing keying sets may change it).
Binding a key to items in the popup menu triggers a popup instead of inserting a key using the keying set from the menu item.

While support for using the current operator could be improved, it will still only work for built-in keying sets, so I'd prefer to use an operator that is intended for key-bindings.

Besides supporting binding keys to menu items there are no functional changes.

After looking into T89560, the user was attempting to set up a key-binding in a way which isn't properly supported, this is understandable that the user would attempt to do this given how the menu exposes the operators but happens not to work properly when binding keys.
This patch uses ANIM_OT_keyframe_insert_by_name to avoid users running into this problem.

NOTE: This could be supported a different way - by having key-map items optionally store the string value instead of the enum index, this is quite a big change and I don't think it would be used in many cases. Mentioning it since support for this would avoid manually expanding the enum in this case.

Diff Detail

Repository

rB Blender

Branch

TEMP-KEYFRAME-KEYMAP-UX (branched from master)

Build Status

Buildable 20938
Build 20938: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Mar 9 2022, 4:18 AM

Campbell Barton (campbellbarton) created this revision.

Harbormaster completed remote builds in B20938: Diff 49082.Mar 9 2022, 4:18 AM

Campbell Barton (campbellbarton) retitled this revision from Improve usability for binding keys to insert key-frame to Improve usability of insert key-frame with key-bindings .Mar 9 2022, 4:42 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) added a reviewer: Sybren A. Stüvel (sybren).

Campbell Barton (campbellbarton) added a project: Animation & Rigging.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) mentioned this in T89560: Insert key dropdown on keymap doesn't show keying sets anymore.Mar 9 2022, 4:46 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Mar 9 2022, 11:04 AM

Campbell Barton (campbellbarton) mentioned this in rB56dba4df3c7b: Revert "Fix T89560: Keymap editor no longer shows keying set dropdown".Mar 23 2022, 6:53 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Mar 28 2022, 10:52 AM

Good find, and I agree using the keying set name (instead of index) makes a lot of sense. Just some small notes about the code itself.

Does this change the UI in any way? Or is the same but just working as people would expect?

source/blender/editors/animation/keyframing.c
1983–1989	Let's not keep dead code around. If the code isn't working well, it should just be removed. I don't think this is a matter of preference -- one approach is unstable and will cause issues, the other will not, so let's remove the first.
1990–2016	This should either be its own function (so it can be appropriately named), or at least get some comment to mark this off as some "section" with an explanation what happens here.
2008	This comment doesn't make much sense to me. If there are no headers in this enum, there are no headers. This sentence acts as if they are both there and not there at the same time, which is only possible on quantum computers.

This revision now requires changes to proceed.Mar 28 2022, 12:09 PM

Add section comment for the enum menu.
Early exit (running exec) when the menu isn't shown.
Remove dead code.

Harbormaster completed remote builds in B21278: Diff 49772.Mar 28 2022, 12:57 PM

Campbell Barton (campbellbarton) marked 3 inline comments as done.Mar 28 2022, 12:59 PM

In D14289#387473, @Sybren A. Stüvel (sybren) wrote:

Good find, and I agree using the keying set name (instead of index) makes a lot of sense. Just some small notes about the code itself.

Does this change the UI in any way? Or is the same but just working as people would expect?

The UI looks the same, I noticed some poor behavior if the keying set doesn't exist (the message assumes it's the active keying set) but think that can be handled as a separate patch, e.g. P2858.

Update comment for negated 'if' check.

Harbormaster completed remote builds in B21279: Diff 49773.Mar 28 2022, 1:15 PM

Sybren A. Stüvel (sybren) added a task: T89560: Insert key dropdown on keymap doesn't show keying sets anymore.Mar 28 2022, 4:17 PM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Mar 29 2022, 12:50 PM

Campbell Barton (campbellbarton) retitled this revision from Improve usability of insert key-frame with key-bindings to Fix assigning key shortcuts in the insert keyframe menu.

Campbell Barton (campbellbarton) retitled this revision from Fix assigning key shortcuts in the insert keyframe menu to Fix setting key shortcuts in the insert keyframe menu items.Mar 29 2022, 12:54 PM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Mar 29 2022, 1:01 PM

Clarify/improve comments.

Harbormaster completed remote builds in B21313: Diff 49827.Mar 29 2022, 1:15 PM

Correct & merge outdated comment.

Harbormaster completed remote builds in B21314: Diff 49828.Mar 29 2022, 1:20 PM

Remove unintended change from my local master branch.

Harbormaster completed remote builds in B21315: Diff 49829.Mar 29 2022, 1:22 PM

In D14289#387502, @Campbell Barton (campbellbarton) wrote:

In D14289#387473, @Sybren A. Stüvel (sybren) wrote:

Does this change the UI in any way? Or is the same but just working as people would expect?

The UI looks the same

It does not look the same. Previously there was a drop-down list of keying sets in the keymap editor. Now animators have to know some string in order to use it.

Since the old behaviour of having a drop-down to choose from is gone, this doesn't fully solve T89560. It's also unclear from the user interface what string is actually expected here, so even I wouldn't know whether this requires some internal name or the UI label of the keying set (I'm guessing the latter). It would seem that the keymap editor is not suitable for this particular operator? If that is the case, this should be clearly described in the patch description (would have to be done for the release notes anyway), because it would mean animators have to walk a different path to get the behaviour they're used to. In the end it might be an even easier path, but if that's not described, it's not easier.

I tried to assign a key just by right-click in the I-menu and assigning a key, and that indeed does work just fine. It also shows it's using some internal name and not the UI label, which is good.
Such information, as well as some screenshots of the new UI, should be in the patch description, though.

In D14289#392447, @Sybren A. Stüvel (sybren) wrote:

In D14289#387502, @Campbell Barton (campbellbarton) wrote:

In D14289#387473, @Sybren A. Stüvel (sybren) wrote:

Does this change the UI in any way? Or is the same but just working as people would expect?

The UI looks the same

It does not look the same. Previously there was a drop-down list of keying sets in the keymap editor. Now animators have to know some string in order to use it.

I thought you were referring to the UI for the menu shown when pressing the I-key, the key-map editor is different.

Since the old behaviour of having a drop-down to choose from is gone, this doesn't fully solve T89560. It's also unclear from the user interface what string is actually expected here, so even I wouldn't know whether this requires some internal name or the UI label of the keying set (I'm guessing the latter). It would seem that the keymap editor is not suitable for this particular operator?

This is a general UI issue in the key-map editor, that there are many areas where string identifiers are used that don't show a useful list of available options.

I think this should be resolved in RNA (where string properties can define a way to show options, similar to what we have for bone names without the need to hard-code the collection in the UI).
This is worth supporting but it's out of scope for this patch (as it's quite a large RNA/UI change which should be discussed and reviewed separately).

If that is the case, this should be clearly described in the patch description (would have to be done for the release notes anyway), because it would mean animators have to walk a different path to get the behaviour they're used to. In the end it might be an even easier path, but if that's not described, it's not easier.

Good point, I was mainly considering users setting they shortcuts from the context menu. I'll update the patch to note how they can assign these shortcuts.

I tried to assign a key just by right-click in the I-menu and assigning a key, and that indeed does work just fine. It also shows it's using some internal name and not the UI label, which is good.
Such information, as well as some screenshots of the new UI, should be in the patch description, though.

No problem.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Apr 11 2022, 12:00 PM

Much better, thanks!

This revision is now accepted and ready to land.Apr 11 2022, 12:31 PM

Campbell Barton (campbellbarton) retitled this revision from Fix setting key shortcuts in the insert keyframe menu items to Fix setting key shortcuts for insert keyframe menu items.Apr 11 2022, 1:09 PM

Campbell Barton (campbellbarton) closed this revision.Apr 11 2022, 1:12 PM

Campbell Barton (campbellbarton) mentioned this in rBcda33345865a: Fix setting key shortcuts for insert keyframe menu items.Apr 11 2022, 1:12 PM

Thanks, committed rBcda33345865a2705e0cbe24448865b2bdfc7ead6