Anim: Reuse action between related data #126655

Merged
Christoph Lendenfeld merged 34 commits from ChrisLend/blender:layered_action_create_once into main 2024-09-13 17:57:40 +02:00

When inserting keys, look on related IDs for an action to reuse that.
This will make use of the slot system on the new layered action to ensure
the animation data doesn't collide.

This is done on the id_action_ensure function since that is the common
function to get an action off an ID.

IDs with more than 1 user will be skipped.

"Related ID" in this case is hardcoded with a switch statement for each ID type.
The system builds a list starting from the ID that should be keyed and
keeps expanding the list until an action is found or no more
non-duplicate IDs can be added. (This is using linear search for duplicate checks,
but I don't think we will deal with a lot of IDs in this case)

When inserting keys, look on related IDs for an action to reuse that. This will make use of the slot system on the new layered action to ensure the animation data doesn't collide. This is done on the `id_action_ensure` function since that is the common function to get an action off an `ID`. IDs with more than 1 user will be skipped. "Related ID" in this case is hardcoded with a `switch` statement for each ID type. The system builds a list starting from the ID that should be keyed and keeps expanding the list until an action is found or no more non-duplicate IDs can be added. (This is using linear search for duplicate checks, but I don't think we will deal with a lot of IDs in this case)
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2024-08-22 16:48:12 +02:00
Christoph Lendenfeld added 1 commit 2024-08-22 16:48:22 +02:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-08-22 16:48:41 +02:00
Christoph Lendenfeld changed title from Anim: Create layered action once for data and action to WIP: Anim: Create layered action once for data and action 2024-08-22 17:51:40 +02:00
Christoph Lendenfeld added 1 commit 2024-08-22 17:58:15 +02:00
Christoph Lendenfeld added 1 commit 2024-08-23 12:02:45 +02:00
Christoph Lendenfeld changed title from WIP: Anim: Create layered action once for data and action to Anim: Create layered action once for data and action 2024-08-23 12:31:33 +02:00
Christoph Lendenfeld added 1 commit 2024-08-23 12:31:36 +02:00
add unit test
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.
56ec0c2d6e
Author
Member

@blender-bot build

@nathanvegdahl crash should be fixed, and I added a unit test for this behavior.

@blender-bot build @nathanvegdahl crash should be fixed, and I added a unit test for this behavior.
Nathan Vegdahl requested changes 2024-08-23 14:34:16 +02:00
Nathan Vegdahl left a comment
Member

Overall, looking good!

