Anim: implement 3D viewport keyframing functionality for layered actions #121661

Merged
Nathan Vegdahl merged 76 commits from nathanvegdahl/blender:baklava_3d_viewport_keyframing into main 2024-06-07 13:26:30 +02:00
Member

(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:

  • Keyframing with keying sets does not yet work.
  • User preferences such as the XYZ to RGB flag and the keyframe interpolation
    type are not yet used/respected.
  • Something not caused by this PR but revealed by it: when the last keyframe of
    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

  • Need to check the experimental flag before inserting keys to the Animation datablock.
  • Error collection via CombinedKeyingResult is currently mostly stubbed out, and needs to be properly filled in.
  • Error collection isn't done properly yet in the lower-level keying functions, like KeyframeStrip::keyframe_insert().
    • Note that even lower-level keying functions that predate the new animation system still don't propagate errors properly yet. Threading error propagation through the entire keyframing stack will be a separate project.
  • Some Dependency Graph update is missing because you need to move keys before they affect the viewport.
  • While auto-keying works, the flag check for "Only insert needed" and "Only insert available" does not.

For future PRs:

  • Refactor: 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)
  • Refactor: insert_key_rna() should likely be merged with insert_keyframe(). (#122053)
  • Pass the user preferences' keyframing settings, rather than hard coding them like they are right now. For example, the 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 in 5405bce9a6, but has been removed in this PR since it's currently failing. Discussed 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.
  • Keyingsets do not work yet. (Addressed by #122053.)
  • A bunch of misc places in Blender call 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.
(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: - Keyframing with keying sets does not yet work. - User preferences such as the `XYZ to RGB` flag and the keyframe interpolation type are not yet used/respected. - Something not caused by this PR but revealed by it: when the last keyframe of 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 - [x] Need to check the experimental flag before inserting keys to the Animation datablock. - [x] Error collection via `CombinedKeyingResult` is currently mostly stubbed out, and needs to be properly filled in. - [x] Error collection isn't done properly yet in the lower-level keying functions, like `KeyframeStrip::keyframe_insert()`. - Note that even lower-level keying functions that predate the new animation system still don't propagate errors properly yet. Threading error propagation through the entire keyframing stack will be a separate project. - [x] Some Dependency Graph update is missing because you need to move keys before they affect the viewport. - [x] While auto-keying works, the flag check for "Only insert needed" and "Only insert available" does not. ## For future PRs: - [x] Refactor: `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) - [x] Refactor: `insert_key_rna()` should likely be merged with `insert_keyframe()`. (#122053) - [x] Pass the user preferences' keyframing settings, rather than hard coding them like they are right now. For example, the `XYZ to RGB` flag, the keyframe interpolation type, etc. - #123006 - #123022 - ~~[ ] 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 in 5405bce9a66605e83dfe8d35eafbe99abb9f030d, but has been removed in this PR since it's currently failing.~~ Discussed 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. - [x] Keyingsets do not work yet. (Addressed by #122053.) - [ ] A bunch of misc places in Blender call `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.
Nathan Vegdahl added 24 commits 2024-05-10 17:57:34 +02:00
So far just a port of #119669 to latest main.
This isn't 100% there yet, but it's a good start.
This still leaves even lower-level code not returning propert failure
results, but one step at a time.
In the last commit I made `insert_vert_fcurves()` actually use
the `INSERTKEY_NEEDED` flag, whereas previously it was ignored.
However, there was a call site that was already passing
`INSERTKEY_NEEDED` to `insert_vert_fcurve()` despite it previously
having no effect.  In order to preserve behavior, this commit replaces
that with `INSERTKEY_NOFLAGS`.
At this point it was literally the same signature, and just calling
the identical method on KeyframeStrip.  We can always reintroduce it
in the future when it makes sense.
Misc cleanup
Some checks failed
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.
733a8d2632
This helps make it feel a little worse to use, which is good because
we want to remove it in the future (as explained in the comment).
Nathan Vegdahl added 1 commit 2024-05-13 18:24:30 +02:00
Nathan Vegdahl added 1 commit 2024-05-13 18:26:46 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 11:13:37 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 11:15:34 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 11:34:02 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 11:37:02 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 11:41:01 +02:00
Nathan Vegdahl added 2 commits 2024-05-14 13:03:23 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 14:41:02 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 14:48:36 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 16:19:38 +02:00
This particular case shouldn't be able to happen, so if we reach it
it's a bug.
Nathan Vegdahl added 1 commit 2024-05-14 16:26:52 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 16:36:39 +02:00
Another place where the non-null check failing would indicate a bug.
Nathan Vegdahl added 1 commit 2024-05-14 16:43:51 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 16:47:42 +02:00
Nathan Vegdahl added 1 commit 2024-05-14 17:49:27 +02:00
Nathan Vegdahl added 2 commits 2024-05-16 14:18:14 +02:00
Also move ensuring a default layer with infinite strip to its own method
on Action as well.
Nathan Vegdahl added 1 commit 2024-05-16 14:24:49 +02:00
Nathan Vegdahl reviewed 2024-05-16 14:49:45 +02:00
@ -962,0 +1043,4 @@
* ensuring that at least one layer exists above.
*
* TODO: revisit this once those semantics are hammered out. */
if (layer == nullptr) {
Author
Member

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?

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.

I agree, it's probably better to change this to an assert + comment.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl reviewed 2024-05-16 14:51:48 +02:00
Nathan Vegdahl added 1 commit 2024-05-16 15:02:06 +02:00
Not sure if this is the best/most logical order, but it least it
keeps things predictable.
Nathan Vegdahl reviewed 2024-05-16 15:09:24 +02:00
@ -977,2 +1106,3 @@
AnimData *adt = BKE_animdata_from_id(id);
bAction *action = id_action_ensure(bmain, id);
if (action == nullptr) {
Author
Member

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...?

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 a nulltptr if BKE_animdata_ensure_id returns null.
So we should be save here

Yes this should also be an assert. `id_action_ensure` only returns a `nulltptr` if `BKE_animdata_ensure_id` returns null. So we should be save here
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl changed title from WIP: Anim: implement 3D viewport keyframing functionality for the Animation data-block to Anim: implement 3D viewport keyframing functionality for the Animation data-block 2024-05-16 15:11:38 +02:00
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-05-16 15:11:53 +02:00
Nathan Vegdahl requested review from Christoph Lendenfeld 2024-05-16 15:12:08 +02:00

from 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 warning

from 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 warning
Author
Member

Ah, thanks for catching that!

Not sure why my compiler isn't emitting those warnings for me (just double-checked with a full rebuild). :-/

Ah, thanks for catching that! Not sure why my compiler isn't emitting those warnings for me (just double-checked with a full rebuild). :-/
Author
Member

Just to double-check: I assume you meant static rather than const?

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?

Just to double-check: I assume you meant `static` rather than `const`? 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?
Nathan Vegdahl added 1 commit 2024-05-16 15:40:18 +02:00
Author
Member

(Gah... copy-paste error in the commit message. Oh well.. will be squashed anyway.)

(Gah... copy-paste error in the commit message. Oh well.. will be squashed anyway.)
Christoph Lendenfeld requested changes 2024-05-16 16:29:57 +02:00
Dismissed
Christoph Lendenfeld left a comment
Member

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 of Animation data-block :)

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 of `Animation 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

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
Author
Member

Yeah, I was thinking about that as well. My rationale for doing it this way right now is basically twofold:

  • For now we're only handling a single strip anyway, and when we want to change that it will be easy to track down the places we need to change by following the compiler errors that come from changing the return type of this function.
  • Even when we do add support for blending strips, this API might not actually change (aside from the name), since ultimately we need to select one strip to insert into anyway, even when there's overlap.
Yeah, I was thinking about that as well. My rationale for doing it this way right now is basically twofold: - For now we're only handling a single strip anyway, and when we want to change that it will be easy to track down the places we need to change by following the compiler errors that come from changing the return type of this function. - Even when we do add support for blending strips, this API might not actually change (aside from the name), since ultimately we need to select one strip to insert into anyway, even when there's overlap.
nathanvegdahl marked this conversation as resolved
@ -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.

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.
Author
Member

Yeah, I think that's a great idea. Thanks!

Yeah, I think that's a great idea. Thanks!
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl changed title from Anim: implement 3D viewport keyframing functionality for the Animation data-block to Anim: implement 3D viewport keyframing functionality for layered actions 2024-05-16 17:12:03 +02:00
Nathan Vegdahl added 3 commits 2024-05-21 14:54:35 +02:00
Nathan Vegdahl added 1 commit 2024-05-21 15:14:05 +02:00
As opposed to per key, which is more challenging to report, and
probably not meaningfully more useful to the user in those cases
anyway.
Author
Member

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).

Thanks! 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.

> 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). Thanks! 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.
Nathan Vegdahl added 2 commits 2024-05-30 15:59:30 +02:00
Nathan Vegdahl requested review from Christoph Lendenfeld 2024-05-30 16:12:55 +02:00
Sybren A. Stüvel reviewed 2024-05-30 16:35:58 +02:00
@ -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() and layer_remove(), a better name would be layer_ensure().

Given `layer_add()` and `layer_remove()`, a better name would be `layer_ensure()`.
nathanvegdahl marked this conversation as resolved
@ -502,2 +534,3 @@
float2 time_value,
const KeyframeSettings &settings);
const KeyframeSettings &settings,
const eInsertKeyFlags insert_key_flags);

remove the const from eInsertKeyFlags, 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.

remove the `const` from `eInsertKeyFlags`, 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.
nathanvegdahl marked this conversation as resolved
@ -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?

Can't this just be a single function with `int count=1` as default value? Or am I mixing up my languages?
nathanvegdahl marked this conversation as resolved
@ -184,1 +184,4 @@
void Action::ensure_layer()
{
if (this->layers().size() == 0) {

this->layers().is_empty() -- no need to count all of them.

if (this->layers().is_empty()) {
  return;
}
... the rest
`this->layers().is_empty()` -- no need to count all of them. ```cpp if (this->layers().is_empty()) { return; } ... the rest ```
nathanvegdahl marked this conversation as resolved
@ -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.

`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`.
nathanvegdahl marked this conversation as resolved
@ -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.

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.
nathanvegdahl marked this conversation as resolved
@ -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 call fcurve_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 calling fcurve_find() and then optionally fcurve_create().

This function can then just always call fcurve_find(), and then if it cannot be found, either create or return.

I don't quite like this ternary here, because `fcurve_find_or_create()` will also call `fcurve_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 calling `fcurve_find()` and then optionally `fcurve_create()`. This function can then just always call `fcurve_find()`, and then if it cannot be found, either create or return.
Author
Member

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 to fcurve_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.

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 to `fcurve_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.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel requested changes 2024-05-30 17:03:19 +02:00
Sybren A. Stüvel left a comment
Member

Overall it looks pretty good! Just a few small remarks.

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".

"ID" is not used in the interface, use "data-block".
nathanvegdahl marked this conversation as resolved
@ -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 the Strip::is_infinite() function ;-) )

Add a `BLI_assert(strip->is_infinite())` (and the `Strip::is_infinite()` function ;-) )
Author
Member

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.

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().

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()`.
Author
Member

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.

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.
nathanvegdahl marked this conversation as resolved
@ -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);

Add `UNUSED_VARS_NDEBUG(success);`
Author
Member

Aside: that should probably be part of the definition of BLI_assert() and friends.

Aside: that should probably be part of the definition of `BLI_assert()` and friends.
nathanvegdahl marked this conversation as resolved
@ -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?

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?
Author
Member

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 an ensure_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.

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 an `ensure_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.
Author
Member

Also, I think it would be good if I changed get_layer_for_keyframing() to return a std::optional<Layer &> to make it explicit that it may not succeed, and harder to forget to handle that in the code that calls it.

Also, I think it would be good if I changed `get_layer_for_keyframing()` to return a `std::optional<Layer &>` to make it explicit that it may not succeed, and harder to forget to handle that in the code that calls it.
Author
Member
Damn it, you [can't (conveniently) have `std::optional`s of references](https://stackoverflow.com/questions/26858034/why-does-stdoptional-not-have-a-specialization-for-reference-types). Thanks C++.
Author
Member

Okay, I made the other changes at least, and also attempted to make the docs of get_layer_for_keyframing() a little clearer.

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.

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.
nathanvegdahl marked this conversation as resolved
@ -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 -- the property_index doesn't change within the for-body.

`const int` -- the `property_index` doesn't change within the `for`-body.
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld approved these changes 2024-05-31 11:03:23 +02:00
Christoph Lendenfeld left a comment
Member

code wise this looks good.

Get Only insert Available working for manual keying.

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.

  • Only Insert Available: when creating keys, then deleting them and then modifying the object again, the setting doesn't work. Probably because empty FCurves are not deleted
  • XYZ to RGB. You mention that in the PR description.
  • user preference interpolation mode isn't respected
code wise this looks good. > Get Only insert Available working for manual keying. 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. * Only Insert Available: when creating keys, then deleting them and then modifying the object again, the setting doesn't work. Probably because empty FCurves are not deleted * XYZ to RGB. You mention that in the PR description. * user preference interpolation mode isn't respected
Nathan Vegdahl added 1 commit 2024-05-31 12:36:50 +02:00
Nathan Vegdahl added 2 commits 2024-05-31 12:46:10 +02:00
Nathan Vegdahl added 1 commit 2024-05-31 13:45:25 +02:00
Nathan Vegdahl added 1 commit 2024-05-31 14:43:41 +02:00
Author
Member

@ChrisLend

This would be a new feature as it's not currently supported.

Ooooh, I misunderstood your earlier comment. 🤦 I thought that's what you meant by this:

Also Only insert Available doesn't work (only for autokeying).

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.

user preference interpolation mode isn't respected

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.

@ChrisLend > This would be a new feature as it's not currently supported. Ooooh, I misunderstood your earlier comment. 🤦 I thought that's what you meant by this: > Also Only insert Available doesn't work (only for autokeying). 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](https://projects.blender.org/blender/blender/pulls/120428#issuecomment-1180470), 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](https://projects.blender.org/blender/blender/pulls/120428#issuecomment-1183529) 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. > user preference interpolation mode isn't respected 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.
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-05-31 15:32:27 +02:00
Author
Member

@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.

@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.
Sybren A. Stüvel requested changes 2024-05-31 16:46:52 +02:00
Sybren A. Stüvel left a comment
Member

I've left some smaller inline notes.

Things seem to work well when toying around, but there is a distinct lack of unit tests.

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 or position[0], but not both in the same bit of code. I'd prefer .x in both places.

Either use `position.x` or `position[0]`, but not both in the same bit of code. I'd prefer `.x` in both places.
Author
Member

Oops! Thanks for catching that.

After addressing another of your comments those lines disappeared completely, though.

Oops! Thanks for catching that. After addressing another of your comments those lines disappeared completely, though.
nathanvegdahl marked this conversation as resolved
@ -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, or id->name if not yet set.

This creates a binding named 'Binding'. It would be better if the new binding would get the name from `id->adt->binding_name`, or `id->name` if not yet set.

Check Action::binding_add_for_id()

Check `Action::binding_add_for_id()`
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 2 commits 2024-06-03 19:03:27 +02:00
Just encode the current assumptions directly, rather than trying to
predict the API's future.

Also improve a few related comments.
Nathan Vegdahl added 2 commits 2024-06-03 19:13:14 +02:00
Author
Member

@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.

@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.
Nathan Vegdahl added 2 commits 2024-06-04 17:53:02 +02:00
Nathan Vegdahl added 1 commit 2024-06-04 18:06:04 +02:00
Nathan Vegdahl added 1 commit 2024-06-04 18:09:31 +02:00
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-06-06 13:44:21 +02:00
Sybren A. Stüvel requested changes 2024-06-06 15:24:10 +02:00
Sybren A. Stüvel left a comment
Member

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,
  • a key is inserted, creating a new Action + Binding

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.

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, - a key is inserted, creating a new Action + Binding 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".

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".
nathanvegdahl marked this conversation as resolved
@ -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.

This should check the layer name. The exact name isn't that important here, but at least it shouldn't be empty.
nathanvegdahl marked this conversation as resolved
@ -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 a float property.

As discussed face-to-face, better to use `empty_display_size` as the property to animate, as that's actually a `float` property.
nathanvegdahl marked this conversation as resolved
@ -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.

This should check the binding name, which should be the same as the object name.
nathanvegdahl marked this conversation as resolved
@ -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.

This should use `action->assign_id(nullptr, armature_object->id)`, to explicitly assign-but-not-bind the Action.
nathanvegdahl marked this conversation as resolved
@ -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.

This should check the binding name against the object name.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-06-06 15:39:21 +02:00
Nathan Vegdahl added 1 commit 2024-06-06 15:48:32 +02:00
Nathan Vegdahl added 1 commit 2024-06-06 15:53:04 +02:00
Nathan Vegdahl added 1 commit 2024-06-06 16:07:22 +02:00
Nathan Vegdahl added 1 commit 2024-06-06 16:25:46 +02:00
Currently failing.  Will fix in a future commit.
Nathan Vegdahl added 1 commit 2024-06-06 16:40:42 +02:00
Nathan Vegdahl added 1 commit 2024-06-06 16:49:02 +02:00
Remove test for object with existing binding name but no assigned action
All checks were successful
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
f7f5072ff8
It was failing due to code unrelated to this PR, and will be addressed
in a futrue PR.
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-06-06 16:49:25 +02:00
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel approved these changes 2024-06-07 11:42:22 +02:00
Sybren A. Stüvel left a comment
Member

LGTM, just a few small nags that can be addressed before landing.

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 use INSERTKEY_NOFLAGS explicitly in one or two new calls to keyframe_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.

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 use `INSERTKEY_NOFLAGS` explicitly in one or two new calls to `keyframe_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.
Author
Member

I think that must have slipped in with a merge or something. Thanks for catching that!

I think that must have slipped in with a merge or something. Thanks for catching that!
nathanvegdahl marked this conversation as resolved
@ -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

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`
nathanvegdahl marked this conversation as resolved
@ -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 wrap char* in std::string or StringRef and use EXPECT_EQ(A, B). That way, when there is a test failure, the actual+expected names are shown, rather than "0 is not 1".

Use `EXPECT_STREQ()`, or wrap `char*` in `std::string` or `StringRef` and use `EXPECT_EQ(A, B)`. That way, when there is a test failure, the actual+expected names are shown, rather than "0 is not 1".
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-06-07 12:00:29 +02:00
Nathan Vegdahl added 1 commit 2024-06-07 12:17:00 +02:00
Nathan Vegdahl added 1 commit 2024-06-07 12:17:25 +02:00
Nathan Vegdahl added 1 commit 2024-06-07 12:36:32 +02:00
Use more unique test values
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-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
9876ce6f2a
Author
Member

@blender-bot build

@blender-bot build
Nathan Vegdahl merged commit a36599b323 into main 2024-06-07 13:26:30 +02:00
Nathan Vegdahl deleted branch baklava_3d_viewport_keyframing 2024-06-07 13:26:32 +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
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
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#121661
No description provided.