Refactor: combine insert_keyframe() and insert_key_rna() into a single function #122053
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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 & 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
Viewport & EEVEE
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
Asset System
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#122053
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "nathanvegdahl/blender:combine_keying_functions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Based on discussions with @ChrisLend
The goal of this PR is to merge
insert_keyframe()
andinsert_key_rna()
intoa single function,
insert_keyframes()
, that fully accommodates thefunctionality 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
andanim_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.
RNAPath
type for conveniently storing and passing rna paths 62e8d00a4finsert_key_rna()
takeRNAPath
's instead of strings e5a812db79insert_key_rna()
's arguments to be consistent withinsert_keyframe()
e697a312fbinsert_key_rna()
to take an ID 1c5267a2fbinsert_key_rna()
was missing frominsert_keyframe()
d6b6638ad9insert_keyframe()
to useinsert_key_rna()
instead 1142b46536insert_key_rna()
and removeinsert_keyframe()
23bb1ddd8ainsert_key_rna()
->insert_keyframes()
1ee34fab41looks 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'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 whereasinsert_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
@ -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?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.
@ -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.
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.force_all
flag 6f7e4b00f8WIP: Refactor: Anim: combine insert_keyframe() and insert_key_rna() into a single functionto Refactor: Anim: combine insert_keyframe() and insert_key_rna() into a single functionOkay, finally ready for a proper review! I'll start another PR (to be merged first) with unit tests that cover the relevant behavior.
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.
RNAPointer *
instead ofID *
toinsert_keyframes()
45541aef8cOkay, fixed! Turns out we have to use
PointerRNA *
instead ofID *
as the input toinsert_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
good job, really liking where this is going
@nathanvegdahl almost forgot, but the title should either be
Anim:
ORRefactor:
iirc there was some discussion about this in blender-coders a while ago. Some script doesn't work correctly if there is both
Ah! Thanks, didn't realize that.
Refactor: Anim: combine insert_keyframe() and insert_key_rna() into a single functionto Refactor: combine insert_keyframe() and insert_key_rna() into a single functionMost 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.Ah! Thanks for catching that. I wrote those docs when I thought
AnimationEvalContext
represented something more fundamental/core than it actually does.@ -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 thefloat time
parameter means here, or why it is necessary.Also it doesn't seem to be modified in the function, so
const
.I agree this is redundant. The reason for both to exist right now is:
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.insert_keyframe()
had no separate frame parameter, butinsert_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 ;-)
Will do. I also left a todo in the doc comment about it now.
@ -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 alsoanim_eval_context.eval_time
?@ -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.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: #123081I've now removed the output parameter descriptions, leaving that to #123081. I also renamed the
elements_to_key
variable torna_values_mask
, to make its relationship torna_values
more obvious.@ -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 toat_least_one_would_succeed
.@ -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.@ -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. */
👍
@ -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 asanim_eval_context
, and so a mistake in which one is picked will not show up in this test.Good point. But since the intent is to remove the redundancy in a future PR anyway, I think it's fine for now.
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&&
.The operator precedence is unambiguous in C++ at least, but I agree that it would help readability a lot here, regardless.
@blender-bot build