Anim: Operator to convert a legacy action to a layered action #122043

Merged
Christoph Lendenfeld merged 37 commits from ChrisLend/blender:action_convert_operator into main 2024-07-11 11:57:44 +02:00

This PR creates an operator to convert a legacy Action into a layered Action.
No data is destroyed. The operator creates a new datablock.
The conversion to the layered Action is lossless.

This PR creates an operator to convert a `legacy Action` into a `layered Action`. No data is destroyed. The operator creates a new datablock. The conversion to the `layered Action` is lossless.
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2024-05-21 12:02:50 +02:00
Christoph Lendenfeld added 2 commits 2024-05-21 12:03:02 +02:00
Christoph Lendenfeld added 1 commit 2024-05-21 14:40:04 +02:00
Christoph Lendenfeld added 1 commit 2024-05-21 15:43:04 +02:00
Christoph Lendenfeld added 2 commits 2024-05-23 15:03:17 +02:00
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-05-28 10:35:11 +02:00
Sybren A. Stüvel requested review from Sybren A. Stüvel 2024-05-28 10:35:12 +02:00
Sybren A. Stüvel approved these changes 2024-05-28 10:40:44 +02:00
Dismissed
@ -105,0 +112,4 @@
char layered_action_name[MAX_ID_NAME - 2];
SNPRINTF(layered_action_name, "%s_layered", legacy_action.id.name);
bAction *baction = BKE_action_add(&bmain, layered_action_name);

This would be a nice case for a unit test, to see what happens when an Action with a maximum-length name gets converted.

This would be a nice case for a unit test, to see what happens when an Action with a maximum-length name gets converted.
ChrisLend marked this conversation as resolved
@ -105,0 +121,4 @@
KeyframeStrip &key_strip = strip.as<KeyframeStrip>();
LISTBASE_FOREACH (FCurve *, fcu, &legacy_action.curves) {
FCurve &new_fcu = key_strip.fcurve_find_or_create(binding, fcu->rna_path, fcu->array_index);

I think it's better to use a BKE function to copy the FCurve, and then use a different function to directly assign it to the right channelbag. That'll keep the "how to copy an FCurve" logic out of this function.

I think it's better to use a BKE function to copy the FCurve, and then use a different function to directly assign it to the right channelbag. That'll keep the "how to copy an FCurve" logic out of this function.
Author
Member

good point.
Do you plan to move the code from intern/animation.cc to intern/action.cc?
Reason being that I'd like to use grow_array_and_append but that's a static function in animation.cc

good point. Do you plan to move the code from `intern/animation.cc` to `intern/action.cc`? Reason being that I'd like to use `grow_array_and_append` but that's a static function in `animation.cc`

Good point, I'll do that ASAP.

Good point, I'll do that ASAP.

Done in #122480 -- I'll land it when the buildbot is green.

Done in #122480 -- I'll land it when the buildbot is green.
ChrisLend marked this conversation as resolved
@ -707,0 +739,4 @@
if (!object) {
return false;
}
return true;

This should probably have the check for whether it has an action.

This should probably have the check for whether it has an action.
ChrisLend marked this conversation as resolved
@ -707,0 +749,4 @@
ot->idname = "ANIM_OT_convert_action";
ot->description =
"Convert between layered and legacy action on the active object. The conversion from "
"layered to legacy is lossy";

It's probably better to have two operators for this, one for legacy → layered, and one for the opposite. That'll make it easier to put more checks in the poll function, show an explicit operator name, etc.

It's probably better to have two operators for this, one for legacy → layered, and one for the opposite. That'll make it easier to put more checks in the poll function, show an explicit operator name, etc.
ChrisLend marked this conversation as resolved
Sybren A. Stüvel requested changes 2024-05-28 10:41:26 +02:00
Dismissed
Sybren A. Stüvel left a comment
Member

See, that's what happens when I review while mildly ill at home, I start accepting WIP PRs ;-)

See, that's what happens when I review while mildly ill at home, I start accepting WIP PRs ;-)
Christoph Lendenfeld added 3 commits 2024-05-30 15:15:02 +02:00
Christoph Lendenfeld added 1 commit 2024-05-30 15:31:13 +02:00
Christoph Lendenfeld changed title from WIP: Anim: Operator to convert to and from legacy action to WIP: Anim: Operator to convert a legacy action to a layered action 2024-05-30 15:58:03 +02:00
Christoph Lendenfeld added 1 commit 2024-05-30 17:18:17 +02:00
Christoph Lendenfeld added 2 commits 2024-05-31 09:47:38 +02:00
Christoph Lendenfeld added 1 commit 2024-05-31 09:48:40 +02:00
Christoph Lendenfeld changed title from WIP: Anim: Operator to convert a legacy action to a layered action to Anim: Operator to convert a legacy action to a layered action 2024-05-31 09:48:47 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-05-31 09:48:54 +02:00
Sybren A. Stüvel requested changes 2024-06-03 14:29:07 +02:00
Dismissed
@ -1076,0 +1086,4 @@
char max_legacy_name[MAX_ID_NAME - 8];
SNPRINTF(max_legacy_name, "%s", legacy_action.id.name);
/* Offsetting the id.name to remove the ID prefix (AC) which gets added back later. */
SNPRINTF(layered_action_name, "%s_layered", max_legacy_name + 2);

I have the feeling that std::string and/or fmt::format() may make this code a bit easier to grok.

I have the feeling that `std::string` and/or `fmt::format()` may make this code a bit easier to grok.
ChrisLend marked this conversation as resolved
@ -1076,0 +1090,4 @@
bAction *baction = BKE_action_add(&bmain, layered_action_name);
Action &converted_action = baction->wrap();
Binding &binding = converted_action.binding_add();

This will create a binding named "Binding", which is yet another thing that will have to be managed / renamed. I also commented on this in Nathan's 3D Viewport keying PR, which has the same logic. I think we just need some utility function "create binding for this ID", which can then do the right thing. I'll work on that.

This will create a binding named "Binding", which is yet another thing that will have to be managed / renamed. I also commented on this in [Nathan's 3D Viewport keying PR](https://projects.blender.org/blender/blender/pulls/121661/files#issuecomment-1201395), which has the same logic. I think we just need some utility function "create binding for this ID", which can then do the right thing. I'll work on that.

Hah, already did but forgot about it, see Action::binding_add_for_id() ;-)

Hah, already did but forgot about it, see `Action::binding_add_for_id()` ;-)
Author
Member

