Page MenuHome
Differential D15047

VSE preview transform autokeying improvements
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on May 27 2022, 3:53 PM.

Details

Reviewers
Sybren A. Stüvel (sybren)
Richard Antalik (ISS)
Sergey Sharybin (sergey)
Maniphest Tasks
T98429: VSE autokeying transforms in preview only creates keyframes for rotation/scale if rotating/scaling around cursor (should keyframe position as well)
T98430: VSE autokeying transforms in preview only creates keyframes if there is an FCurve already
T98431: VSE autokeying transforms in preview does not work during animation playback
T98015: VSE strip transforms do not animate correctly and persist after clearing the animation
Commits
rB9ccc21dde372: VSE preview transform autokeying improvements
Summary
NOTE: this patch originated in T98015 which was split into multiple reports. While it could be split into multiple patches these are very much related so keeping as one for now

Test builds: https://builder.blender.org/download/patch/D15047/

This patch fixes the following issues:

[1] autokeying transforms in preview only creates keyframes if there is
an FCurve already
[2] autokeying transforms in preview only creates keyframes for
rotation/scale if rotating/scaling around cursor (should keyframe
position as well)
[3] autokeying transforms in preview does not work during animation
playback

For [1], a param was added to ED_autokeyframe_property which can tweak
its default behavior of only creating keyframes on already keyed
properties (which was fine because this is mostly called from buttons
where this behavior is desired). Callers such as gizmos (or the VSE in
our case) can use this additional param so that keyframes are also
created on "not-yet-keyframed" properties.

For [2], the pivot is checked and position properties also keyed if
necessary (which is also consistent with the way objects are keyed in
the 3DView)

For [3], animrecord_check_state was changed to be able to work on
scenes as well and the transform system in the VSE preview was made
aware of the screen's animtimer.

NOTE: there are still things to be improved for keyframing in the VSE, the most obvious is probably a keyframe_insert operator (with keyingsets)
NOTE: while testing, you may run into T98432: VSE unkeyed transforms in preview persist in cache

Fixes T98429, T98430, T98431

Diff Detail

Repository
rB Blender

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.May 27 2022, 3:53 PM
Philipp Oeser (lichtwerk) created this revision.
Harbormaster completed remote builds in B22301: Diff 51927.May 27 2022, 3:53 PM
Philipp Oeser (lichtwerk) added projects: VFX & Video, Video Sequencer, Animation & Rigging.May 27 2022, 3:56 PM
Philipp Oeser (lichtwerk) added a subscriber: Campbell Barton (campbellbarton).

CCing @Campbell Barton (campbellbarton) only for the case of ED_autokeyframe_property being called from a gizmo

Philipp Oeser (lichtwerk) added a comment.EditedMay 27 2022, 5:07 PM

Testbuilds up https://builder.blender.org/download/patch/D15047/

Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)May 27 2022, 5:34 PM
Richard Antalik (ISS) added a comment.May 29 2022, 2:53 AM

When checking functionality, I got crash after transformation if no keyframes were present - null dereference of fcu. Don't see fcurve created anywhere in this patch too.

Philipp Oeser (lichtwerk) added a comment.May 30 2022, 8:28 AM
In D15047#405617, @Richard Antalik (ISS) wrote:

When checking functionality, I got crash after transformation if no keyframes were present - null dereference of fcu.

Cannot repro, are you testing a Debug build?

Don't see fcurve created anywhere in this patch too.

This is eventually done by insert_keyframe

Philipp Oeser (lichtwerk) updated this revision to Diff 51981.May 30 2022, 8:56 AM

fix flipped condition for assert (Debug builds)

Harbormaster completed remote builds in B22315: Diff 51981.May 30 2022, 8:56 AM
Philipp Oeser (lichtwerk) added a reviewer: Sergey Sharybin (sergey).May 30 2022, 5:25 PM
Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)
Richard Antalik (ISS) accepted this revision.May 31 2022, 4:58 AM
This revision is now accepted and ready to land.May 31 2022, 4:58 AM
Sergey Sharybin (sergey) added inline comments.May 31 2022, 11:08 AM
source/blender/editors/include/ED_keyframing.h
664

