Refactor: combine insert_keyframe() and insert_key_rna() into a single function #122053

Merged
Nathan Vegdahl merged 49 commits from nathanvegdahl/blender:combine_keying_functions into main 2024-06-11 16:43:08 +02:00
Member

Based on discussions with @ChrisLend

The goal of this PR is to merge insert_keyframe() and insert_key_rna() into
a single function, insert_keyframes(), that fully accommodates the
functionality of both. This results in a bit of a mega function, which isn't
great, but it centralizes a lot of otherwise redundant keyframing code so it
only needs to be changed in one place in the future.

Future PRs can work to reduce the "mega" aspect of this function, stripping it
down to its core functionality and eliminating left over incidental redundancy
(e.g. passing both scene_frame and anim_eval_context).

As a wonderful side effect, this PR also makes layered action keyframing work in
most of the remaining places left over from #121661, such as buttons, the
dopesheet, the graph editor, etc.

Based on discussions with @ChrisLend The goal of this PR is to merge `insert_keyframe()` and `insert_key_rna()` into a single function, `insert_keyframes()`, that fully accommodates the functionality of both. This results in a bit of a mega function, which isn't great, but it centralizes a lot of otherwise redundant keyframing code so it only needs to be changed in one place in the future. Future PRs can work to reduce the "mega" aspect of this function, stripping it down to its core functionality and eliminating left over incidental redundancy (e.g. passing both `scene_frame` and `anim_eval_context`). As a wonderful side effect, this PR also makes layered action keyframing work in most of the remaining places left over from #121661, such as buttons, the dopesheet, the graph editor, etc.
Nathan Vegdahl added 9 commits 2024-05-21 15:27:00 +02:00
Nathan Vegdahl added the
Module
Animation & Rigging
label 2024-05-21 15:28:09 +02:00
Nathan Vegdahl added 1 commit 2024-05-21 15:47:31 +02:00
Nathan Vegdahl added 1 commit 2024-05-21 16:25:43 +02:00
Nathan Vegdahl added 1 commit 2024-05-21 17:36:29 +02:00
This also involved switching some more call sites over to
`insert_key_rna()` that LSP missed
Nathan Vegdahl added 1 commit 2024-05-21 17:49:09 +02:00
Also slightly reorganize the parameters so that the RNA paths come
earlier, since they're more primary than e.g. the channel group.
Nathan Vegdahl added 2 commits 2024-05-23 16:09:36 +02:00
This splits channel group naming off into a separate function that
actually does the right thing.
Nathan Vegdahl added 1 commit 2024-05-23 16:24:27 +02:00
Nathan Vegdahl added 1 commit 2024-05-23 16:32:17 +02:00
Christoph Lendenfeld requested changes 2024-05-23 17:14:21 +02:00
Dismissed
Christoph Lendenfeld left a comment
Member

looks good overall, surprisingly little changes outside the keying code which is nice