I had a look at this. Using binding_add_for_id requires to pass in an object which I would like to avoid. Not every legacy action is always bound to an object. I'd like to keep the assumption that we are operating on an object at the operator level.

Let me know what you think

I had a look at this. Using `binding_add_for_id` requires to pass in an object which I would like to avoid. Not every legacy action is always bound to an object. I'd like to keep the assumption that we are operating on an object at the operator level. Let me know what you think
ChrisLend marked this conversation as resolved
@ -1076,0 +1093,4 @@
Binding &binding = converted_action.binding_add();
Layer &layer = converted_action.layer_add(legacy_action.id.name);
Strip &strip = layer.strip_add(Strip::Type::Keyframe);
KeyframeStrip &key_strip = strip.as<KeyframeStrip>();

I'll write a templated function for this, as I've seen the same pattern in my own code as well (creating a generic Strip, which is then only used to cast into the desired type).

I'll write a templated function for this, as I've seen the same pattern in my own code as well (creating a generic Strip, which is then only used to cast into the desired type).
ChrisLend marked this conversation as resolved
@ -472,0 +495,4 @@
ASSERT_TRUE(bag->fcurve_array[1]->modifiers.first != nullptr);
Action *long_name_action = static_cast<Action *>(BKE_id_new(
bmain, ID_AC, "name_for_an_action_that_is_exactly_64_chars_which_is_MAX_ID_NAME"));

*chef's kiss* for the name.

And no, I won't point out that the 64 chars should include the trailing \0 byte, and that this name will thus be truncated. It is too perfect to be bothered by such trivialities.

`*`chef's kiss`*` for the name. And no, I won't point out that the 64 chars should include the trailing `\0` byte, and that this name will thus be truncated. It is too perfect to be bothered by such trivialities.
ChrisLend marked this conversation as resolved
@ -749,0 +751,4 @@
using namespace blender;
Object *object = CTX_data_active_object(C);
if (!object) {

I think this & the next nullability checks can be removed, as the exec() shouldn't be called if the poll() returns false. Just let Blender crash to show there's a bug. It'll make the code nice & compact.

I think this & the next nullability checks can be removed, as the `exec()` shouldn't be called if the `poll()` returns false. Just let Blender crash to show there's a bug. It'll make the code nice & compact.
Author
Member

In that case you might also want to remove those checks from binding_unassign_object_exec ;)

