Fix: Autokeyframe with Insert Needed with no keyframes #116219

Christoph Lendenfeld · 2023-12-15T11:33:36+01:00

2023-12-15 11:33:36 +01:00

Caused by #115522: Anim: Don't use keying sets when inserting keyframes during autokeying

The issue occurs when auto-keyframe AND "Only Insert Needed" enabled.
And only when no keyframes have been added yet.

Before the commit that caused the issue, moving an object with G would
create only location keyframes. After it would key all.
That is because that commit removed the logic that checks the
eTfmMode (Transform mode). That only works as long as there are already
keyframes on the object/bone because the logic needs an
existing value to compare against. In the case where the first keyframe is set,
it would always key everything.

The fix is to bring back the logic that checks the Transform mode and pass
a Span of rna paths to the autokeyframe function. This restores the behavior.

This still has the issue that "Only Insert Needed" behaves differently if
keys exist vs inserting the first keys. While this isn't ideal, I don't see a way
to get values of an object/bone before and after the transformation.
We might be able to fix this in a future PR, but for now we restore the
old behavior.

Caused by [#115522: Anim: Don't use keying sets when inserting keyframes during autokeying](https://projects.blender.org/blender/blender/pulls/115522) The issue occurs when auto-keyframe AND "Only Insert Needed" enabled. And only when no keyframes have been added yet. Before the commit that caused the issue, moving an object with `G` would create only location keyframes. After it would key all. That is because that commit removed the logic that checks the `eTfmMode` (Transform mode). That only works as long as there are already keyframes on the object/bone because the logic needs an existing value to compare against. In the case where the first keyframe is set, it would always key everything. The fix is to bring back the logic that checks the Transform mode and pass a `Span` of rna paths to the autokeyframe function. This restores the behavior. This still has the issue that "Only Insert Needed" behaves differently if keys exist vs inserting the first keys. While this isn't ideal, I don't see a way to get values of an object/bone before and after the transformation. We might be able to fix this in a future PR, but for now we restore the old behavior.

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-12-15 11:33:36 +01:00

Christoph Lendenfeld added 2 commits 2023-12-15 11:33:46 +01:00

21f436dc93 wip

93e05d4979 the fix

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-12-15 11:34:02 +01:00

Sybren A. Stüvel approved these changes 2023-12-19 10:48:26 +01:00

Sybren A. Stüvel left a comment

👍 for the reasoning behind this PR.

I think some of the code is just put back pretty much as-is, so then I can finally complain about its state ;-)
Just a few small nags though, nothing shocking. Those can be addressed when landing.

:+1: for the reasoning behind this PR. I think some of the code is just put back pretty much as-is, so then I can finally complain about its state ;-) Just a few small nags though, nothing shocking. Those can be addressed when landing.

source/blender/animrig/ANIM_keyframing.hh Outdated

						
				@ -143,3 +143,3 @@

				bool autokeyframe_cfra_can_key(const Scene *scene, ID *id);

				void autokeyframe_object(bContext *C, Scene *scene, Object *ob);

				void autokeyframe_object(bContext *C, Scene *scene, Object *ob, Span<std::string> rna_paths);

Sybren A. Stüvel commented

2023-12-19 10:19:08 +01:00

Maybe add some documentation for the rna_paths parameter. It's otherwise hard to know whether it's "always key all these paths" or "never key anything outside this set".

Maybe add some documentation for the `rna_paths` parameter. It's otherwise hard to know whether it's "always key all these paths" or "never key anything outside this set".

source/blender/animrig/ANIM_keyframing.hh Outdated

						
				@ -163,0 +163,4 @@

				                               Scene *scene,

				                               Object *ob,

				                               bPoseChannel *pose_channel,

				                               Span<std::string> rna_paths,

Sybren A. Stüvel commented

2023-12-19 10:20:17 +01:00

Same request as above, to document rna_paths. I think the rest is either already documented or clear enough from the context.

Same request as above, to document `rna_paths`. I think the rest is either already documented or clear enough from the context.

source/blender/animrig/ANIM_rna.hh

						
				@ -17,2 +18,4 @@

				Vector<float> get_rna_values(PointerRNA *ptr, PropertyRNA *prop);

				/** Helper function to get the rna path for the given rotation mode. */

				std::string get_rotation_mode_path(eRotationModes rotation_mode);

Sybren A. Stüvel commented

2023-12-19 10:23:01 +01:00

"Helper function to" can be removed; "Get the rna path for the given rotation mode." is clear enough.

source/blender/animrig/intern/anim_rna.cc

						
				@ -69,2 +70,4 @@

				  return values;

				}

				std::string get_rotation_mode_path(const eRotationModes rotation_mode)

Sybren A. Stüvel commented

2023-12-19 10:25:17 +01:00

Low prio: this might not need to be std::string, maybe std::string_view or blender::StringRef works just as well, and avoids a copy.

Not for this PR though, as just moving the function as-is is better here.

Low prio: this might not need to be `std::string`, maybe `std::string_view` or `blender::StringRef` works just as well, and avoids a copy. Not for this PR though, as just moving the function as-is is better here.