looks good overall, surprisingly little changes outside the keying code which is nice
@ -988,3 +839,1 @@
}
const float nla_frame = BKE_nla_tweakedit_remap(adt, scene_frame, NLATIME_CONVERT_UNMAP);
const float nla_frame = nla_time_remap(scene_frame.value_or(anim_eval_context.eval_time),

I think it would make sense to pass in the scene_frame and a Depsgraph pointer, both required, instead of sometimes using the time from the eval context.
Considering an AnimEvalContext is exactly that it would make things a lot clearer. We can always construct an AnimEvalContext from that data if needed.

Edit: or just pass an AnimEvalContext and use that exclusively

I think it would make sense to pass in the scene_frame and a Depsgraph pointer, both required, instead of sometimes using the time from the eval context. Considering an AnimEvalContext is exactly that it would make things a lot clearer. We can always construct an AnimEvalContext from that data if needed. Edit: or just pass an AnimEvalContext and use that exclusively
Author
Member

(I've clarified this in the PR description now.)

I agree that we should move toward that. For this PR I'm just giving the function all the capabilities of the two it was created from, and trying to make minimal changes to the code that called each function. In this case insert_keyframe() implicitly used scene time whereas insert_key_rna() took the time explicitly. So this line accommodates both behaviors.

I was thinking we could strip this function down in those ways in a separate PR.

(I've clarified this in the PR description now.) I agree that we should move toward that. For this PR I'm just giving the function all the capabilities of the two it was created from, and trying to make minimal changes to the code that called each function. In this case `insert_keyframe()` implicitly used scene time whereas `insert_key_rna()` took the time explicitly. So this line accommodates both behaviors. I was thinking we could strip this function down in those ways in a separate PR.

sounds good. Got nothing against reworking this in a separate PR

sounds good. Got nothing against reworking this in a separate PR
nathanvegdahl marked this conversation as resolved
@ -1028,0 +886,4 @@
if (combined_result.get_count(SingleKeyingResult::SUCCESS) > 0) {
DEG_id_tag_update(&action->id, ID_RECALC_ANIMATION_NO_FLUSH);
if (adt->action != nullptr && adt->action != action) {
DEG_id_tag_update(&adt->action->id, ID_RECALC_ANIMATION_NO_FLUSH);

In what case would the action from the AnimData and the action we get from id_action_ensure not be the same?

In what case would the action from the AnimData and the action we get from `id_action_ensure` not be the same?
Author
Member

That's a great question. I'm not sure myself, but insert_keyframe() checked for this, so I'm keeping it in for now just in case. (At first I thought it might be related to the NLA, but IIRC if there's an NLA action for keying then it's temporarily stored as the animdata action anyway.) It's something I think we can probably investigate and remove later, but for now it's a "don't remove code you don't understand" kind of situation.

That's a great question. I'm not sure myself, but `insert_keyframe()` checked for this, so I'm keeping it in for now just in case. (At first I thought it might be related to the NLA, but IIRC if there's an NLA action for keying then it's temporarily stored as the animdata action anyway.) It's something I think we can probably investigate and remove later, but for now it's a "don't remove code you don't understand" kind of situation.

Maybe add your findings as a comment then so it's easier to investigate later when we actually want to remove it.

Maybe add your findings as a comment then so it's easier to investigate later when we actually want to remove it.
nathanvegdahl marked this conversation as resolved
@ -1093,0 +1086,4 @@
CombinedKeyingResult result = insert_keyframes(bmain,
*keyingset_path->id,
{{keyingset_path->rna_path, {}, array_index}},
groupname ? std::optional(groupname) :

might just be me but I find it easier to read if such things are outside the function arguments.

might just be me but I find it easier to read if such things are outside the function arguments.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-05-24 15:27:20 +02:00
Nathan Vegdahl added 1 commit 2024-05-24 15:53:46 +02:00
Author
Member

I noticed at the end of last week when reviewing my own code here that I missed something from insert_keyframe() in the new merged function. So it's back to not being ready for review again.

I noticed at the end of last week when reviewing my own code here that I missed something from `insert_keyframe()` in the new merged function. So it's back to not being ready for review again.
Nathan Vegdahl added 1 commit 2024-05-27 11:07:43 +02:00
Nathan Vegdahl added 1 commit 2024-05-27 12:19:26 +02:00
Nathan Vegdahl added 1 commit 2024-05-27 13:40:37 +02:00
Nathan Vegdahl added 1 commit 2024-05-27 15:48:46 +02:00
Nathan Vegdahl added 1 commit 2024-05-27 17:55:32 +02:00
Nathan Vegdahl added 1 commit 2024-05-27 17:57:04 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 11:39:29 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 11:44:48 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 14:42:18 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 15:44:26 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 16:06:40 +02:00
Nathan Vegdahl changed title from WIP: Refactor: Anim: combine insert_keyframe() and insert_key_rna() into a single function to Refactor: Anim: combine insert_keyframe() and insert_key_rna() into a single function 2024-05-28 16:10:39 +02:00
Nathan Vegdahl requested review from Christoph Lendenfeld 2024-05-28 16:10:57 +02:00
Author
Member

Okay, finally ready for a proper review! I'll start another PR (to be merged first) with unit tests that cover the relevant behavior.

Okay, finally ready for a proper review! I'll start another PR (to be merged first) with unit tests that cover the relevant behavior.
Author
Member

Hmm, perhaps I spoke to soon. Actually testing, some things are definitely broken. Feel free to hold off on the review until I track them down and fix them.

Hmm, perhaps I spoke to soon. Actually testing, some things are definitely broken. Feel free to hold off on the review until I track them down and fix them.
Nathan Vegdahl added 1 commit 2024-05-28 16:59:02 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 17:20:14 +02:00
Nathan Vegdahl added 1 commit 2024-05-28 17:32:13 +02:00
Author
Member

Okay, fixed! Turns out we have to use PointerRNA * instead of ID * as the input to insert_keyframes(), because for some call sites it might be e.g. a pose bone rather than an ID.

Okay, fixed! Turns out we *have* to use `PointerRNA *` instead of `ID *` as the input to `insert_keyframes()`, because for some call sites it might be e.g. a pose bone rather than an ID.

did a superficial test and looks like it's working great. Will dive a bit deeper on thursday

did a superficial test and looks like it's working great. Will dive a bit deeper on thursday
Christoph Lendenfeld approved these changes 2024-05-30 11:51:05 +02:00
Christoph Lendenfeld left a comment
Member

good job, really liking where this is going

good job, really liking where this is going

@nathanvegdahl almost forgot, but the title should either be Anim: OR Refactor:
iirc there was some discussion about this in blender-coders a while ago. Some script doesn't work correctly if there is both

@nathanvegdahl almost forgot, but the title should either be `Anim:` OR `Refactor:` iirc there was some discussion about this in blender-coders a while ago. Some script doesn't work correctly if there is both
Nathan Vegdahl added 2 commits 2024-05-30 12:41:34 +02:00
Author
Member

Ah! Thanks, didn't realize that.

Ah! Thanks, didn't realize that.
Nathan Vegdahl changed title from Refactor: Anim: combine insert_keyframe() and insert_key_rna() into a single function to Refactor: combine insert_keyframe() and insert_key_rna() into a single function 2024-05-30 12:43:45 +02:00
Nathan Vegdahl added 2 commits 2024-06-06 12:43:26 +02:00
Nathan Vegdahl added 1 commit 2024-06-06 13:13:59 +02:00
Nathan Vegdahl added 1 commit 2024-06-10 11:00:16 +02:00
Nathan Vegdahl added 1 commit 2024-06-10 11:32:52 +02:00
Nathan Vegdahl added 1 commit 2024-06-10 11:49:11 +02:00
Nathan Vegdahl added 1 commit 2024-06-10 12:00:04 +02:00
Nathan Vegdahl added 1 commit 2024-06-10 12:22:26 +02:00
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-06-10 12:24:14 +02:00
Sybren A. Stüvel requested changes 2024-06-11 11:57:03 +02:00
Sybren A. Stüvel left a comment
Member

Most of this PR looks good, and is a really nice cleanup. Just a few remarks / questions.

Most of this PR looks good, and is a really nice cleanup. Just a few remarks / questions.
@ -130,0 +138,4 @@
*
* \param scene_frame: the frame to insert the keys at. This is in scene time,
* not NLA mapped (NLA mapping is already handled internally by this function).
* If not given, the current scene time is used.

"the current scene time" is ill defined here. There is no parameter that gives you this information unambiguously. I think this assumes that anim_eval_context.eval_time is always the scene time (which it may or may not be), but that assumption is not documented here. This means that the reader wouldn't know where this value comes from.

"the current scene time" is ill defined here. There is no parameter that gives you this information unambiguously. I think this assumes that `anim_eval_context.eval_time` is always the scene time (which it may or may not be), but that assumption is not documented here. This means that the reader wouldn't know where this value comes from.
Author
Member

Ah! Thanks for catching that. I wrote those docs when I thought AnimationEvalContext represented something more fundamental/core than it actually does.

Ah! Thanks for catching that. I wrote those docs when I thought `AnimationEvalContext` represented something more fundamental/core than it actually does.
nathanvegdahl marked this conversation as resolved
@ -428,2 +429,3 @@
static float nla_time_remap(const AnimationEvalContext *anim_eval_context,
static float nla_time_remap(float time,
const AnimationEvalContext *anim_eval_context,

anim_eval_context also contains a time, so it's unclear what the float time parameter means here, or why it is necessary.

Also it doesn't seem to be modified in the function, so const.

`anim_eval_context` also contains a time, so it's unclear what the `float time` parameter means here, or why it is necessary. Also it doesn't seem to be modified in the function, so `const`.
Author
Member

I agree this is redundant. The reason for both to exist right now is:

  1. That same redundancy was already present in insert_key_rna(), and I'm trying to keep this as close to a pure "combine these two functions" refactor as I reasonably can, to reduce the number of moving bits.
  2. This lets us keep the call sites as similar as possible to how they already were (insert_keyframe() had no separate frame parameter, but insert_key_rna() did) in this PR.

My plan is to eliminate this redundancy (along with some other clean ups/reductions of this mega function) in a future PR.

I agree this is redundant. The reason for both to exist right now is: 1. That same redundancy was already present in `insert_key_rna()`, and I'm trying to keep this as close to a pure "combine these two functions" refactor as I reasonably can, to reduce the number of moving bits. 2. This lets us keep the call sites as similar as possible to how they already were (`insert_keyframe()` had no separate frame parameter, but `insert_key_rna()` did) in this PR. My plan is to eliminate this redundancy (along with some other clean ups/reductions of this mega function) in a future PR.

Ok, that's fine then. Please do mention this in the PR description ;-)

Ok, that's fine then. Please do mention this in the PR description ;-)
Author
Member

Will do. I also left a todo in the doc comment about it now.

Will do. I also left a todo in the doc comment about it now.
nathanvegdahl marked this conversation as resolved
@ -1111,0 +948,4 @@
const std::optional<StringRefNull> channel_group,
const blender::Span<RNAPath> rna_paths,
const std::optional<float> scene_frame,
const AnimationEvalContext &anim_eval_context,

Same question as above. Why is there an std::optional<float> scene_frame when there's also anim_eval_context.eval_time?

Same question as above. Why is there an `std::optional<float> scene_frame` when there's also `anim_eval_context.eval_time`?
nathanvegdahl marked this conversation as resolved
@ -1167,0 +1009,4 @@
bool force_all;
/* NOTE: this function call is complex with interesting effects. Of
* particular note is that in addition to doing value remapping, it also:

The functionality of BKE_animsys_nla_remap_keyframe_values() should be documented at the declaration of that function, not here. The note that this is an "interesting" function with various potentially-unexpected effects is very welcome though.

The functionality of `BKE_animsys_nla_remap_keyframe_values()` should be documented at the declaration of that function, not here. The note that this is an "interesting" function with various potentially-unexpected effects is very welcome though.
Author
Member

Good call. I've made a separate PR that can be landed either before or after this one that (hopefully) improves the documentation of BKE_animsys_nla_remap_keyframe_values() in the ways needed to understand this call site's usage of it: #123081

Good call. I've made a separate PR that can be landed either before or after this one that (hopefully) improves the documentation of `BKE_animsys_nla_remap_keyframe_values()` in the ways needed to understand this call site's usage of it: #123081
Author
Member

I've now removed the output parameter descriptions, leaving that to #123081. I also renamed the elements_to_key variable to rna_values_mask, to make its relationship to rna_values more obvious.

I've now removed the output parameter descriptions, leaving that to #123081. I also renamed the `elements_to_key` variable to `rna_values_mask`, to make its relationship to `rna_values` more obvious.
nathanvegdahl marked this conversation as resolved
@ -1175,0 +1039,4 @@
* we're faithfully reproducing the original behavior.
*/
eInsertKeyFlags insert_key_flags_adjusted = insert_key_flags;
if (force_all && rna_values.size() > 0 &&

rna_values.size() > 0!rna_values.is_empty().

But actually that part of the condition can be entirely removed. The chance of rna_values actually being empty is minute (why is this function even getting called, in that case?), and so this only 'optimizes' away two numerical comparisons and an assignment to at_least_one_would_succeed.

`rna_values.size() > 0` → `!rna_values.is_empty()`. But actually that part of the condition can be entirely removed. The chance of `rna_values` actually being empty is minute (why is this function even getting called, in that case?), and so this only 'optimizes' away two numerical comparisons and an assignment to `at_least_one_would_succeed`.
nathanvegdahl marked this conversation as resolved
@ -1175,0 +1040,4 @@
*/
eInsertKeyFlags insert_key_flags_adjusted = insert_key_flags;
if (force_all && rna_values.size() > 0 &&
(insert_key_flags & (INSERTKEY_REPLACE | INSERTKEY_AVAILABLE)) != 0)

The != 0 can be removed, which would, for me, increase the readability of this condition.

The ` != 0` can be removed, which would, for me, increase the readability of this condition.
nathanvegdahl marked this conversation as resolved
@ -1189,0 +1097,4 @@
* as the action in AnimData. Further, it's not clear why it would need to
* be tagged for a depsgraph update regardless. This code is here because it
* was part of the function this one was refactored from, but at some point
* this should be investigated and either documented or removed. */

👍

:+1:
nathanvegdahl marked this conversation as resolved
@ -266,0 +264,4 @@
&object_rna_pointer,
std::nullopt,
{{"rotation_euler"}},
1.0,

Maybe not for this PR, as this is not a thing that's introduced here & now, but the scene frame range is a trivial value 1.0 (which I would try to avoid), but also it's the same as anim_eval_context, and so a mistake in which one is picked will not show up in this test.

Maybe not for this PR, as this is not a thing that's introduced here & now, but the scene frame range is a trivial value `1.0` (which I would try to avoid), but also it's the same as `anim_eval_context`, and so a mistake in which one is picked will not show up in this test.
Author
Member

Good point. But since the intent is to remove the redundancy in a future PR anyway, I think it's fine for now.

Good point. But since the intent is to remove the redundancy in a future PR anyway, I think it's fine for now.
dr.sybren marked this conversation as resolved
Nathan Vegdahl added 2 commits 2024-06-11 12:35:51 +02:00
Nathan Vegdahl added 1 commit 2024-06-11 14:03:07 +02:00
Nathan Vegdahl added 1 commit 2024-06-11 14:20:53 +02:00
Nathan Vegdahl added 1 commit 2024-06-11 15:42:26 +02:00
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-06-11 15:44:41 +02:00
Sybren A. Stüvel approved these changes 2024-06-11 16:00:36 +02:00
Sybren A. Stüvel left a comment
Member

LGTM! Just one thing that can be handled when landing.

LGTM! Just one thing that can be handled when landing.
@ -1173,0 +1031,4 @@
* we're faithfully reproducing the original behavior.
*/
eInsertKeyFlags insert_key_flags_adjusted = insert_key_flags;
if (force_all && insert_key_flags & (INSERTKEY_REPLACE | INSERTKEY_AVAILABLE)) {

I think parentheses around insert_key_flags & (INSERTKEY_REPLACE | INSERTKEY_AVAILABLE) is still necessary. AFAIK some compilers will complain about ambiguity between & and &&.

I think parentheses around `insert_key_flags & (INSERTKEY_REPLACE | INSERTKEY_AVAILABLE)` is still necessary. AFAIK some compilers will complain about ambiguity between `&` and `&&`.
Author
Member

The operator precedence is unambiguous in C++ at least, but I agree that it would help readability a lot here, regardless.

The [operator precedence is unambiguous in C++](https://en.cppreference.com/w/cpp/language/operator_precedence) at least, but I agree that it would help readability a lot here, regardless.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-06-11 16:11:49 +02:00
Make if condition more immediately readable
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
61184ba9b6
Author
Member

@blender-bot build

@blender-bot build
Nathan Vegdahl merged commit 1a4f084806 into main 2024-06-11 16:43:08 +02:00
Nathan Vegdahl deleted branch combine_keying_functions 2024-06-11 16:43:11 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#122053
No description provided.