In that case you might also want to remove those checks from `binding_unassign_object_exec` ;)
ChrisLend marked this conversation as resolved
@ -749,0 +762,4 @@
animrig::Action &anim = adt->action->wrap();
if (anim.is_empty()) {
return OPERATOR_CANCELLED;

This should report a warning that it's empty, and thus already considered to be a layered Action.

This should report a warning that it's empty, and thus already considered to be a layered Action.
ChrisLend marked this conversation as resolved
@ -749,0 +764,4 @@
if (anim.is_empty()) {
return OPERATOR_CANCELLED;
}
Main *bmain = CTX_data_main(C);

This line can be moved down to right above the call that needs the bmain.

This line can be moved down to right above the call that needs the `bmain`.
dr.sybren marked this conversation as resolved
@ -749,0 +766,4 @@
}
Main *bmain = CTX_data_main(C);
if (anim.is_action_layered()) {
BKE_report(op->reports, RPT_WARNING, "Action is already a layered type");

"a layered type" → "layered", as there won't be (at least not until we finish this one) be multiple layerednesses that would warrant introducing a "type".

"a layered type" → "layered", as there won't be (at least not until we finish this one) be multiple layerednesses that would warrant introducing a "type".
dr.sybren marked this conversation as resolved
@ -749,0 +786,4 @@
if (!adt || !adt->action) {
return false;
}

I think this could get a check for layeredness as well, something like

if (adt->action.wrap().is_action_layered()) {
  CTX_poll_message_set(C, "Action is already layered");
  return false;
}
I think this could get a check for layeredness as well, something like ```cpp if (adt->action.wrap().is_action_layered()) { CTX_poll_message_set(C, "Action is already layered"); return false; } ```
Author
Member

did that and also removed the same check in the exec function.

did that and also removed the same check in the exec function.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel reviewed 2024-06-03 14:41:10 +02:00
@ -749,0 +795,4 @@
/* identifiers */
ot->name = "Convert to Layered Action";
ot->idname = "ANIM_OT_convert_to_layered_action";
ot->description = "Convert a legacy Action to a layered Action on the active object.";

Remove the tailing period.

Remove the tailing period.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel reviewed 2024-06-03 14:42:35 +02:00
@ -749,0 +770,4 @@
return OPERATOR_CANCELLED;
}
animrig::convert_to_layered_action(*bmain, anim);

The newly created Action should get assigned to the data-block, so that it's visible in the GUI what happened.

The newly created Action should get assigned to the data-block, so that it's visible in the GUI what happened.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel reviewed 2024-06-03 14:46:02 +02:00
@ -472,0 +483,4 @@
insert_vert_fcurve(legacy_fcu_0, {1, 1}, settings, INSERTKEY_NOFLAGS);
add_fmodifier(&legacy_fcu_1->modifiers, FMODIFIER_TYPE_NOISE, legacy_fcu_1);
Action *converted = convert_to_layered_action(*bmain, *anim);

This could also assert that converted != anim, just to be sure.

This could also assert that `converted != anim`, just to be sure.
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-06-11 13:09:09 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-11 13:10:33 +02:00
Sybren A. Stüvel requested changes 2024-06-14 11:44:42 +02:00
Dismissed
Sybren A. Stüvel left a comment
Member

Starting to look good! Just a few superficial remarks.

Starting to look good! Just a few superficial remarks.
@ -1142,0 +1152,4 @@
std::string legacy_name = legacy_action.id.name + 2;
/* In case the legacy action has a long name it is shortened to make space for the suffix. */
if (legacy_name.size() > MAX_ID_NAME - (suffix.size() + 3)) {
legacy_name = legacy_name.substr(0, MAX_ID_NAME - (suffix.size() + 3));

This will break UTF-8 strings when a multi-byte character is truncated. Probably better to use STRNCPY_UTF8() (or a similar function).

This will break UTF-8 strings when a multi-byte character is truncated. Probably better to use `STRNCPY_UTF8()` (or a similar function).
Author
Member

thanks for catching that, did a bit of reading into this to see if there is a nice C++ way. Doesn't seem to exist yet

thanks for catching that, did a bit of reading into this to see if there is a nice C++ way. Doesn't seem to exist yet
ChrisLend marked this conversation as resolved
@ -1142,0 +1156,4 @@
}
const std::string layered_action_name = legacy_name + suffix;
bAction *baction = BKE_action_add(&bmain, layered_action_name.c_str());

For the C type, I usually use the variable name dna_action. Doesn't matter much, just brings a bit more consistency. (disclaimer: I'm sure the code I wrote is also not always as consistent as I'd like)

For the C type, I usually use the variable name `dna_action`. Doesn't matter much, just brings a bit more consistency. (disclaimer: I'm sure the code I wrote is also not always as consistent as I'd like)
ChrisLend marked this conversation as resolved
@ -1142,0 +1159,4 @@
bAction *baction = BKE_action_add(&bmain, layered_action_name.c_str());
Action &converted_action = baction->wrap();
Binding &binding = converted_action.binding_add();