Can the parameter be called only_if_property_keyed or something like this?

Philipp Oeser (lichtwerk) updated this revision to Diff 52055.May 31 2022, 12:33 PM

rename parameter

Harbormaster completed remote builds in B22347: Diff 52055.May 31 2022, 12:33 PM
Philipp Oeser (lichtwerk) updated this revision to Diff 52056.May 31 2022, 12:35 PM

again :)

Harbormaster completed remote builds in B22348: Diff 52056.May 31 2022, 12:35 PM
Philipp Oeser (lichtwerk) marked an inline comment as done.May 31 2022, 12:35 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Jun 2 2022, 5:08 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/animation/keyframing.c
3122–3124

This logic is confusing. Here only_if_property_keyed is set to true, but the check if (only_if_property_keyed) { further down can only be reached if (driven || special) is false. This means that setting the variable here to true is only done for the immediately following condition, but not for the other use of the same variable.

I'm not a fan of boolean parameters in the first place, but having them change semantics throughout the function is really not a good idea.

Declare the parameter as const bool, and change the condition on the next line to

if (fcu == NULL && (driven || special || only_if_property_keyed)) {
  return changed;
}
3159

path is only used when fcu == NULL, so it's a bit inefficient to allocate memory, build the path, and free the memory again, when it's not going to be used.

source/blender/editors/include/ED_keyframing.h
665

Space after period

source/blender/editors/transform/transform_convert_sequencer_image.c
160

const

166–183

I think this is expressed in a clearer way like this:

const bool around_cursor = scene->toolsettings->sequencer_tool_settings->pivot_point ==
                           V3D_AROUND_CURSOR;
const bool do_loc = tmode == TFM_TRANSLATION || around_cursor;
const bool do_rot = tmode == TFM_ROTATION;
const bool do_scale = tmode == TFM_RESIZE;

That way you can use const bool instead of bool, and it's much simpler to see when a certain do_xxx is set or not.

This revision now requires changes to proceed.Jun 2 2022, 5:08 PM
Philipp Oeser (lichtwerk) updated this revision to Diff 52219.Jun 3 2022, 2:53 PM

address review comments

Harbormaster completed remote builds in B22413: Diff 52219.Jun 3 2022, 2:53 PM
Sybren A. Stüvel (sybren) accepted this revision.EditedJun 3 2022, 3:30 PM

Just a small change; MEM_SAFE_FREE(x) is pretty much the same as the current code, but easier to read.

No need to re-review after changing.

source/blender/editors/animation/keyframing.c
3178–3181

MEM_SAFE_FREE(path);

This revision is now accepted and ready to land.Jun 3 2022, 3:30 PM
Closed by commit rB9ccc21dde372: VSE preview transform autokeying improvements (authored by Philipp Oeser (lichtwerk)). · Explain WhyJun 7 2022, 9:56 AM
This revision was automatically updated to reflect the committed changes.
Philipp Oeser (lichtwerk) added a commit: rB9ccc21dde372: VSE preview transform autokeying improvements.

Revision Contents
Changeset List

PathSize
source/
blender/
editors/
animation/
31 lines
include/
9 lines
interface/
2 lines
transform/
2 lines
3 lines
2 lines
2 lines
65 lines
4 lines
windowmanager/
gizmo/
intern/
2 lines

Diff 52331

View Options

source/blender/editors/animation/keyframing.c

Loading...
View Options

source/blender/editors/include/ED_keyframing.h

Loading...
View Options

source/blender/editors/interface/interface_anim.c

Loading...
View Options

source/blender/editors/transform/transform_convert.h

Loading...
View Options

source/blender/editors/transform/transform_convert.c

Loading...
View Options

source/blender/editors/transform/transform_convert_armature.c

Loading...
View Options

source/blender/editors/transform/transform_convert_object.c

Loading...
View Options

source/blender/editors/transform/transform_convert_sequencer_image.c

Loading...
View Options

source/blender/editors/transform/transform_generics.c

Loading...
View Options

source/blender/windowmanager/gizmo/intern/wm_gizmo_target_props.c

Loading...