source/blender/animrig/intern/keyframing_auto.cc

						
				@ -200,3 +187,3 @@

				}

				void autokeyframe_pose(bContext *C, Scene *scene, Object *ob, short targetless_ik)

				void autokeyframe_pose_channel(bContext *C,

Sybren A. Stüvel commented

2023-12-19 10:32:32 +01:00

This function now grabs stuff from the context and creates the AnimationEvalContext for every pose channel:

ReportList *reports = CTX_wm_reports(C);
ToolSettings *ts = scene->toolsettings;
KeyingSet *active_ks = ANIM_scene_get_active_keyingset(scene);
Depsgraph *depsgraph = CTX_data_depsgraph_pointer(C);
const float scene_frame = BKE_scene_frame_get(scene);
const AnimationEvalContext anim_eval_context = BKE_animsys_eval_context_construct(depsgraph, scene_frame);

For this PR I think this is fine, but it might be worth looking into creating some AutoKeyframeContext struct that contains all of those, and pass that into this function instead.

This function now grabs stuff from the context and creates the `AnimationEvalContext` for every pose channel: ```cpp ReportList *reports = CTX_wm_reports(C); ToolSettings *ts = scene->toolsettings; KeyingSet *active_ks = ANIM_scene_get_active_keyingset(scene); Depsgraph *depsgraph = CTX_data_depsgraph_pointer(C); const float scene_frame = BKE_scene_frame_get(scene); const AnimationEvalContext anim_eval_context = BKE_animsys_eval_context_construct(depsgraph, scene_frame); ``` For this PR I think this is fine, but it might be worth looking into creating some `AutoKeyframeContext` struct that contains all of those, and pass that into this function instead.

Christoph Lendenfeld commented

2023-12-19 12:01:16 +01:00

In the long term I'd like to remove the need for AnimationEvalContext from this function entirely

In the long term I'd like to remove the need for `AnimationEvalContext` from this function entirely

source/blender/editors/transform/transform_convert_armature.cc Outdated

						
				@ -1268,1 +1269,4 @@

				static blender::Vector<std::string> get_modified_rna_paths(const eTfmMode tmode,

				                                                           ToolSettings *toolsettings,

				                                                           std::string &rotation_path,

Sybren A. Stüvel commented

2023-12-19 10:33:20 +01:00

rotation_path can be a blender::StringRef.

`rotation_path` can be a `blender::StringRef`.

source/blender/editors/transform/transform_convert_armature.cc Outdated

						
				@ -1269,0 +1273,4 @@

				                                                           const bool targetless_ik)

				{

				  blender::Vector<std::string> rna_paths;

				  if (tmode == TFM_TRANSLATION) {

Sybren A. Stüvel commented

2023-12-19 10:33:47 +01:00

This should be a switch (tmode) instead.

This should be a `switch (tmode)` instead.

source/blender/editors/transform/transform_convert_armature.cc

						
				@ -1269,0 +1315,4 @@

				    }

				    blender::Vector<std::string> rna_paths;

				    std::string rotation_path = blender::animrig::get_rotation_mode_path(

Sybren A. Stüvel commented

2023-12-19 10:36:39 +01:00

const, and possibly a blender::StringRef

`const`, and possibly a `blender::StringRef`

source/blender/editors/transform/transform_convert_object.cc Outdated

						
				@ -755,6 +756,70 @@ static bool motionpath_need_update_object(Scene *scene, Object *ob)

				/** \name Recalc Data object

				 * \{ */

				static blender::Vector<std::string> get_modified_rna_paths(const eTfmMode tmode,

Sybren A. Stüvel commented

2023-12-19 10:40:47 +01:00

The function name could be clearer. It's likely meant as "the RNA paths that were modified by the transform system", but that's only clear once you know what it does. Maybe rna_paths_touched_by_transform_system or something along those lines?

This code also looks very much dependent on specifics of other areas of the code. I don't think we can totally avoid that without major rewrites of other areas. It would be nice, if it's doable, to add a comment here that explains which function(s) this logic should be in sync with.

The function name could be clearer. It's likely meant as "the RNA paths that were modified by the transform system", but that's only clear once you know what it does. Maybe `rna_paths_touched_by_transform_system` or something along those lines? This code also looks very much dependent on specifics of other areas of the code. I don't think we can totally avoid that without major rewrites of other areas. It would be nice, if it's doable, to add a comment here that explains which function(s) this logic should be in sync with.

Christoph Lendenfeld commented

2023-12-19 11:58:01 +01:00

named it get_affected_rna_paths_from_transform_mode and also used a switch here instead of the if/else chain

About which functions this should be in sync with. Not sure what you mean. You mean that it needs to be in sync with the transform system in general?

named it `get_affected_rna_paths_from_transform_mode` and also used a `switch` here instead of the if/else chain About which functions this should be in sync with. Not sure what you mean. You mean that it needs to be in sync with the transform system in general?

Sybren A. Stüvel commented

2023-12-21 15:39:17 +01:00

Yeah, and with the code as it is now, that's obvious enough 👍

Yeah, and with the code as it is now, that's obvious enough :+1:

Christoph Lendenfeld added 2 commits 2023-12-19 11:58:26 +01:00

0c7ca94e60 Merge branch 'main' into fix_autokeyframe_keyed_channels

354dba5efc implement review comments

Christoph Lendenfeld merged commit 614d7749df into main

2023-12-21 10:50:26 +01:00

Christoph Lendenfeld referenced this issue from a commit

2023-12-21 10:50:26 +01:00

Fix: Autokeyframe with Insert Needed with no keyframes