This will create a binding called "Binding". It's better to use the name of the animated ID. However, that would make things a little less clean (as it needs another param that may not even be available in the general 'convert this action' case). Maybe this could be the responsibility of the operator? That's assigning the new Action to the ID anyway, and so it has all the info it needs. That way this function can remain ID-agnostic.

This will create a binding called "Binding". It's better to use the name of the animated ID. However, that would make things a little less clean (as it needs another param that may not even be available in the general 'convert this action' case). Maybe this could be the responsibility of the operator? That's assigning the new Action to the ID anyway, and so it has all the info it needs. That way this function can remain ID-agnostic.
@ -1142,0 +1162,4 @@
Binding &binding = converted_action.binding_add();
Layer &layer = converted_action.layer_add(legacy_action.id.name);
Strip &strip = layer.strip_add(Strip::Type::Keyframe);
KeyframeStrip &key_strip = strip.as<KeyframeStrip>();

This:

  Strip &strip = layer.strip_add(Strip::Type::Keyframe);
  KeyframeStrip &key_strip = strip.as<KeyframeStrip>();

can be shortened to:

  KeyframeStrip &key_strip = layer.strip_add<KeyframeStrip>();

Not all existing code has been ported to use this, but I think it's kinda nice for new code.

This: ```cpp Strip &strip = layer.strip_add(Strip::Type::Keyframe); KeyframeStrip &key_strip = strip.as<KeyframeStrip>(); ``` can be shortened to: ```cpp KeyframeStrip &key_strip = layer.strip_add<KeyframeStrip>(); ``` Not all existing code has been ported to use this, but I think it's kinda nice for new code.
ChrisLend marked this conversation as resolved
@ -508,0 +522,4 @@
Action *converted = convert_to_layered_action(*bmain, *anim);
ASSERT_TRUE(converted != anim);
EXPECT_STREQ(converted->id.name, "ACACÄnimåtië_layered");
Strip *strip = converted->layers()[0]->strips()[0];

Array access ->plural()[index] can be replaced with ->singular(index). Same for channelbags()[0] below.

Array access `->plural()[index]` can be replaced with `->singular(index)`. Same for `channelbags()[0]` below.
ChrisLend marked this conversation as resolved
@ -770,0 +788,4 @@
animrig::unassign_animation(object->id);
BLI_assert(layered_action->binding_array_num == 1);
layered_action->assign_id(&layered_action->binding_array[0]->wrap(), object->id);

You can shorten &layered_action->binding_array[0]->wrap() to layered_action->binding(0)

You can shorten `&layered_action->binding_array[0]->wrap()` to `layered_action->binding(0)`
ChrisLend marked this conversation as resolved
@ -770,0 +790,4 @@
BLI_assert(layered_action->binding_array_num == 1);
layered_action->assign_id(&layered_action->binding_array[0]->wrap(), object->id);
return OPERATOR_FINISHED;

This is missing a notifier and a depsgraph relations update call. Something like this should work:

diff --git a/source/blender/editors/animation/anim_ops.cc b/source/blender/editors/animation/anim_ops.cc
index a9f3e315c19..23e864965b2 100644
--- a/source/blender/editors/animation/anim_ops.cc
+++ b/source/blender/editors/animation/anim_ops.cc
@@ -36,6 +36,7 @@
 #include "ED_time_scrub_ui.hh"
 
 #include "DEG_depsgraph.hh"
+#include "DEG_depsgraph_build.hh"
 
 #include "SEQ_iterator.hh"
 #include "SEQ_sequencer.hh"
@@ -790,6 +791,11 @@ static int convert_action_exec(bContext *C, wmOperator *op)
   BLI_assert(layered_action->binding_array_num == 1);
   layered_action->assign_id(&layered_action->binding_array[0]->wrap(), object->id);
 
+  ANIM_id_update(bmain, &object->id);
+  DEG_relations_tag_update(CTX_data_main(C));
+
+  WM_main_add_notifier(NC_ANIMATION | ND_NLA_ACTCHANGE, nullptr);
+
   return OPERATOR_FINISHED;
 }
 
