Paths

Table of Contentst

VSE: Add insert keyframe menu
Needs RevisionPublic
Actions

Authored by Richard Antalik (ISS) on Jun 20 2022, 5:00 PM.

Details

Reviewers

Summary

Add new builtin keying sets for sequencer to allow for quickly
animating image location, rotation and scale. This is useful feature,
that improves workflow when animating image transformation.

It is possible to insert keyframes to custom keying sets in context of
preview region, but since these properties can be changed only from
side panel, it does not make sense to add these to builtin sets at this
moment.

Poll functions has been refactored to allow other editors to be used.
RKS_POLL_selected_items was replaced by
RKS_POLL_selected_objects_bones.

Refactoring was also necessary for "Available" keyng sets, since it was
assumed, that object is being animated. To work within sequencer and
3D view context, functions RKS_POLL_view_3d and
RKS_POLL_selected_sequences were created.

Example:

Diff Detail

Repository

rB Blender

Branch

insert_kf_menu (branched from master)

Build Status

Buildable 22604
Build 22604: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Jun 20 2022, 5:00 PM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B22604: Diff 52697.Jun 20 2022, 5:00 PM

The patch description seems incomplete. The diff changes the code of the existing keying sets, but this is not mentioned at all in the description. What's the reason for this, and what's the impact?

Also please see Ingredients of a Patch -- changes in the UI also should be accompanied by some screenshots.

This revision now requires changes to proceed.Nov 14 2022, 6:04 PM

Sybren A. Stüvel (sybren) added a project: Animation & Rigging.Nov 14 2022, 6:04 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Nov 14 2022, 7:14 PM

Fix list of "declared functions"

Harbormaster completed remote builds in B24640: Diff 57616.Nov 14 2022, 7:18 PM

In D15243#449051, @Sybren A. Stüvel (sybren) wrote:

Changes in the UI also should be accompanied by some screenshots.

I wouldn't consider this as change in UI, as it reuses existing feature, but I have added a screenshot. It's true that sometimes lay people look at these patches and they have no idea what is done by the patch.

Other properties are still possible to animate in preview

What does "to animate in preview" mean?

In D15243#449082, @Richard Antalik (ISS) wrote:

I wouldn't consider this as change in UI, as it reuses existing feature

You still added something that wasn't there before.

but I have added a screenshot.

Thanks for that.

It's true that sometimes lay people look at these patches and they have no idea what is done by the patch.

Also reviewers like to have a better picture of what the patch is doing before actually looking at the code. For me, when reviewing, the first question is always whether the patch does something desirable or not. Seeing what things look like helps.

As for the overall design of the code, I think it would be better to make the separation of available keying sets at a higher level. Currently there is a reuse of functions which then, at their lowest level, change their behaviour depending on the currently active editor. This gives a bit of a code smell, since it's hard to know what a keying set is doing until you dive into the nitty-gritty of the implementation details. My suggestion is to add a few more KeyingSetInfo subclasses specifically for the VSE cases. The existing subclasses can then check for the 3D Viewport in their poll function, and the rest of their code doesn't need to change. The VSE-specific subclasses can then do whatever they need to do. This also avoids confusion as to what keying sets will do when called from C code, which also happens in various places.

release/scripts/modules/keyingsets_utils.py
54	To ensure a boolean is returned, use `... and bool(context.selected_sequences)`

This revision now requires changes to proceed.Nov 15 2022, 4:55 PM

In D15243#449492, @Sybren A. Stüvel (sybren) wrote:

Other properties are still possible to animate in preview

What does "to animate in preview" mean?

What I meant is that you can insert keyframes to custom keying sets in context of preview region.

It's true that sometimes lay people look at these patches and they have no idea what is done by the patch.

Also reviewers like to have a better picture of what the patch is doing before actually looking at the code. For me, when reviewing, the first question is always whether the patch does something desirable or not. Seeing what things look like helps.

My suggestion is to add a few more KeyingSetInfo subclasses specifically for the VSE cases.

Do you suggest to not touch existing classes at all? I would rename these so they have prefix BUILTIN_KSI_View3D_. But some don't have bl_idname defined, so this would have to be defined to match old name so scripts using bpy.ops.anim.keyframe_insert_by_name() won't break by this change.
Alternatively, I can just add new classes with prefix BUILTIN_KSI_Sequencer_.

In D15243#449910, @Richard Antalik (ISS) wrote:

In D15243#449492, @Sybren A. Stüvel (sybren) wrote:

Other properties are still possible to animate in preview

What does "to animate in preview" mean?

What I meant is that you can insert keyframes to custom keying sets in context of preview region.

Ah, right. It will be clearer to just use a few more words, and describe it as "the VSE's review region".

Do you suggest to not touch existing classes at all? I would rename these so they have prefix BUILTIN_KSI_View3D_. But some don't have bl_idname defined, so this would have to be defined to match old name so scripts using bpy.ops.anim.keyframe_insert_by_name() won't break by this change.

Yeah, not touching them seems like the right way to go. Otherwise people's keymaps, quick favourites, etc. also all have to change, and I think that's not worth it.

Alternatively, I can just add new classes with prefix BUILTIN_KSI_Sequencer_.

I think that's better.

Alternatively, I can just add new classes with prefix BUILTIN_KSI_Sequencer_.

I think that's better.

There is a problem though - builtin keyng sets' bl_idname but also bl_label is ensured to be unique. Having unique bl_label is useful for ading keying sets via UI template.
This is how it looks after BUILTIN_KSI_Sequencer_ classes are used:

A solution could be not rename labels for builtin keying sets in BKE_keyingset_add. If that is not good solution, I can only think of implementing this patch as it stands.

In D15243#453213, @Richard Antalik (ISS) wrote:

There is a problem though - builtin keyng sets' bl_idname but also bl_label is ensured to be unique. Having unique bl_label is useful for ading keying sets via UI template.
This is how it looks after BUILTIN_KSI_Sequencer_ classes are used:

Oh my that is ugly.

A solution could be not rename labels for builtin keying sets in BKE_keyingset_add. If that is not good solution, I can only think of implementing this patch as it stands.

I think keeping the labels as-is is the right way to go. I mean, even with the renaming you wouldn't know which one was kept as "Location" and which one is "Location.001", so I don't see what that renaming is actually solving.

Use sequencer specific keying sets

Harbormaster completed remote builds in B25077: Diff 58703.Dec 16 2022, 8:30 PM

Cleanup whitespace

Harbormaster completed remote builds in B25078: Diff 58704.Dec 16 2022, 8:33 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Dec 16 2022, 8:41 PM

I think the code structure can be improved. RKS_ITER_selected_item is now actually two functions in one (one for the 3D view and one for the sequencer). Since it is used in selection sets that already have specific poll functions that separate out these cases, it doesn't make sense to have them call the same function, only to have that function separate those cases out yet again. Why not have two different iterator functions that match the two different poll functions?

This also applies to the if isinstance(data, bpy.types.ID): ... elif isinstance(data, bpy.types.Sequence): ... functions. It would be quite a bit clearer if the sequencer-specific classes would use a sequencer-specific generator.

This revision now requires changes to proceed.Jan 2 2023, 5:42 PM