Anim: migrate Action assignments to the new API #128026

Sybren A. Stüvel · 2024-09-23T13:04:47+02:00

Sybren A. Stüvel commented

2024-09-23 13:04:47 +02:00

Instead of assigning Actions by direct pointer manipulation (and the
corresponding juggling of user counts), call animrig::assign_action()
and animrig::unassign_action().

These functions not only correctly handle user counts, but also ensure
that slot assignments & user tracking works. The former always happens,
the latter only when building with experimental features enabled.

Because (un)assigning slotted Actions need the animated ID (instead of
just the AnimData *), more functions now require an OwnedAnimData.

Note that there is still some user count juggling. This is caused by
BKE_id_new(), and by extension BKE_action_add, returning an ID with
user count = 1, even though that ID is not yet used. A todo task #128017
has been made to change BKE_action_add() so that the Action it returns
can be directly fed into animrig::assign_action().

This PR updates the following areas:

Creating a node group by grouping animated nodes, as this has to move
the animation data. This PR just handles the assignment of a new
Action.
Temporary Action creation for ungrouping node groups.
Versioning of pre-2.5 animation data.

No functional changes.

Ref: #123424

This PR sits on top of #128030, will rebase later.

Instead of assigning Actions by direct pointer manipulation (and the corresponding juggling of user counts), call `animrig::assign_action()` and `animrig::unassign_action()`. These functions not only correctly handle user counts, but also ensure that slot assignments & user tracking works. The former always happens, the latter only when building with experimental features enabled. Because (un)assigning slotted Actions need the animated ID (instead of just the `AnimData *`), more functions now require an `OwnedAnimData`. Note that there is still some user count juggling. This is caused by `BKE_id_new()`, and by extension `BKE_action_add`, returning an ID with user count = 1, even though that ID is not yet used. A todo task #128017 has been made to change `BKE_action_add()` so that the Action it returns can be directly fed into `animrig::assign_action()`. This PR updates the following areas: - Creating a node group by grouping animated nodes, as this has to move the animation data. This PR just handles the assignment of a new Action. - Temporary Action creation for ungrouping node groups. - Versioning of pre-2.5 animation data. No functional changes. Ref: #123424 ------- This PR sits on top of #128030, will rebase later.

Sybren A. Stüvel added the

Module

Animation & Rigging

label 2024-09-23 13:04:47 +02:00

Sybren A. Stüvel added 5 commits 2024-09-23 13:04:56 +02:00

Cleanup: remove unnecessary call to BKE_animdata_from_id() 3e60c85f0a

The call was followed by `BKE_animdata_ensure_id(id)`, which already
returns any existing animdata.

No functional changes.

Refactor: BKE_animdata_action_ensure_idroot compat with layered Actions cc95aaba68

Make `BKE_animdata_action_ensure_idroot()` do the right thing when a
layered Action is passed in.

Anim: add slotted Action support to animdata_copy_id_action d400dfd9a1

Instead of simply reassigning the `adt->action` and `adt->tmpact` pointers,
the code now uses the `animrig::assign_action()` and `assign_tmpaction()`.
This way the slot user maps should be properly updated.

Refactor: Anim, improve Action assignment functions ed7330cd60

The `animrig::assign_action(action, id)` function now takes a `bAction`
pointer (instead of its C++ wrapper `animrig::Action`). This makes it
easier to call from code that just deals with the DNA/C struct.

Also an override was added that takes an `OwnedAnimData` struct instead
of just the ID. This saves the lookup of the AnimData, to be used in cases
where the caller already has it.

Anim: correctly assign Action in id_action_ensure()

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

e6c2688e61

Fix two bugs by replacing direct assignment to `adt->action` with a call
to `animrig::assign_action()`:

- If a related Action was found, its user count was not incremented.
- If a new Action is created, its `idroot` was not properly set.

Sybren A. Stüvel commented

2024-09-23 13:05:20 +02:00

@blender-bot build

Sybren A. Stüvel force-pushed anim/refactor-action-assignments from bc57f6994d to 7c2fd9f964

2024-09-23 14:10:16 +02:00

Compare

Sybren A. Stüvel changed title from ~~anim/refactor-action-assignments~~ to Anim: migrate Action assignments to the new API

2024-09-23 14:10:36 +02:00

Sybren A. Stüvel requested review from Nathan Vegdahl 2024-09-23 14:13:34 +02:00

Nathan Vegdahl approved these changes 2024-09-23 15:25:39 +02:00

Nathan Vegdahl left a comment

Just one place where I think the code could be clearer, which can be addressed while landing. Otherwise looks good, and kicking the tires a bit didn't reveal any issues.

source/blender/blenkernel/intern/anim_data.cc Outdated

						
				@ -746,3 +761,1 @@

				      id_us_min(&dstAdt->action->id);

				      dstAdt->action = BKE_action_add(bmain, dstAdt->action->id.name + 2);

				      BKE_animdata_action_ensure_idroot(dstID, dstAdt->action);

				      if (dstAdt->action) {

Nathan Vegdahl commented

2024-09-23 15:01:54 +02:00

If I understand correctly, what this is really saying is if (dstAdt->action == srcAdt->action). But you have to reason about the outer if's conditions to see that.

I think this can be refactored to be clearer by changing this to if (dstAdt->action == srcAdt->action) and moving it above the if (!dstAdt->action || dstAdt->action == srcAdt->action) block. And then also changing that to just if (!dstAdt->action).

In other words, the flow would then be:

If dst action == src action, issue warning and unassign action from dst.
If dst has no action assigned, create and assign one.

If I understand correctly, what this is really saying is `if (dstAdt->action == srcAdt->action)`. But you have to reason about the outer if's conditions to see that. I think this can be refactored to be clearer by changing this to `if (dstAdt->action == srcAdt->action)` and moving it above the `if (!dstAdt->action || dstAdt->action == srcAdt->action)` block. And then also changing *that* to just `if (!dstAdt->action)`. In other words, the flow would then be: 1. If dst action == src action, issue warning and unassign action from dst. 2. If dst has no action assigned, create and assign one.

👍 1

dr.sybren marked this conversation as resolved

Sybren A. Stüvel force-pushed anim/refactor-action-assignments from 7c2fd9f964 to 77d51d1603

2024-09-23 15:49:56 +02:00

Compare

Sybren A. Stüvel added 1 commit 2024-09-23 15:53:03 +02:00

Feedback Nathan 944c6295a3

Sybren A. Stüvel merged commit 6115132998 into main

2024-09-23 15:53:48 +02:00

Sybren A. Stüvel referenced this issue from a commit

2024-09-23 15:53:50 +02:00

Anim: migrate Action assignments to the new API

Sybren A. Stüvel deleted branch anim/refactor-action-assignments

2024-09-23 15:53:51 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Anim: migrate Action assignments to the new API #128026