This is missing a notifier and a depsgraph relations update call. Something like this should work: ```diff diff --git a/source/blender/editors/animation/anim_ops.cc b/source/blender/editors/animation/anim_ops.cc index a9f3e315c19..23e864965b2 100644 --- a/source/blender/editors/animation/anim_ops.cc +++ b/source/blender/editors/animation/anim_ops.cc @@ -36,6 +36,7 @@ #include "ED_time_scrub_ui.hh" #include "DEG_depsgraph.hh" +#include "DEG_depsgraph_build.hh" #include "SEQ_iterator.hh" #include "SEQ_sequencer.hh" @@ -790,6 +791,11 @@ static int convert_action_exec(bContext *C, wmOperator *op) BLI_assert(layered_action->binding_array_num == 1); layered_action->assign_id(&layered_action->binding_array[0]->wrap(), object->id); + ANIM_id_update(bmain, &object->id); + DEG_relations_tag_update(CTX_data_main(C)); + + WM_main_add_notifier(NC_ANIMATION | ND_NLA_ACTCHANGE, nullptr); + return OPERATOR_FINISHED; } ```
ChrisLend marked this conversation as resolved
@ -770,0 +805,4 @@
return false;
}
if (adt->action->wrap().is_action_layered()) {

Just pondering out loud. .is_action_layered() will return true for empty Actions, as there is no way to distinguish legacy from layered actions when they're empty. Do you think it makes sense to check for !adt->action->wrap().is_action_legacy()? That way an empty legacy action can be converted to a layered action, which will actually do something, namely creating a binding. And because the binding is there, it's no longer empty, and thus explicitly a layered action.

Just pondering out loud. `.is_action_layered()` will return `true` for empty Actions, as there is no way to distinguish legacy from layered actions when they're empty. Do you think it makes sense to check for `!adt->action->wrap().is_action_legacy()`? That way an empty legacy action can be converted to a layered action, which will actually do _something_, namely creating a binding. And because the binding is there, it's no longer empty, and thus explicitly a layered action.
Author
Member

good idea, also added that to the unit tests

good idea, also added that to the unit tests
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 1 commit 2024-06-14 13:10:34 +02:00
Christoph Lendenfeld added 1 commit 2024-06-14 14:38:35 +02:00
Christoph Lendenfeld added 1 commit 2024-06-14 15:11:27 +02:00
Author
Member

This will create a binding called "Binding". It's better to use the name of the animated ID. However, that would make things a little less clean (as it needs another param that may not even be available in the general 'convert this action' case). Maybe this could be the responsibility of the operator? That's assigning the new Action to the ID anyway, and so it has all the info it needs. That way this function can remain ID-agnostic.

@dr.sybren taking this out of the review comments so it isn't lost.

Should the operator then create the action, and the function just fill it? I do need the binding already within the function because that is what holds the animation data.
Or should I just update the name of the binding in the operator code

> This will create a binding called "Binding". It's better to use the name of the animated ID. However, that would make things a little less clean (as it needs another param that may not even be available in the general 'convert this action' case). Maybe this could be the responsibility of the operator? That's assigning the new Action to the ID anyway, and so it has all the info it needs. That way this function can remain ID-agnostic. @dr.sybren taking this out of the review comments so it isn't lost. Should the operator then create the action, and the function just fill it? I do need the binding already within the function because that is what holds the animation data. Or should I just update the name of the binding in the operator code

Or should I just update the name of the binding in the operator code

I think that'll be the best approach for now.

> Or should I just update the name of the binding in the operator code I think that'll be the best approach for now.
Christoph Lendenfeld added 2 commits 2024-06-20 11:44:37 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-20 11:47:03 +02:00
Sybren A. Stüvel requested changes 2024-06-20 16:00:01 +02:00
Dismissed
@ -770,0 +778,4 @@
animrig::Action &anim = adt->action->wrap();
if (anim.is_empty()) {
BKE_report(
op->reports, RPT_WARNING, "The given action is empty, so it is considered to be layered");

The poll operator code says the exec will add a binding, but here this is still prevented.

The poll operator code says the exec will add a binding, but here this is still prevented.
Author
Member

good catch. Since there is no data to copy, the case of converting an empty action differs in that it doesn't create a second action datablock. Instead it "converts" it in place by just adding the binding

good catch. Since there is no data to copy, the case of converting an empty action differs in that it doesn't create a second action datablock. Instead it "converts" it in place by just adding the binding

I think it's better to always have the same semantics, so either it always converts in place, or it always produces a new Action.

I think it's better to always have the same semantics, so either it always converts in place, or it always produces a new Action.
@ -770,0 +794,4 @@
layered_action->assign_id(binding, object->id);
ANIM_id_update(bmain, &object->id);
DEG_relations_tag_update(CTX_data_main(C));

CTX_data_main(C) -> bmain

`CTX_data_main(C)` -> `bmain`
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-06-20 16:16:53 +02:00
convert empty actions
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
1ad79a581f
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-20 16:18:10 +02:00
Author
Member

@blender-bot build

@blender-bot build
Nathan Vegdahl requested changes 2024-06-21 17:41:18 +02:00
Nathan Vegdahl left a comment
Member

Basically looks good to me. Just a few minor comments.

Basically looks good to me. Just a few minor comments.
@ -1218,0 +1237,4 @@
Layer &layer = converted_action.layer_add(legacy_action.id.name);
KeyframeStrip &strip = layer.strip_add<KeyframeStrip>();
ChannelBag *bag = strip.channelbag_for_binding(binding);
if (!bag) {
Member

I'm a little confused why this is necessary. Since we're freshly creating this action, shouldn't it be fine to just unconditionally add a channel bag here, rather than trying to look it up first (which will presumably always fail?).

Maybe I'm misunderstanding something, though.

I'm a little confused why this is necessary. Since we're freshly creating this action, shouldn't it be fine to just unconditionally add a channel bag here, rather than trying to look it up first (which will presumably always fail?). Maybe I'm misunderstanding something, though.
Author
Member

indeed. I don't see a reason why there would be a bag already. I'll replace that with an assert

indeed. I don't see a reason why there would be a bag already. I'll replace that with an assert
nathanvegdahl marked this conversation as resolved
@ -1218,0 +1243,4 @@
LISTBASE_FOREACH (FCurve *, fcu, &legacy_action.curves) {
FCurve *new_fcu = BKE_fcurve_copy(fcu);
grow_array_and_append(&bag->fcurve_array, &bag->fcurve_array_num, new_fcu);
Member

This is probably fine as-is given the number of items we're likely dealing with. But just a note that this has quadratic complexity since it's growing the array one item at a time, which involves copying the entire array so far each time.

If you feel like going to the effort, you could count the f-curves and pre-allocate the whole array up front for linear run time. But its probably not a big deal.

This is probably fine as-is given the number of items we're likely dealing with. But just a note that this has quadratic complexity since it's growing the array one item at a time, which involves copying the entire array so far each time. If you feel like going to the effort, you could count the f-curves and pre-allocate the whole array up front for linear run time. But its probably not a big deal.
Author
Member

decided to do that now. Simple thing to change and faster is better.
@dr.sybren mind taking a look at that specifically?

decided to do that now. Simple thing to change and faster is better. @dr.sybren mind taking a look at that specifically?

I think Nathan's idea is a good one. I'm pondering whether to do that in function like ChannelBag::fcurve_array_grow_to(const int num_fcurves) or not. One the one hand, it feels like it's nice to have these things in a separate function. But on the other hand, the caller then has to ensure that none of those pointers is left as nullptr, creating strong cohesion between those bits of code. So yeah, keep this logic here, so that all the code that handles this is in one place.

For safety, do include a BLI_assert(bag->fcurve_array == nullptr) before overwriting its value. Just so that the reader doesn't have to think about whether this is a potential memory leak or not.

I think Nathan's idea is a good one. I'm pondering whether to do that in function like `ChannelBag::fcurve_array_grow_to(const int num_fcurves)` or not. One the one hand, it feels like it's nice to have these things in a separate function. But on the other hand, the caller then has to ensure that none of those pointers is left as `nullptr`, creating strong cohesion between those bits of code. So yeah, keep this logic here, so that all the code that handles this is in one place. For safety, do include a `BLI_assert(bag->fcurve_array == nullptr)` before overwriting its value. Just so that the reader doesn't have to think about whether this is a potential memory leak or not.
nathanvegdahl marked this conversation as resolved
@ -761,0 +765,4 @@
Object *object = CTX_data_active_object(C);
AnimData *adt = BKE_animdata_from_id(&object->id);
Member

(Commenting on the line between because Sybren already has unrelated comments on the lines I want.)

I think asserts that the animdata and action exist would be good. Partly to catch if the assumption doesn't hold, and partly to make it explicit when reading the code in isolation that it was considered.

(Commenting on the line between because Sybren already has unrelated comments on the lines I want.) I think asserts that the animdata and action exist would be good. Partly to catch if the assumption doesn't hold, and partly to make it explicit when reading the code in isolation that it was considered.
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-06-25 11:43:30 +02:00
Christoph Lendenfeld added 1 commit 2024-06-25 11:45:31 +02:00
Christoph Lendenfeld added 1 commit 2024-06-25 12:02:31 +02:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-06-25 12:03:37 +02:00
Sybren A. Stüvel reviewed 2024-06-26 17:27:40 +02:00
Sybren A. Stüvel left a comment
Member

I think the conversion code is in good shape. Not 100% sure about the workflow/UX aspects yet, when it comes to replacing the old Action or creating a new one.

For one user of that Action, it doesn't matter much, as after running the operator the user is now animated with the layered Action. But in the case of several users of the same Action, this might be too limiting (requiring manual lookups of the users of the old Action, and replacing all of those with the new one). In-place conversion would solve this, but would also make the old legacy Action unavailable (which may not be a huge issue). Alternatively, the current behaviour could be extended to assign the converted Action to all the users of the old Action.

I think the conversion code is in good shape. Not 100% sure about the workflow/UX aspects yet, when it comes to replacing the old Action or creating a new one. For one user of that Action, it doesn't matter much, as after running the operator the user is now animated with the layered Action. But in the case of several users of the same Action, this might be too limiting (requiring manual lookups of the users of the old Action, and replacing all of those with the new one). In-place conversion would solve this, but would also make the old legacy Action unavailable (which may not be a huge issue). Alternatively, the current behaviour could be extended to assign the converted Action to all the users of the old Action.
Author
Member

@dr.sybren I think we should make sure no data is lost and the user can easily go back if needed. That's why I decided to create a new action.
As for re-assignging all users of that action, that is a good idea. I will add that.

For empty actions I still think in place conversion is acceptable since no data is lost/changed.
Edit: just read your comment, of course we can also duplicate the empty action to convert to layered.

@dr.sybren I think we should make sure no data is lost and the user can easily go back if needed. That's why I decided to create a new action. As for re-assignging all users of that action, that is a good idea. I will add that. For empty actions I still think in place conversion is acceptable since no data is lost/changed. Edit: just read your comment, of course we can also duplicate the empty action to convert to layered.

I wanted to use this operator, had to do this to get it to build with main:

diff --git a/source/blender/animrig/intern/action_test.cc b/source/blender/animrig/intern/action_test.cc
index f4e5ee859dd..f87c0b133a9 100644
--- a/source/blender/animrig/intern/action_test.cc
+++ b/source/blender/animrig/intern/action_test.cc
@@ -573,8 +573,8 @@ TEST_F(ActionLayersTest, is_action_assignable_to)
 TEST_F(ActionLayersTest, conversion_to_layered)
 {
   EXPECT_TRUE(anim->is_empty());
-  FCurve *legacy_fcu_0 = action_fcurve_ensure(bmain, anim, "Test", nullptr, "location", 0);
-  FCurve *legacy_fcu_1 = action_fcurve_ensure(bmain, anim, "Test", nullptr, "location", 1);
+  FCurve *legacy_fcu_0 = action_fcurve_ensure(bmain, anim, "Test", nullptr, {"location", 0});
+  FCurve *legacy_fcu_1 = action_fcurve_ensure(bmain, anim, "Test", nullptr, {"location", 1});
 
   KeyframeSettings settings;
   settings.handle = HD_AUTO;
@@ -598,7 +598,7 @@ TEST_F(ActionLayersTest, conversion_to_layered)
 
   Action *long_name_action = static_cast<Action *>(BKE_id_new(
       bmain, ID_AC, "name_for_an_action_that_is_exactly_64_chars_which_is_MAX_ID_NAME"));
-  action_fcurve_ensure(bmain, long_name_action, "Long", nullptr, "location", 0);
+  action_fcurve_ensure(bmain, long_name_action, "Long", nullptr, {"location", 0});
   converted = convert_to_layered_action(*bmain, *long_name_action);
   /* AC gets added automatically by Blender, the long name is shortened to make space for
    * "_layered". */

Just want to leave this here so you don't have to dig into this as well ;-)

I wanted to use this operator, had to do this to get it to build with `main`: ```diff diff --git a/source/blender/animrig/intern/action_test.cc b/source/blender/animrig/intern/action_test.cc index f4e5ee859dd..f87c0b133a9 100644 --- a/source/blender/animrig/intern/action_test.cc +++ b/source/blender/animrig/intern/action_test.cc @@ -573,8 +573,8 @@ TEST_F(ActionLayersTest, is_action_assignable_to) TEST_F(ActionLayersTest, conversion_to_layered) { EXPECT_TRUE(anim->is_empty()); - FCurve *legacy_fcu_0 = action_fcurve_ensure(bmain, anim, "Test", nullptr, "location", 0); - FCurve *legacy_fcu_1 = action_fcurve_ensure(bmain, anim, "Test", nullptr, "location", 1); + FCurve *legacy_fcu_0 = action_fcurve_ensure(bmain, anim, "Test", nullptr, {"location", 0}); + FCurve *legacy_fcu_1 = action_fcurve_ensure(bmain, anim, "Test", nullptr, {"location", 1}); KeyframeSettings settings; settings.handle = HD_AUTO; @@ -598,7 +598,7 @@ TEST_F(ActionLayersTest, conversion_to_layered) Action *long_name_action = static_cast<Action *>(BKE_id_new( bmain, ID_AC, "name_for_an_action_that_is_exactly_64_chars_which_is_MAX_ID_NAME")); - action_fcurve_ensure(bmain, long_name_action, "Long", nullptr, "location", 0); + action_fcurve_ensure(bmain, long_name_action, "Long", nullptr, {"location", 0}); converted = convert_to_layered_action(*bmain, *long_name_action); /* AC gets added automatically by Blender, the long name is shortened to make space for * "_layered". */ ``` Just want to leave this here so you don't have to dig into this as well ;-)
Christoph Lendenfeld added 3 commits 2024-07-02 13:21:35 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-07-02 13:21:55 +02:00
Author
Member

@dr.sybren I removed the special handling of empty actions. However I didn't add the feature of re-assigning the converted action to old users of the old action. I am wondering if this is even something we SHOULD do because it could affect hidden objects which kind of goes against the design principles of blender

@dr.sybren I removed the special handling of empty actions. However I didn't add the feature of re-assigning the converted action to old users of the old action. I am wondering if this is even something we SHOULD do because it could affect hidden objects which kind of goes against the design principles of blender
Sybren A. Stüvel requested changes 2024-07-05 11:39:47 +02:00
Sybren A. Stüvel left a comment
Member

Let's keep it as-is then, and see how people respond to the feature. We can always add an operator option so they can choose:

  1. Keep legacy Action assigned
  2. Assign converted Action
  3. Assign converted Action to all users

As for the operator name, after discussing with Dalai I think it's better to name it "Convert Legacy Action". The aim is to reduce the mention of "layered Actions", as those will just be "Actions" when we go out of experimental.

Let's keep it as-is then, and see how people respond to the feature. We can always add an operator option so they can choose: 1. Keep legacy Action assigned 2. Assign converted Action 3. Assign converted Action to all users As for the operator name, after discussing with Dalai I think it's better to name it "Convert Legacy Action". The aim is to reduce the mention of "layered Actions", as those will just be "Actions" when we go out of experimental.
Christoph Lendenfeld added 2 commits 2024-07-05 12:29:10 +02:00
Author
Member

@dr.sybren I agree with the notion of layered actions just being actions, "Convert Legacy Action" doesn't tell me what it is converted into though.
I made the change, but let me know if any of the following are clearer

  • Upgrade Legacy Action
  • Add Layers to Action
@dr.sybren I agree with the notion of layered actions just being actions, "Convert Legacy Action" doesn't tell me what it is converted into though. I made the change, but let me know if any of the following are clearer * Upgrade Legacy Action * Add Layers to Action
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-07-05 12:32:17 +02:00
Sybren A. Stüvel approved these changes 2024-07-05 12:44:13 +02:00
Sybren A. Stüvel left a comment
Member

@dr.sybren I agree with the notion of layered actions just being actions, "Convert Legacy Action" doesn't tell me what it is converted into though.

True, although I feel that the use of "legacy" already implies "to whatever the non-legacy thing is". Let's keep it like this for now.

Just one final note that can be handled when landing.

> @dr.sybren I agree with the notion of layered actions just being actions, "Convert Legacy Action" doesn't tell me what it is converted into though. True, although I feel that the use of "legacy" already implies "to whatever the non-legacy thing is". Let's keep it like this for now. Just one final note that can be handled when landing.
@ -761,0 +814,4 @@
{
/* identifiers */
ot->name = "Convert Legacy Action";
ot->idname = "ANIM_OT_convert_legacy_action";

The function name should match the idname.

The function name should match the `idname`.
Christoph Lendenfeld added 1 commit 2024-07-05 13:22:36 +02:00
rename function
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
6443d4c478
Author
Member

@blender-bot build

@blender-bot build
Christoph Lendenfeld added 2 commits 2024-07-09 17:34:30 +02:00
fix merge issues
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 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-coordinator Build done.
0cb92f14e1
Author
Member

@blender-bot build

had to resolve a bunch of merge issues due to binding->slot rename

@blender-bot build had to resolve a bunch of merge issues due to binding->slot rename
Christoph Lendenfeld added 1 commit 2024-07-09 17:36:33 +02:00

The Windows build error was caused by something on the main branch, so just try another merge & poke the build-bot.

The Windows build error was caused by something on the `main` branch, so just try another merge & poke the build-bot.
Christoph Lendenfeld added 1 commit 2024-07-11 10:52:48 +02:00
Merge branch 'main' into action_convert_operator
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.
6fc95f435e
Author
Member

@blender-bot build

@blender-bot build
Christoph Lendenfeld merged commit 907c49da08 into main 2024-07-11 11:57:44 +02:00
Christoph Lendenfeld deleted branch action_convert_operator 2024-07-11 11:57:49 +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#122043
No description provided.