I think it would be good to throw in at least one or two other cases. For example:

  • Assigning node trees to the same action as their material (if they're directly assigned to one) and vice-versa.
  • Assigning shape key sets (which are for some reason a separate ID from meshes) to the same action as their mesh and vice-versa.

I think that will help inform the code structure a bit, and will also just generally help to flesh out the feature.

Overall, looking good! I think it would be good to throw in at least one or two other cases. For example: - Assigning node trees to the same action as their material (if they're directly assigned to one) and vice-versa. - Assigning shape key sets (which are for some reason a separate ID from meshes) to the same action as their mesh and vice-versa. I think that will help inform the code structure a bit, and will also just generally help to flesh out the feature.
@ -37,1 +39,4 @@
/* Find an action that is related to the given ID. Either on the data if the ID is an Object or a
* user of the ID. */
static bAction *find_action(const Main &bmain, const ID &id)
Member

Maybe call this find_relevant_action_for_id() or find_suitable_action_for_id() or something like that. At-a-glance just find_action() doesn't really suggest it's purpose very well, to me at least.

Maybe call this `find_relevant_action_for_id()` or `find_suitable_action_for_id()` or something like that. At-a-glance just `find_action()` doesn't really suggest it's purpose very well, to me at least.
Member

Another possibility just occurred to me: find_related_action(). It's right there in your doc comment, and works well I think.

Another possibility just occurred to me: `find_related_action()`. It's right there in your doc comment, and works well I think.
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-08-23 15:43:04 +02:00
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-08-26 12:24:02 +02:00
Christoph Lendenfeld added 1 commit 2024-08-27 10:34:26 +02:00
Christoph Lendenfeld added 1 commit 2024-08-27 11:51:26 +02:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-08-27 11:51:36 +02:00
Nathan Vegdahl requested changes 2024-09-02 11:22:08 +02:00
Nathan Vegdahl left a comment
Member

Reading through the code, one thing that occurred to me is that I don't think this is keying-order invariant. In other words, the code as it is right now will result in different action arrangements (what does and doesn't share an action) depending on the order you key things in.

For example, if you key object -> mesh -> shapekeys, then all three end up in the same action, but if you key object -> shapekeys -> mesh then the shapekeys end up in a different action.

My gut feeling is that this will confuse people. Basically, as long as the scene itself is the same, I'm thinking that the order in which you key things probably shouldn't have an effect on what things end up in actions together.

Admittedly, that might make the code more complicated. But I think it's worth it from a UX perspective.

Reading through the code, one thing that occurred to me is that I don't think this is keying-order invariant. In other words, the code as it is right now will result in different action arrangements (what does and doesn't share an action) depending on the order you key things in. For example, if you key object -> mesh -> shapekeys, then all three end up in the same action, but if you key object -> shapekeys -> mesh then the shapekeys end up in a different action. My gut feeling is that this will confuse people. Basically, as long as the scene itself is the same, I'm thinking that the order in which you key things probably shouldn't have an effect on what things end up in actions together. Admittedly, that might make the code more complicated. But I think it's worth it from a UX perspective.
@ -38,0 +45,4 @@
static bAction *find_related_action(const Main &bmain, const ID &id)
{
switch (GS(id.name)) {
case ID_OB: {
Member

Might be worth putting a comment at the top of each case explaining its intent (e.g. "search for an action already assigned to the object's data").

Might be worth putting a comment at the top of each case explaining its intent (e.g. "search for an action already assigned to the object's data").
nathanvegdahl marked this conversation as resolved
@ -38,0 +47,4 @@
switch (GS(id.name)) {
case ID_OB: {
Object *ob = (Object *)(&id);
if (!ob) {
Member

Can this ever be null? I might be misunderstanding the code, but it looks to me like this could be an assert instead.

Can this ever be null? I might be misunderstanding the code, but it looks to me like this could be an assert instead.
nathanvegdahl marked this conversation as resolved
Member

An approach that might(?) help simplify the code, or at least make it easier to follow, is to have a function that (given an ID) returns a priority-ordered list of other IDs to check for actions. Then the find_related_action() function itself becomes really simple: it just calls that function and then goes down the list looking for attached actions to use.

Then again, maybe in practice it wouldn't actually help. But it seems worth a shot.

An approach that might(?) help simplify the code, or at least make it easier to follow, is to have a function that (given an ID) returns a priority-ordered list of other IDs to check for actions. Then the `find_related_action()` function itself becomes really simple: it just calls that function and then goes down the list looking for attached actions to use. Then again, maybe in practice it wouldn't actually help. But it seems worth a shot.
Author
Member

For example, if you key object -> mesh -> shapekeys, then all three end up in the same action, but if you key object -> shapekeys -> mesh then the shapekeys end up in a different action.

I agree and that is what I tried to convey in my self critique in the PR description. The thing is I am not sure adding code to handle the case from shapekey to object or the other way around is a reliable solution.

> > For example, if you key object -> mesh -> shapekeys, then all three end up in the same action, but if you key object -> shapekeys -> mesh then the shapekeys end up in a different action. > I agree and that is what I tried to convey in my self critique in the PR description. The thing is I am not sure adding code to handle the case from shapekey to object or the other way around is a reliable solution.
Member

The thing is I am not sure adding code to handle the case from shapekey to object or the other way around is a reliable solution.

I think it should be reliable in terms of keying order?

The thing that would change results is the actual bmain data being different (e.g. if a mesh only has one user then it shares its action with that user, but if it has more than one user then it gets its own action). Admittedly, that does mean the user has to be aware to some extent, so there are some real tradeoffs there, you're right.


If we do still want to go in whole-hog on this, rather than keeping it limited to e.g. just embedded IDs, then I think conceptually we can think of it in terms of what connections between IDs we consider to be valid. And then any ID that's reachable from any other ID via those valid connections gets hooked up to the same action. In other words, there are basically "islands" of IDs, and islands share an action. (There could be cases where the user has already set up two IDs in the same island to have different actions, and in that case it could work based on some kind of simple priority, maybe based on the directness of the connections or prefering to connect "up" or something like that.)

Conceptually I don't think this is too complicated. But actually representing this in code might be tricky in practice without over-complicating things or making it difficult to maintain. If that's the case, that may also be a good argument for just keeping it simple and only handling embedded IDs. Also because we have limited time to try to make this work at the moment.

But I just wanted to throw that out there in case it helped inspire a solution.

> The thing is I am not sure adding code to handle the case from shapekey to object or the other way around is a reliable solution. I think it should be reliable in terms of keying order? The thing that would change results is the actual bmain data being different (e.g. if a mesh only has one user then it shares its action with that user, but if it has more than one user then it gets its own action). Admittedly, that does mean the user has to be aware to some extent, so there are some real tradeoffs there, you're right. ---- If we do still want to go in whole-hog on this, rather than keeping it limited to e.g. just embedded IDs, then I think conceptually we can think of it in terms of what connections between IDs we consider to be valid. And then any ID that's reachable from any other ID via those valid connections gets hooked up to the same action. In other words, there are basically "islands" of IDs, and islands share an action. (There could be cases where the user has already set up two IDs in the same island to have different actions, and in that case it could work based on some kind of simple priority, maybe based on the directness of the connections or prefering to connect "up" or something like that.) Conceptually I don't think this is too complicated. But actually representing this in code might be tricky in practice without over-complicating things or making it difficult to maintain. If that's the case, that may also be a good argument for just keeping it simple and only handling embedded IDs. Also because we have limited time to try to make this work at the moment. But I just wanted to throw that out there in case it helped inspire a solution.
Christoph Lendenfeld added 2 commits 2024-09-03 13:45:08 +02:00
Christoph Lendenfeld added 1 commit 2024-09-03 15:43:52 +02:00
Author
Member

once again this crashes the unit tests, but with the recent changes this is less spaghetti and more lasagne so thats good

once again this crashes the unit tests, but with the recent changes this is less spaghetti and more lasagne so thats good
Christoph Lendenfeld added 1 commit 2024-09-03 16:20:20 +02:00
fix segfault in unit tests
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.
d679d7a1d0
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-09-03 16:23:47 +02:00
Author
Member

@blender-bot build

Crash is fixed, should be good to have another look.
Thanks for the suggestion of a priority ordered list @nathanvegdahl
That's kind of what I ended up with now

@blender-bot build Crash is fixed, should be good to have another look. Thanks for the suggestion of a priority ordered list @nathanvegdahl That's kind of what I ended up with now
Nathan Vegdahl requested changes 2024-09-05 11:03:44 +02:00
Nathan Vegdahl left a comment
Member

Looking good! I'm mostly just missing some documentation/comments.

Looking good! I'm mostly just missing some documentation/comments.
@ -35,6 +41,215 @@ namespace blender::animrig {
/** \name Public F-Curves API
* \{ */
static void add_object_data_user(const Main &bmain,
Member

I think the name should be plural, unless I'm misunderstanding the code. add_object_data_users

Additionally, I think both this and add_id_materials() below could use a brief documentation comment. For example, on this function something like "Append the object IDs that use id as their data to r_related_ids. Skips adding IDs that are already in r_related_ids."

I think the name should be plural, unless I'm misunderstanding the code. `add_object_data_users` Additionally, I think both this and `add_id_materials()` below could use a brief documentation comment. For example, on this function something like "Append the object IDs that use `id` as their data to `r_related_ids`. Skips adding IDs that are already in `r_related_ids`."
Author
Member

I've named it singular because it returns after finding the first one. In hindsight, this couples the code very closely to the calling function (which checks if there is only one user anyway).

I've changed it now so it will find all users of that object data and add them to the list. That shouldn't change behavior due to the user count check I mentioned before.

I've named it singular because it returns after finding the first one. In hindsight, this couples the code very closely to the calling function (which checks if there is only one user anyway). I've changed it now so it will find all users of that object data and add them to the list. That shouldn't change behavior due to the user count check I mentioned before.
ChrisLend marked this conversation as resolved
@ -38,0 +99,4 @@
}
}
/* Find an action that is related to the given ID. Either on the data if the ID is an Object or a
Member

This doc comment needs to be updated, since it now also searches indirect users.

This doc comment needs to be updated, since it now also searches indirect users.
ChrisLend marked this conversation as resolved
@ -38,0 +106,4 @@
{
Vector<const ID *> related_ids({&id});
for (int i = 0; i < related_ids.size(); i++) {
Member

I really like this solution! Very elegant, IMO.

Having said that, it took me a moment of brain work and looking at the code inside the loop to figure out what was going on, so a brief comment explaining that the size of related_ids expands during iteration, or maybe even going into a little bit more detail than that about how the loop works in general, might be useful here.

I really like this solution! Very elegant, IMO. Having said that, it took me a moment of brain work and looking at the code inside the loop to figure out what was going on, so a brief comment explaining that the size of `related_ids` expands during iteration, or maybe even going into a little bit more detail than that about how the loop works in general, might be useful here.
Author
Member

do you think it would be clearer if it was a while loop?
I chose a for loop because it will always increment i, so no risk of falling into an endless loop. For loops do give the impression of a pre-known size though

do you think it would be clearer if it was a while loop? I chose a for loop because it will always increment i, so no risk of falling into an endless loop. For loops do give the impression of a pre-known size though
Member

I think a for loop is actually better, for the same reason you gave. Just needs a comment to clarify, I think.

I think a for loop is actually better, for the same reason you gave. Just needs a comment to clarify, I think.

We have macro to iterate over linked lists no?/

We have macro to iterate over linked lists no?/
Member

It's a Vector in this case, not a linked list.

It's a `Vector` in this case, not a linked list.
nathanvegdahl marked this conversation as resolved
@ -218,6 +218,55 @@ TEST_F(KeyframingTest, insert_keyframes__layered_action__non_array_property)
EXPECT_EQ(7.0, fcurve->bezt[1].vec[1][1]);
}
TEST_F(KeyframingTest, insert_keyframes__layered_action__action_reuse)
Member

Probably could use some more unit tests as well to test at least one or two of the more complex cases.

Probably could use some more unit tests as well to test at least one or two of the more complex cases.
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-09-05 15:25:07 +02:00
Christoph Lendenfeld added 1 commit 2024-09-06 12:21:20 +02:00
add unit tests covering more specific cases
All checks were successful
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint 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.
66a0c78701
Author
Member

@blender-bot build

I added unit tests covering more action reuse cases

@blender-bot build I added unit tests covering more action reuse cases
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-09-06 12:22:12 +02:00
Sybren A. Stüvel requested changes 2024-09-06 14:27:54 +02:00
Sybren A. Stüvel left a comment
Member

Anim: Create layered action once for data and action

I don't quite get the title. Should this be "for object and its data"? Or does this also cover other cases?

"Related ID" in this case is hardcoded with a switch statement for each ID type.
The system builds a list starting from the ID that should be keyed and
keeps expanding the list until an action is found or no more
non-duplicate IDs can be added. (This is using linear search for duplicate checks,
but I don't think we will deal with a lot of IDs in this case)

I don't think this is entirely the right approach. IMO it would be better if Blender would be a bit more conservative in terms of what it considers "related", and because of that be more predictable. For example, materials are often reused, and I think it's better to give them an Action of their own.

IMO these combinations are the most important to implement here:

  • Object + data. I think it's good to keep that clause that does a user count on the object data, to avoid sharing Actions on shared object data.
  • Mesh + Shape Keys: even though the Shape Keys are stored as a separate ID (and not embedded), they cannot be reused because they're so bound to a single specific mesh. This relationship should thus follow the same logic as embedded IDs. The Mesh can be found by following the Key::from pointer.
  • Embedded IDs will never be reused, so these can always use the Action of the owner ID (or any of its other embedded IDs, if there are multiple, but not sure if we have that in Blender right now). This Action should then also be named after the owner ID, and not the embedded ID. You can get the owner ID via BKE_id_owner_get(). An embedded ID can be recognised with id->flag & ID_FLAG_EMBEDDED_DATA.
  • IDs with embedded IDs need to check whether there is already an Action on any of its embedded IDs. I think you can use BKE_library_foreach_ID_link() to find those, but I'll check with a responsible adult to be sure. (update: no, use something else, see below)
> Anim: Create layered action once for data and action I don't quite get the title. Should this be "for object and its data"? Or does this also cover other cases? > "Related ID" in this case is hardcoded with a `switch` statement for each ID type. > The system builds a list starting from the ID that should be keyed and > keeps expanding the list until an action is found or no more > non-duplicate IDs can be added. (This is using linear search for duplicate checks, > but I don't think we will deal with a lot of IDs in this case) I don't think this is entirely the right approach. IMO it would be better if Blender would be a bit more conservative in terms of what it considers "related", and because of that be more predictable. For example, materials are often reused, and I think it's better to give them an Action of their own. IMO these combinations are the most important to implement here: - **Object + data**. I think it's good to keep that clause that does a user count on the object data, to avoid sharing Actions on shared object data. - **Mesh + Shape Keys**: even though the Shape Keys are stored as a separate ID (and not embedded), they cannot be reused because they're so bound to a single specific mesh. This relationship should thus follow the same logic as embedded IDs. The Mesh can be found by following the `Key::from` pointer. - **Embedded IDs** will never be reused, so these can always use the Action of the owner ID (or any of its other embedded IDs, if there are multiple, but not sure if we have that in Blender right now). This Action should then also be named after the owner ID, and not the embedded ID. You can get the owner ID via `BKE_id_owner_get()`. An embedded ID can be recognised with `id->flag & ID_FLAG_EMBEDDED_DATA`. - **IDs with embedded IDs** need to check whether there is already an Action on any of its embedded IDs. ~~I think you can use `BKE_library_foreach_ID_link()` to find those~~, but I'll check with a responsible adult to be sure. (update: no, use something else, see below)
@ -38,0 +48,4 @@
Vector<const ID *> &r_related_ids)
{
for (Object *ob = static_cast<Object *>(bmain.objects.first); ob;
ob = static_cast<Object *>(ob->id.next))

Commenting here instead of where it's applicable, because Nathan already commented on a line, and Gitea only supports one comment per line 😿

This could use something like:

Object *ob;
FOREACH_MAIN_LISTBASE_ID_BEGIN(&bmain.objects, ob) {
  ...
}
FOREACH_MAIN_LISTBASE_ID_END;

Also r_related_ids should be named related_ids as it's not a return parameter: it doesn't get a value from this function; this function uses it to add a value, but that's different.

Commenting here instead of where it's applicable, because Nathan already commented on a line, and Gitea only supports one comment per line 😿 This could use something like: ```cpp Object *ob; FOREACH_MAIN_LISTBASE_ID_BEGIN(&bmain.objects, ob) { ... } FOREACH_MAIN_LISTBASE_ID_END; ``` Also `r_related_ids` should be named `related_ids` as it's not a return parameter: it doesn't get a value from this function; this function uses it to add a value, but that's different.
Author
Member

@dr.sybren Just to clarify, the behavior you are describing is the following cases

<-> relation
// no relation

Object <-> Mesh <-> Shapekey
Object <-> Curve <-> Shapekey
Object <-> Mesh // Material <-> embedded Nodetree
Object // Material // non-embedded Nodetree

@dr.sybren Just to clarify, the behavior you are describing is the following cases `<->` relation `//` no relation Object <-> Mesh <-> Shapekey Object <-> Curve <-> Shapekey Object <-> Mesh // Material <-> embedded Nodetree Object // Material // non-embedded Nodetree

@dr.sybren Just to clarify, the behavior you are describing is the following cases

Affirmative!

  • IDs with embedded IDs need to check whether there is already an Action on any of its embedded IDs. I think you can use BKE_library_foreach_ID_link() to find those, but I'll check with a responsible adult to be sure.

Ok, the answer from the responsible adult was a clear "no". Currently there is no generic iterator that can produce embedded ID pointers. So far this is done by explicitly checking all known cases. Fortunately there aren't that many: the Scene master_collection (which cannot be animated and thus we can skip), and embedded shader node trees. For the latter you can use bke::node_tree_ptr_from_id() (see e.g. id_swap).

> @dr.sybren Just to clarify, the behavior you are describing is the following cases Affirmative! > - **IDs with embedded IDs** need to check whether there is already an Action on any of its embedded IDs. I think you can use `BKE_library_foreach_ID_link()` to find those, but I'll check with a responsible adult to be sure. Ok, the answer from the responsible adult was a clear "no". Currently there is no generic iterator that can produce embedded ID pointers. So far this is done by explicitly checking all known cases. Fortunately there aren't that many: the Scene `master_collection` (which cannot be animated and thus we can skip), and embedded shader node trees. For the latter you can use `bke::node_tree_ptr_from_id()` (see e.g. `id_swap`).
Christoph Lendenfeld changed title from Anim: Create layered action once for data and action to Anim: Reuse action between related data 2024-09-06 16:50:14 +02:00
Christoph Lendenfeld added 1 commit 2024-09-06 16:52:29 +02:00
Christoph Lendenfeld added 1 commit 2024-09-06 17:05:05 +02:00
Christoph Lendenfeld added 1 commit 2024-09-06 17:38:26 +02:00
Christoph Lendenfeld added 1 commit 2024-09-10 10:00:44 +02:00
Christoph Lendenfeld added 1 commit 2024-09-10 10:30:25 +02:00
Christoph Lendenfeld added 1 commit 2024-09-10 10:41:02 +02:00
use owner action name if ID is embedded
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-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
b11e4bc910
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-09-10 10:46:29 +02:00
Author
Member

@blender-bot build

Material and its users are no longer related
Find all owners of embedded bNodeTrees
Find all nodetrees of IDs if they have it.
used the name of the owner ID for embedded IDs

@blender-bot build Material and its users are no longer related Find all owners of embedded `bNodeTree`s Find all nodetrees of IDs if they have it. used the name of the owner ID for embedded IDs
Christoph Lendenfeld added 1 commit 2024-09-10 11:23:28 +02:00
Nathan Vegdahl reviewed 2024-09-10 15:27:37 +02:00
@ -38,0 +71,4 @@
/* In the case of more than 1 user we cannot properly determine from which the action should be
* taken, so those are skipped. Including the 0 users case for embedded IDs. */
if (ID_REAL_USERS(related_id) > 1) {
Member

I think this might not be quite what we want. The intent is right, but for example:

If you have a shared mesh with shapekeys, and the shapekeys have animation, and then you key the mesh, this will bail out entirely due to the objects sharing the mesh, and thus the mesh won't end up finding and sharing the action of the shapekeys.

(Unless I'm misunderstanding something, of course.)

I think this might not be quite what we want. The *intent* is right, but for example: If you have a shared mesh with shapekeys, and the shapekeys have animation, and then you key the mesh, this will bail out entirely due to the objects sharing the mesh, and thus the mesh won't end up finding and sharing the action of the shapekeys. (Unless I'm misunderstanding something, of course.)
Author
Member

that is 100% intentional and the case you are describing is a behavior I expect and even test for in the unit tests.

I understand "related" as a bidirectional graph. As such in the mesh multiuser case, both Objects using the Mesh would be related (ObjectA <-> Mesh <-> ObjectB).
I think this is a case we should avoid, as such the multiuser case is excluded.

that is 100% intentional and the case you are describing is a behavior I expect and even test for in the unit tests. I understand "related" as a bidirectional graph. As such in the mesh multiuser case, both Objects using the Mesh would be related (ObjectA <-> Mesh <-> ObjectB). I think this is a case we should avoid, as such the multiuser case is excluded.

I agree with Nathan here. Just because the Mesh is shared by multiple Objects, doesn't mean that its Action should be ignored when keying its Shape Key.

In other words, the invalidation of OB ↔ ME (due to ME user count), does not invalide the relationship ME ↔ KE.

Same for the user count of MA, that doesn't matter at all when it comes to its embedded NT.

IMO user count only matters when looking at object ↔ data relationships. For the relationships with embedded IDs, the user count of the owner ID is irrelevant.

I agree with Nathan here. Just because the Mesh is shared by multiple Objects, doesn't mean that its Action should be ignored when keying its Shape Key. In other words, the invalidation of `OB ↔ ME` (due to `ME` user count), does _not_ invalide the relationship `ME ↔ KE`. Same for the user count of `MA`, that doesn't matter at all when it comes to its embedded `NT`. IMO user count only matters when looking at object ↔ data relationships. For the relationships with embedded IDs, the user count of the owner ID is irrelevant.
Author
Member

ah ok I see the check is in the wrong place for that.
We want to trace all edges that are 1:1 and even if an ID has more than 1 user there may be edges leading away from it that are 1:1

ah ok I see the check is in the wrong place for that. We want to trace all edges that are 1:1 and even if an ID has more than 1 user there may be edges leading away from it that are 1:1
dr.sybren marked this conversation as resolved
Sybren A. Stüvel requested changes 2024-09-10 16:17:58 +02:00
@ -38,0 +75,4 @@
continue;
}
AnimData *adt = BKE_animdata_from_id(related_id);

This logic can be replaced with a call to Action *animrig::get_action(ID &animated_id);

This logic can be replaced with a call to `Action *animrig::get_action(ID &animated_id);`
ChrisLend marked this conversation as resolved
@ -38,0 +101,4 @@
case ID_KE: {
/* Shapekeys. */
/* Find a mesh using this shapekey. */

If this ID is a shapekey, you can just follow the Key::from pointer to find the mesh. No need to do a full scan. And this would also nicely unify the mesh-shapekeys and curve-shapekeys cases.

If this ID is a shapekey, you can just follow the `Key::from` pointer to find the mesh. No need to do a full scan. And this would also nicely unify the mesh-shapekeys and curve-shapekeys cases.
ChrisLend marked this conversation as resolved
@ -38,0 +137,4 @@
case ID_NT: {
/* bNodeTree. */
/* Only allow embedded IDs. */

As I said in an earlier comment, finding the owner ID of an embedded ID should just use BKE_id_owner_get(). There is no need to loop over everything here.

As I said in an earlier comment, finding the owner ID of an embedded ID should just use `BKE_id_owner_get()`. There is no need to loop over everything here.
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 1 commit 2024-09-10 16:45:10 +02:00
Christoph Lendenfeld added 1 commit 2024-09-10 17:03:36 +02:00
Christoph Lendenfeld added 1 commit 2024-09-10 17:13:28 +02:00
Merge branch 'main' into layered_action_create_once
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.
8bb4043e63
Author
Member

@blender-bot build

@nathanvegdahl I completely missed your point before.
The relation Mesh <-> Shapekey is now valid even though the Mesh may be used multiple times.

@blender-bot build @nathanvegdahl I completely missed your point before. The relation Mesh <-> Shapekey is now valid even though the Mesh may be used multiple times.
Christoph Lendenfeld added 1 commit 2024-09-10 17:23:59 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-09-10 17:24:33 +02:00
Sybren A. Stüvel requested changes 2024-09-10 18:34:29 +02:00
Sybren A. Stüvel left a comment
Member

Some more things I noticed while browsing through the code. I do want to try it out first for myself before a final accept (and already looking forward to that, this PR is pretty much the most-asked-for with the whole slotted actions feature!)

Some more things I noticed while browsing through the code. I do want to try it out first for myself before a final accept (and already looking forward to that, this PR is pretty much the most-asked-for with the whole slotted actions feature!)
@ -38,0 +86,4 @@
switch (GS(related_id->name)) {
case ID_OB: {
Object *ob = (Object *)related_id;
BLI_assert(ob != nullptr);

I think this assertion can be removed. If this one were to fail, get_action(*related_id); would have already crashed.

I think this assertion can be removed. If this one were to fail, `get_action(*related_id);` would have already crashed.
dr.sybren marked this conversation as resolved
@ -38,0 +118,4 @@
case ID_NT: {
/* bNodeTree. */
/* Only allow embedded IDs. */
if (!(related_id->flag & ID_FLAG_EMBEDDED_DATA)) {

I think this check for embedded IDs can be done outside of this switch, as it doesn't use the fact that this is a node tree. If then in the future other IDs get embedded, things keep working.

I think this check for embedded IDs can be done outside of this `switch`, as it doesn't use the fact that this is a node tree. If then in the future other IDs get embedded, things keep working.
dr.sybren marked this conversation as resolved
@ -38,0 +130,4 @@
break;
}
case ID_ME: {

I'm missing the curves shapekeys

I'm missing the curves shapekeys
dr.sybren marked this conversation as resolved
Christoph Lendenfeld added 1 commit 2024-09-12 09:54:27 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-09-13 10:23:24 +02:00
Christoph Lendenfeld added 2 commits 2024-09-13 10:24:33 +02:00
Sybren A. Stüvel requested changes 2024-09-13 13:03:55 +02:00
Sybren A. Stüvel left a comment
Member

find_related_action() should IMO also go over particle systems. I'll attach a test file. The active object (Particle Cube) has a legacy particle system with a texture.

  • In the Properties editor, ensure the Texture tab is active.
  • Influence panel: insert key on General Time
  • Colors panel: insert key on Multiply R
  • Animation panel: see that there are different panels for the texture and the particle system. This is OK as textures are reusable things, just like materials are.
  • 3D Viewport: key the Object.
  • The Object now gets a separate Action. I think it'll be good if this is shared too, in the case that the particle settings ID has a single user.

When it comes to the naming of the Action, I'm not entirely sure it's working how I'd expect as an animator. But I'm also not 100% about this yet. My gut feeling tells me it would be nice if the Action gets named after the most top-level thing it could be shared with. So when keying a shape key, if the mesh is only used by one object, the Action would be shared with that object (if the object gets keyed). So in that case, following my logic, the action would get named after the object.

As I said, it's more of a gut feeling right now, and I can see that there could be some unexpected side-effects of this. What do you think Christoph?

`find_related_action()` should IMO also go over particle systems. I'll attach a test file. The active object (`Particle Cube`) has a legacy particle system with a texture. - In the Properties editor, ensure the Texture tab is active. - Influence panel: insert key on General Time - Colors panel: insert key on Multiply R - Animation panel: see that there are different panels for the texture and the particle system. **This is OK** as textures are reusable things, just like materials are. - 3D Viewport: key the Object. - The Object now gets a separate Action. I think it'll be good if this is shared too, in the case that the particle settings ID has a single user. When it comes to the naming of the Action, I'm not entirely sure it's working how I'd expect as an animator. But I'm also not 100% about this yet. My gut feeling tells me it would be nice if *the Action gets named after the most top-level thing it could be shared with*. So when keying a shape key, if the mesh is only used by one object, the Action would be shared with that object (if the object gets keyed). So in that case, following my logic, the action would get named after the object. As I said, it's more of a gut feeling right now, and I can see that there could be some unexpected side-effects of this. What do you think Christoph?
@ -38,0 +124,4 @@
case ID_ME: {
add_object_data_users(bmain, *related_id, related_ids);
Mesh *mesh = (Mesh *)related_id;
if (mesh->key) {

The check for the shapekey can also be generalised by calling BKE_key_from_id(related_id).
That should allow you to remove the special cases for ID_ME and ID_CU. And it'll also make sure that it automatically handles the ID_LT case all three of us missed ;-)

The check for the shapekey can also be generalised by calling `BKE_key_from_id(related_id)`. That should allow you to remove the special cases for `ID_ME` and `ID_CU`. And it'll also make sure that it automatically handles the `ID_LT` case all three of us missed ;-)
Author
Member

nice, another function I didn't know existed

nice, another function I didn't know existed
dr.sybren marked this conversation as resolved
@ -59,0 +182,4 @@
if (action == nullptr) {
/* init action name from name of ID block */
char actname[sizeof(id->name) - 2];
if (id->flag & ID_FLAG_EMBEDDED_DATA) {

This should also treat the shapekey datablock as "embedded", even though it isn't.

This should also treat the shapekey datablock as "embedded", even though it isn't.
dr.sybren marked this conversation as resolved
Author
Member

When it comes to the naming of the Action, I'm not entirely sure it's working how I'd expect as an animator.

@dr.sybren hmm not sure I agree with that. I feel like that's trying to be a bit too smart. The relations can change at any moment so I don't think this is a good solution.
I would put it that way. As a user you either care about the name or you don't.

  • If you care about it, you need to rename it anyway. ArmatureAction is as useful as KeyAction or MaterialAction
  • If you don't care about it you will not have a look at it anyway

Edit: writing that kind of sparked an idea. Maybe we shouldn't name the action after the thing it animates. Just Action.001, Action.002 is just as good. And since an action can now animate more than 1 thing potentially more appropriate. You need to look at the slots anyway to know what is being animated

> When it comes to the naming of the Action, I'm not entirely sure it's working how I'd expect as an animator. @dr.sybren hmm not sure I agree with that. I feel like that's trying to be a bit too smart. The relations can change at any moment so I don't think this is a good solution. I would put it that way. As a user you either care about the name or you don't. * If you care about it, you need to rename it anyway. `ArmatureAction` is as useful as `KeyAction` or `MaterialAction` * If you don't care about it you will not have a look at it anyway Edit: writing that kind of sparked an idea. Maybe we shouldn't name the action after the thing it animates. Just `Action.001`, `Action.002` is just as good. And since an action can now animate more than 1 thing potentially more appropriate. You need to look at the slots anyway to know what is being animated
Christoph Lendenfeld added 1 commit 2024-09-13 14:21:28 +02:00
Christoph Lendenfeld added 1 commit 2024-09-13 14:36:12 +02:00

I would put it that way. As a user you either care about the name or you don't.

  • If you care about it, you need to rename it anyway. ArmatureAction is as useful as KeyAction or MaterialAction
  • If you don't care about it you will not have a look at it anyway

Edit: writing that kind of sparked an idea. Maybe we shouldn't name the action after the thing it animates. Just Action.001, Action.002 is just as good. And since an action can now animate more than 1 thing potentially more appropriate. You need to look at the slots anyway to know what is being animated

I don't quite agree here. When an Action is used to bundle highly-related things together (like camera object+data, material+nodetree, or mesh+shapekey) it makes sense (at least to me) to pick the name that represents the "highest level". Your PR already does this for embedded IDs.

Maybe it's enough to expand the embedded ID handling with a special case for shape keys, to treat those as "embedded" as well when it comes to naming. By default an object and its data have the same name anyway, and so then it is already less noticable which one we pick.

> I would put it that way. As a user you either care about the name or you don't. > * If you care about it, you need to rename it anyway. `ArmatureAction` is as useful as `KeyAction` or `MaterialAction` > * If you don't care about it you will not have a look at it anyway > > Edit: writing that kind of sparked an idea. Maybe we shouldn't name the action after the thing it animates. Just `Action.001`, `Action.002` is just as good. And since an action can now animate more than 1 thing potentially more appropriate. You need to look at the slots anyway to know what is being animated I don't quite agree here. When an Action is used to bundle highly-related things together (like camera object+data, material+nodetree, or mesh+shapekey) it makes sense (at least to me) to pick the name that represents the "highest level". Your PR already does this for embedded IDs. Maybe it's enough to expand the embedded ID handling with a special case for shape keys, to treat those as "embedded" as well when it comes to naming. By default an object and its data have the same name anyway, and so then it is already less noticable which one we pick.
Christoph Lendenfeld added 1 commit 2024-09-13 15:36:44 +02:00
use Key owner when generating action name
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.
db0e930e44
Author
Member

@blender-bot build

@dr.sybren I use the pointer Key->from now to generate the action name now. Seems like a reasonable tradeoff. Though that means the action name will follow the object data which is hardly ever in sync with the actual object name

@blender-bot build @dr.sybren I use the pointer `Key->from` now to generate the action name now. Seems like a reasonable tradeoff. Though that means the action name will follow the object data which is hardly ever in sync with the actual object name
Sybren A. Stüvel approved these changes 2024-09-13 16:39:13 +02:00
Sybren A. Stüvel left a comment
Member

LGTM! Just two very minor notes that can be handled when landing.

LGTM! Just two very minor notes that can be handled when landing.
@ -38,0 +129,4 @@
if (node_tree && ID_REAL_USERS(&node_tree->id) == 1) {
related_ids.append_non_duplicates(&node_tree->id);
}
Key *key = BKE_key_from_id(related_id);

IMO the Key *key = ... line can be moved one line down (swapping that line with the empty line below it). That'll separate the 'node group handling' and 'shape key handling' from each other.

IMO the `Key *key = ...` line can be moved one line down (swapping that line with the empty line below it). That'll separate the 'node group handling' and 'shape key handling' from each other.
@ -59,0 +169,4 @@
if (action == nullptr) {
/* init action name from name of ID block */
char actname[sizeof(id->name) - 2];
if (id->flag & ID_FLAG_EMBEDDED_DATA) {

I think it would be good to also guard the naming changes behind USER_EXPERIMENTAL_TEST.

I think it would be good to also guard the naming changes behind `USER_EXPERIMENTAL_TEST`.
Christoph Lendenfeld added 1 commit 2024-09-13 16:46:54 +02:00
Christoph Lendenfeld added 1 commit 2024-09-13 17:23:04 +02:00
support for legacy particle systems
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-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
f413b59ae5
Author
Member

@blender-bot build

action sharing to and from particle systems

@blender-bot build action sharing to and from particle systems
Christoph Lendenfeld merged commit cb6ed12ef1 into main 2024-09-13 17:57:40 +02:00
Christoph Lendenfeld deleted branch layered_action_create_once 2024-09-13 17:57:43 +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
4 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#126655
No description provided.