Anim: implement 3D viewport keyframing functionality for layered actions #121661
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
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, 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
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
Core
Module
Development Management
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
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#121661
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "nathanvegdahl/blender:baklava_3d_viewport_keyframing"
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?
(Continues and supersedes #119669, and split off from #120428)
This PR modifies the 3D viewport keyframing operators to work with layered
actions. The functionality is relative basic, and still leaves some things out.
Of particular note:
XYZ to RGB
flag and the keyframe interpolationtype are not yet used/respected.
an fcurve is deleted in a layered action, the fcurve still sticks around. This
is different from legacy actions, which delete fcurves when their last key is
deleted. Since the "Only Insert Available" feature is based on the existence
of fcurves, that makes the feature appear broken in some circumstances right
now with layered actions.
TODO
CombinedKeyingResult
is currently mostly stubbed out, and needs to be properly filled in.KeyframeStrip::keyframe_insert()
.For future PRs:
insert_key_rna()
currently keys all elements of an array property (e.g. object location), but should instead take each element of the property separately to allow individual keying. (#121879)insert_key_rna()
should likely be merged withinsert_keyframe()
. (#122053)XYZ to RGB
flag, the keyframe interpolation type, etc.[ ] Keying an object with an existing binding name but no action should create an action with a binding with that binding name, but currently does not. The unit test for this is inDiscussed this with @dr.sybren, and we decided that it's a bit subjective what the right behavior is here. So we're going to wait until people get some hands on experience with it before deciding what to do in this case.5405bce9a6
, but has been removed in this PR since it's currently failing.action_fcurve_ensure()
, which currently is only for legacy actions. There is some kind of refactor that we'll need to do here to make all of those call sites work with layered actions.INSERTKEY_NEEDED
flag 831c4952bfGENERIC_FAILURE
toUNKNOWN_FAILURE
d626e30f2cbinding_for_id()
was removed 691bab3ccacan_create_fcurve()
->key_insertion_may_create_fcurve()
f9476d438e*
instead of.value()
to access value ofstd::optional
05f2575e26@ -962,0 +1043,4 @@
* ensuring that at least one layer exists above.
*
* TODO: revisit this once those semantics are hammered out. */
if (layer == nullptr) {
Despite the comment, I'm not sure it makes sense to leave this null check in. I think there's a lot about the future structure of the code that we just don't know, and won't know until we actually start implementing the layered functionality. So this feels premature / trying to predict the future.
Thoughts?
I agree, it's probably better to change this to an assert + comment.
@ -977,2 +1106,3 @@
AnimData *adt = BKE_animdata_from_id(id);
bAction *action = id_action_ensure(bmain, id);
if (action == nullptr) {
Maybe this null check should just be an assert? Along the lines of the code comment below, I think a null value might be impossible/a bug here...?
Yes this should also be an assert.
id_action_ensure
only returns anulltptr
ifBKE_animdata_ensure_id
returns null.So we should be save here
WIP: Anim: implement 3D viewport keyframing functionality for the Animation data-blockto Anim: implement 3D viewport keyframing functionality for the Animation data-blockfrom my build
insert_key_legacy_action
should probably be const, right now it gives a warning because it's not been declared.in
insert_key_layered_action
line 1032 -> success is not used giving a warningAh, thanks for catching that!
Not sure why my compiler isn't emitting those warnings for me (just double-checked with a full rebuild). :-/
Just to double-check: I assume you meant
static
rather thanconst
?As for the warning on line 1032, I'm not sure what to do about that.
success
is used, but only in the assert right below it. Are you getting that warning with a release build?insert_key_layered_action()
static d9b0c82707(Gah... copy-paste error in the commit message. Oh well.. will be squashed anyway.)
The user preference flag
XYZ to RGB
isn't passed through. FCurve colors are rainbows when inserting keys.Also
Only insert Available
doesn't work (only for autokeying).Edit: not sure if those issues should be part of this PR though
The PR title should probably say
Layered Action
instead ofAnimation data-block
:)@ -370,0 +389,4 @@
*
* \return the index of the strip, or -1 if no strip was found at that time.
*/
int strip_index_at_time(float time) const;
something I realized while reading the code, nothing for this PR
We are assuming there is always only one strip at a given time. It will be very useful to slide strips into each other for blending. There are other ways of doing that probably, just wanted to mention it
Yeah, I was thinking about that as well. My rationale for doing it this way right now is basically twofold:
@ -962,0 +1046,4 @@
if (layer == nullptr) {
/* TODO: count the rna paths properly (e.g. accounting for multi-element
* properties). */
combined_result.add(SingleKeyingResult::NO_VALID_LAYER, rna_paths.size());
I think it might be better to change the meaning of
SingleKeyingResult
a bit.It was meant to represent errors that occur at the FCurve level, but
NO_VALID_LAYER
is above that so we can't give a meaningful count of FCurves that failed.I we re-interpret
SingleKeyingResult
as "One thing has failed during keying",NO_VALID_LAYER
could create an error message like "Could not add keyframes into n Layer(s) because ... " Which is probably more meaningful than counting the FCurves.Yeah, I think that's a great idea. Thanks!
Anim: implement 3D viewport keyframing functionality for the Animation data-blockto Anim: implement 3D viewport keyframing functionality for layered actionsThanks! Added them to the list of future PR todos. We could do those in this PR, but in the interest of getting this landed I think it's fine to leave those for future smaller PRs.
@ -119,0 +120,4 @@
* If the Action is empty, create a default layer with a single infinite
* keyframe strip.
*/
void ensure_layer();
Given
layer_add()
andlayer_remove()
, a better name would belayer_ensure()
.@ -502,2 +534,3 @@
float2 time_value,
const KeyframeSettings &settings);
const KeyframeSettings &settings,
const eInsertKeyFlags insert_key_flags);
remove the
const
fromeInsertKeyFlags
, it's without meaning here.Can we make
INSERTKEY_NOFLAGS
the default value for this parameter? I think that'll be nicer than having to pass this everywhere.@ -64,0 +79,4 @@
/**
* Increase the count of the given `SingleKeyingResult` by `count`.
*/
void add(SingleKeyingResult result, int count);
Can't this just be a single function with
int count=1
as default value? Or am I mixing up my languages?@ -184,1 +184,4 @@
void Action::ensure_layer()
{
if (this->layers().size() == 0) {
this->layers().is_empty()
-- no need to count all of them.@ -185,0 +186,4 @@
{
if (this->layers().size() == 0) {
/* TODO: default layer name localization. */
Layer &layer = this->layer_add("Layer");
DATA_("Layer")
should take care of the TODO.But do define the default name at the top of the file, close to
binding_default_name
.@ -397,0 +406,4 @@
Layer *Action::get_layer_for_keyframing()
{
/* TODO: handle multiple layers. */
if (this->layers().size() == 0) {
Same as before, use
is_empty()
.And I think it's fine here to print something to the terminal as a warning in case of multiple layers, because this situation shouldn't exist with the current limitations.
@ -853,0 +902,4 @@
* allow. */
FCurve *fcurve = key_insertion_may_create_fcurve(insert_key_flags) ?
&this->fcurve_find_or_create(binding, rna_path, array_index) :
this->fcurve_find(binding, rna_path, array_index);
I don't quite like this ternary here, because
fcurve_find_or_create()
will also callfcurve_find()
.Would be better to split up the 'or_create' part of that function into a separate one, so that the
fcurve_find_or_create()
is reduced to callingfcurve_find()
and then optionallyfcurve_create()
.This function can then just always call
fcurve_find()
, and then if it cannot be found, either create or return.I agree when looking only at this code. However, I think from an API standpoint a stand-alone
fcurve_create()
function is a little weird, which I assume is why you didn't already implement it.Specifically, what are the semantics of
fcurve_create()
if the fcurve already exists? Does it return nullptr? Does it overwrite the existing fcurve with an empty one? Or does it return the already existing fcurve... in which case then we're just back tofcurve_find_or_create()
but with a different name.So I think the existing API is already the right one: it provides all the capabilities we need, and is hard to misuse. I agree that it makes this code feel slightly weirder than it might otherwise, but it's not that bad and it lets us stick with the current (IMO good) API.
Overall it looks pretty good! Just a few small remarks.
@ -148,1 +153,4 @@
if (this->get_count(SingleKeyingResult::NO_VALID_LAYER) > 0) {
const int error_count = this->get_count(SingleKeyingResult::NO_VALID_LAYER);
errors.append(fmt::format(RPT_("Inserting keys on {:d} ID(s) has been skipped because there "
"ID" is not used in the interface, use "data-block".
@ -978,0 +1024,4 @@
return SingleKeyingResult::NO_VALID_STRIP;
}
/* TODO: morph key data based on Layer position in stack and Strip offset. */
Add a
BLI_assert(strip->is_infinite())
(and theStrip::is_infinite()
function ;-) )This code doesn't assume that the strip is infinite: if the strip doesn't overlap with the key insertion time, then it wouldn't be returned by
strip_at_time()
in the first place.However, it probably does make sense to assert that invariant, so I've added an assertion for that using the existing
Strip::contains_frame()
method.The code also currently assumes that strip time aligns with scene time (offset = 0), so I've added an assertion for that as well.
Hmmm not too thrilled to have three functions in the
Layer
class now, all to support a feature that's currently not implemented.I'd rather have all the relevant "assume 1-layer/1-strip" code in
insert_key_layer()
.Yeah, I think you're right. I'm trying to predict the future too much here. I'll collapse it down, and then we can figure out the right APIs once we're actually implementing the layered animation functionality.
@ -978,0 +1045,4 @@
Binding *binding = action.binding_for_handle(binding_handle);
if (binding == nullptr) {
binding = &action.binding_add();
const bool success = action.assign_id(binding, *id);
Add
UNUSED_VARS_NDEBUG(success);
Aside: that should probably be part of the definition of
BLI_assert()
and friends.@ -978,0 +1053,4 @@
}
action.ensure_layer();
Layer *layer = action.get_layer_for_keyframing();
This assumes that "ensure layer" creates one that will be returned by the "get layer" function, creating tightly coupled code. Why not have an
action.ensure_layer_for_keyframing()
that returns the appropriate layer?That would be semantically different than the current code (at least if
ensure_layer_for_keyframing()
would do what its name sounds like to me).Specifically,
ensure_layer_for_keyframing()
sounds to me like it ensures there is a layer that can accept the keys, creating one if no such layer exists. Whereas the current code creates a layer only if there are no layers at all, which I think(?) is what we want. For example, if there are already layers but they're all locked, the current code would fail to find a layer and subsequently report a keying error to the user, whereas code based on anensure_layer_for_keyframing()
function would automatically create another layer and insert the keys there.To me the current code is actually decoupled, because the concern of ensuring at least one layer and the concern of trying to find a layer appropriate for keying are separate.
I think the issue is just that there should be some white space between the two statements and a better name for the ensure method (maybe
layer_ensure_at_least_one()
) to better establish that these two statements aren't that closely related.Also, I think it would be good if I changed
get_layer_for_keyframing()
to return astd::optional<Layer &>
to make it explicit that it may not succeed, and harder to forget to handle that in the code that calls it.Damn it, you can't (conveniently) have
std::optional
s of references. Thanks C++.Okay, I made the other changes at least, and also attempted to make the docs of
get_layer_for_keyframing()
a little clearer.Creating a new layer when there is none sounds like a good idea.
When there is already a layer, but that cannot be used for keyframing (let's say there's an infinite simulation strip on there), I'm not convinced that Blender shouldn't just create a new layer for those keys.
This is a discussion for when we actually support layered animation, so let's keep the code as-is for now.
@ -978,0 +1079,4 @@
BLI_assert(rna_path_id_to_prop.has_value());
Vector<float> rna_values = get_keyframe_values(&ptr, prop, use_visual_keyframing);
for (int property_index : rna_values.index_range()) {
const int
-- theproperty_index
doesn't change within thefor
-body.code wise this looks good.
This would be a new feature as it's not currently supported. But indeed it would be nice to split that like we did for "Insert Needed".
Following things are not working, some mentioned in the PR description but noting for completeness.
@ChrisLend
Ooooh, I misunderstood your earlier comment. 🤦 I thought that's what you meant by this:
I had a brain fart and thought the "(only for autokeying)" bit meant "(you've currently only got it working for autokeying, but it should also work for manual)". Ha ha.
But you meant the issue you mentioned in the PR this was split off from, where the Only Insert Available code is based on the existence of fcurves rather than the existence of keys. Yeah, I'll add that to the PR notes, because that does indeed currently result in unexpected behavior. But based on the earlier discussion I think the right fix is to delete the fcurves when the last key is deleted, just like the current animation system does. Which is outside the scope of this PR.
I think that's covered by "Don't hard-code keyframing settings in the new animation system's keyframing code path." I'll rephrase that to make it clearer.
@dr.sybren I've addressed most of your comments exactly as you suggested. There are just a few that I addressed differently, or in one case that I think is best left as-is, and I've left those marked unresolved with replies explaining. Let me know what you think.
I've left some smaller inline notes.
Things seem to work well when toying around, but there is a distinct lack of unit tests.
@ -978,0 +1018,4 @@
if (strip == nullptr) {
return SingleKeyingResult::NO_VALID_STRIP;
}
BLI_assert(strip->contains_frame(key_data.position.x));
Either use
position.x
orposition[0]
, but not both in the same bit of code. I'd prefer.x
in both places.Oops! Thanks for catching that.
After addressing another of your comments those lines disappeared completely, though.
@ -978,0 +1042,4 @@
Binding *binding = action.binding_for_handle(binding_handle);
if (binding == nullptr) {
binding = &action.binding_add();
const bool success = action.assign_id(binding, *id);
This creates a binding named 'Binding'. It would be better if the new binding would get the name from
id->adt->binding_name
, orid->name
if not yet set.Check
Action::binding_add_for_id()
Layer
functions f4e97094b0Action::binding_add_for_id()
to add bindings for IDs c3613e63b8@dr.sybren I've addressed your comments so far aside from adding unit tests. Once we land #122553 I'll get to work on that.
There's one test I'm missing, and that's to test the situation where:
object.adt
is not NULL,object.adt.binding_name
is set to a non-empty string,The resulting binding should copy the name from
object.adt.binding_name
to ensure that the new Action and any previously-assigned Actions have consistent binding names for this object.@ -118,0 +155,4 @@
/* The action has a binding and it's assigned to the object. */
ASSERT_EQ(1, action.bindings().size());
Binding *binding = action.binding(0);
EXPECT_EQ(object->adt->binding_handle, binding->handle);
This should also check the binding name, as this is super important for re-assigning to other actions with automatic selection of the binding in that new action.
Also it could check that
object->adt->binding_name
is set correctly to the same name. This is not directly the responsibility of the function under test (the action+binding assignment code is responsible for that), but given that we have so few tests in Blender, I think it's nice to not only check "is assigned" but check "is assigned correctly".@ -118,0 +159,4 @@
/* We have the default layer and strip. */
ASSERT_TRUE(action.is_action_layered());
ASSERT_EQ(1, action.layers().size());
This should check the layer name. The exact name isn't that important here, but at least it shouldn't be empty.
@ -118,0 +172,4 @@
/* The fcurves in the channel bag are what we expect. */
EXPECT_EQ(1, channel_bag->fcurves().size());
const FCurve *fcurve = channel_bag->fcurve_find("rotation_mode", 0);
As discussed face-to-face, better to use
empty_display_size
as the property to animate, as that's actually afloat
property.@ -118,0 +411,4 @@
/* The action has a binding and it's assigned to the first object. */
ASSERT_EQ(1, action.bindings().size());
Binding *binding_1 = action.binding_for_handle(object->adt->binding_handle);
This should check the binding name, which should be the same as the object name.
@ -118,0 +426,4 @@
ASSERT_NE(nullptr, channel_bag_1);
/* Assign the action to the second object. */
armature_object->adt = BKE_animdata_ensure_id(&armature_object->id);
This should use
action->assign_id(nullptr, armature_object->id)
, to explicitly assign-but-not-bind the Action.@ -118,0 +441,4 @@
EXPECT_EQ(1, result_2.get_count(SingleKeyingResult::SUCCESS));
ASSERT_EQ(2, action.bindings().size());
Binding *binding_2 = action.binding_for_handle(armature_object->adt->binding_handle);
This should check the binding name against the object name.
empty_display_size
instead ofrotation_mode
dd01cb328cAction::assign_id()
33d80559ba@blender-bot build
LGTM, just a few small nags that can be addressed before landing.
@ -132,2 +132,3 @@
binding, "location", 0, {1.0f, 47.0f}, settings, INSERTKEY_NOFLAGS);
another_strip.as<KeyframeStrip>().keyframe_insert(
binding, "location", 0, {1.0f, 47.0f}, settings);
binding, "location", 0, {1.0f, 47.0f}, settings, INSERTKEY_NOFLAGS);
There is no more need to pass
INSERTKEY_NOFLAGS
here, as that's already the parameter's default value. It might be a nice touch to useINSERTKEY_NOFLAGS
explicitly in one or two new calls tokeyframe_insert()
just to be sure that that use is covered as well. But there's no need for this PR to modify existing test code just to add this parameter.I think that must have slipped in with a merge or something. Thanks for catching that!
@ -118,0 +139,4 @@
* - Infinite KeyframeStrip
* - FCurve with a single key
*/
object->empty_drawsize = 1.0;
Use different values for the frame number and the property value. Right now these two could be swapped, and the test wouldn't fail.
For tests I always try and avoid the trivial numbers, and go for, say, frame=47 and value=0.327 (just to have some references to my two favourite agents in there).
Also
empty_drawsize = 1.0
is actually setting it to its default value. Just for test resilience, I'd use something different for that reason as well.Same for the tests in
source/blender/animrig/intern/keyframing_test.cc
@ -118,0 +156,4 @@
* to the object. */
ASSERT_EQ(1, action.bindings().size());
Binding *binding = action.binding(0);
EXPECT_EQ(0, strcmp(object->id.name, binding->name));
Use
EXPECT_STREQ()
, or wrapchar*
instd::string
orStringRef
and useEXPECT_EQ(A, B)
. That way, when there is a test failure, the actual+expected names are shown, rather than "0 is not 1".EXPECT_STREQ()
for string equality tests aac5783a0f@blender-bot build