Anim: Reuse action between related data #126655
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#126655
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ChrisLend/blender:layered_action_create_once"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 commonfunction 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)
Anim: Create layered action once for data and actionto WIP: Anim: Create layered action once for data and actionWIP: Anim: Create layered action once for data and actionto Anim: Create layered action once for data and action@blender-bot build
@nathanvegdahl crash should be fixed, and I added a unit test for this behavior.
Overall, looking good!
I think it would be good to throw in at least one or two other cases. For example:
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)
Maybe call this
find_relevant_action_for_id()
orfind_suitable_action_for_id()
or something like that. At-a-glance justfind_action()
doesn't really suggest it's purpose very well, to me at least.Another possibility just occurred to me:
find_related_action()
. It's right there in your doc comment, and works well I think.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: {
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").
@ -38,0 +47,4 @@
switch (GS(id.name)) {
case ID_OB: {
Object *ob = (Object *)(&id);
if (!ob) {
Can this ever be null? I might be misunderstanding the code, but it looks to me like this could be an assert instead.
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.
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.
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.
Sybren A. Stüvel referenced this pull request2024-09-03 13:23:17 +02:00
once again this crashes the unit tests, but with the recent changes this is less spaghetti and more lasagne so thats good
@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
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,
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 useid
as their data tor_related_ids
. Skips adding IDs that are already inr_related_ids
."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.
@ -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
This doc comment needs to be updated, since it now also searches indirect users.
@ -38,0 +106,4 @@
{
Vector<const ID *> related_ids({&id});
for (int i = 0; i < related_ids.size(); i++) {
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.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
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?/
It's a
Vector
in this case, not a linked list.@ -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)
Probably could use some more unit tests as well to test at least one or two of the more complex cases.
@blender-bot build
I added unit tests covering more action reuse cases
I don't quite get the title. Should this be "for object and its data"? Or does this also cover other cases?
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:
Key::from
pointer.BKE_id_owner_get()
. An embedded ID can be recognised withid->flag & ID_FLAG_EMBEDDED_DATA
.I think you can use, but I'll check with a responsible adult to be sure. (update: no, use something else, see below)BKE_library_foreach_ID_link()
to find those@ -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:
Also
r_related_ids
should be namedrelated_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.@dr.sybren Just to clarify, the behavior you are describing is the following cases
<->
relation//
no relationObject <-> Mesh <-> Shapekey
Object <-> Curve <-> Shapekey
Object <-> Mesh // Material <-> embedded Nodetree
Object // Material // non-embedded Nodetree
Affirmative!
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 usebke::node_tree_ptr_from_id()
(see e.g.id_swap
).Anim: Create layered action once for data and actionto Anim: Reuse action between related data@blender-bot build
Material and its users are no longer related
Find all owners of embedded
bNodeTree
sFind all nodetrees of IDs if they have it.
used the name of the owner ID for embedded IDs
@ -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) {
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.)
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 toME
user count), does not invalide the relationshipME ↔ KE
.Same for the user count of
MA
, that doesn't matter at all when it comes to its embeddedNT
.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.
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
@ -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);
@ -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.@ -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.@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.
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.@ -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.@ -38,0 +130,4 @@
break;
}
case ID_ME: {
I'm missing the curves shapekeys
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.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
andID_CU
. And it'll also make sure that it automatically handles theID_LT
case all three of us missed ;-)nice, another function I didn't know existed
@ -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.
@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.
ArmatureAction
is as useful asKeyAction
orMaterialAction
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 animatedI 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.
@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 nameLGTM! 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.@ -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
.@blender-bot build
action sharing to and from particle systems