This commit takes the 'Slotted Actions' out of the experimental phase.
As a result:
- All newly created Actions will be slotted Actions.
- Legacy Actions loaded from disk will be versioned to slotted Actions.
- The new Python API for slots, layers, strips, and channel bags is
available.
- The legacy Python API for accessing F-Curves and Action Groups is
still available, and will operate on the F-Curves/Groups for the first
slot only.
- Creating an Action by keying (via the UI, operators, or the
`rna_struct.keyframe_insert` function) will try and share Actions
between related data-blocks. See !126655 for more info about this.
- Assigning an Action to a data-block will auto-assign a suitable Action
Slot. The logic for this is described below. However, There are cases
where this does _not_ automatically assign a slot, and thus the Action
will effectively _not_ animate the data-block. Effort has been spent
to make Action selection work both reliably for Blender users as well
as keep the behaviour the same for Python scripts. Where these two
goals did not converge, reliability and understandability for users
was prioritised.
Auto-selection of the Action Slot upon assigning the Action works as
follows. The first rule to find a slot wins.
1. The data-block remembers the slot name that was last assigned. If the
newly assigned Action has a slot with that name, it is chosen.
2. If the Action has a slot with the same name as the data-block, it is
chosen.
3. If the Action has only one slot, and it has never been assigned to
anything, it is chosen.
4. If the Action is assigned to an NLA strip or an Action constraint,
and the Action has a single slot, and that slot has a suitable ID
type, it is chosen.
This last step is what I was referring to with "Where these two goals
did not converge, reliability and understandability for users was
prioritised." For regular Action assignments (like via the Action
selectors in the Properties editor) this rule doesn't apply, even though
with legacy Actions the final state ("it is animated by this Action")
differs from the final state with slotted Actions ("it has no slot so is
not animated"). This is done to support the following workflow:
- Create an Action by animating Cube.
- In order to animate Suzanne with that same Action, assign the Action
to Suzanne.
- Start keying Suzanne. This auto-creates and auto-assigns a new slot
for Suzanne.
If rule 4. above would apply in this case, the 2nd step would
automatically select the Cube slot for Suzanne as well, which would
immediately overwrite Suzanne's properties with the Cube animation.
Technically, this commit:
- removes the `WITH_ANIM_BAKLAVA` build flag,
- removes the `use_animation_baklava` experimental flag in preferences,
- updates the code to properly deal with the fact that empty Actions are
now always considered slotted/layered Actions (instead of that relying
on the user preference).
Note that 'slotted Actions' and 'layered Actions' are the exact same
thing, just focusing on different aspects (slot & layers) of the new
data model.
The "Baklava phase 1" assumptions are still asserted. This means that:
- an Action can have zero or one layer,
- that layer can have zero or one strip,
- that strip must be of type 'keyframe' and be infinite with zero
offset.
The code to handle legacy Actions is NOT removed in this commit. It will
be removed later. For now it's likely better to keep it around as
reference to the old behaviour in order to aid in some inevitable
bugfixing.
Ref: #120406
The versioning code that upgrades legacy actions to new slotted actions
also needs to properly assign slots to the IDs that use those upgraded
actions. It was doing this correctly except for not traversing into and
assigning slots to embedded IDs.
This commit adds the code to handle embedded IDs as well.
Additionally, this changes how mismatched `id_type`s are handled when upgrading
actions. Rather than refusing to assign the slot created during the upgrade if
the `id_type` doesn't match the ID, we assign it anyway with a warning. The
rationale is that this represents a case where the Action `idroot` was already
mismatched, and it turns out that has always been possible. So we now opt to
simply preserve that state of affairs rather than attempt to "fix" it.
Pull Request: https://projects.blender.org/blender/blender/pulls/129002
Building the F-Curve cache used for pose flipping now also works with
slotted Actions. Like the pose library itself, it only considers the first
slot of the pose asset. Multi-slot pose assets are not supported.
Pull Request: https://projects.blender.org/blender/blender/pulls/128992
When creating a new NLA strip for an action, as well as when setting
`strip.action` via RNA, use the generic action-assignment code. This
ensures that the slot selection follows the same logic as other Action
assignments.
If the generic slot selection doesn't find a suitable slot, and there is
a single slot on that Action of a suitable ID type, always assign it.
This is to support the following scenario:
- Python script creates an Action and adds F-Curves via the legacy API.
- This creates a slot 'XXSlot'.
- The script creates multiple NLA strips for that Action.
- The desired result is that these strips get the same Slot assigned as
well.
The generic code doesn't work for this, because:
- The first strip assignment would see the slot `XXSlot` (`XX`
indicating "not bound to any ID type yet"). Because that slot has
never been used, it will be assigned (which is good). This assignment
would change its name to, for example, `OBSlot`.
- The second strip assignment would not see a 'virgin' slot, and thus
not auto-select `OBSlot`. This behaviour makes sense when assigning
Actions in the Action editor (assigning an Action that already
animates 'Cube' to 'Suzanne' should not assign the 'OBCube' slot to
Suzanne), but for the NLA I feel that it could be a bit more
'enthousiastic' in auto-picking a slot to support the above case.
This is preparation for the removal of the 'Slotted Actions'
experimental flag, and getting the new code to run as compatibly as
possible with the legacy code.
The return value of `animrig::nla::assign_action()` has changed a bit.
It used to indicate whether a slot was auto-selected; it now indicates
whether the Action assignment was successful. Whether a slot was
assigned or not can be seen at `strip.action_slot`.
Pull Request: https://projects.blender.org/blender/blender/pulls/128892
When assigning an Action, and it has only one slot that has never been
assigned to anything before, auto-assign that slot.
This is the last option for the slot auto-selection. It is in place mostly
for backward compatibility in the following situation:
- Python script creates a new Action, and adds F-Curves via the legacy API
`action.fcurves.new(...)`. This automatically creates a slot with
`id_type = 0`, indicating it is not yet bound to any ID type.
- The script assigns the Action to a data-block.
In this case the implicitly created slot is automatically assigned, and
thus the data-block is animated by the F-Curves created through the legacy
API.
Pull Request: https://projects.blender.org/blender/blender/pulls/128892
Cycle-aware keying on slotted Actions now works the same as on legacy
Actions. In the future this will be improved, but for now it's good enough
to have the same behaviour as before.
Pull Request: https://projects.blender.org/blender/blender/pulls/128892
Split the legacy Action handling code of `action_fcurve_ensure()` into a
new function `action_fcurve_ensure_legacy()`. This makes it possible for
unit tests to explicitly create a legacy Action for testing, regardless
of the 'Slotted Actions' experimental feature flag.
When we drop that flag, the unit tests that explicitly test legacy
behaviour will keep running.
Pull Request: https://projects.blender.org/blender/blender/pulls/128892
When inserting a key on a slotted Action, apply NLA remapping on both
the key's time and value.
This pushes the fork in the code (between legacy & new Actions) a bit
further down the call stack.
Ref: #120406
Pull Request: https://projects.blender.org/blender/blender/pulls/128700
Reorder the Action and Slot assignment function declarations, so that
they are grouped a bit more sensibly.
Of the touched functions, I also replaced `ATTR_WARN_UNUSED_RESULT` with
`[[nodiscard]]`.
No functional changes.
Pull Request: https://projects.blender.org/blender/blender/pulls/128695
This is a follow-up to #128363, and fixes up the remaining areas of
the sequencer's code that didn't yet account for slotted actions:
1. Moving strips failed to move their animation with them.
2. Duplicating strips failed to duplicate their animation with them.
3. Deleting strips didn't delete their animation channels with them.
This also takes the opportunity to add depsgraph tagging and
notifiers that were already missing in the pre-slotted-actions
code, for the strip delete and strip paste operators. The absence
of these was making the UI not update and was also causing stale
animation data to get evaluated.
Pull Request: https://projects.blender.org/blender/blender/pulls/128440
Instead of calling `BKE_nlastrip_new()` (which, due to backward compat
reasons automatically picks a slot), the Push Down operator now calls
`BKE_nlastrip_new_for_slot()`, which explicitly assigns the given slot.
On top of that, the frame range of the slot is used to set the strip's
frame range (instead of the range of the entire Action).
Pull Request: https://projects.blender.org/blender/blender/pulls/128444
Instead of using direct property manipulation to enter tweak mode, use
the regular `animrig::assign_…` functions. When used in the right order,
as introduced in this commit, the "disallow assigning an Action in tweak
mode" logic can just stay simple as it is.
Before this, copy-pasting sequencer strips with animation on them would
fail to copy-paste their animation along with them if they were animated
by a slotted action. This fixes that.
There are three remaining known issues in the sequencer when used with
slotted actions that this PR does not fix, and will be addressed in a
follow-up PR:
1. Moving strips fails to move the animation of their fcurves with them,
which it should.
2. Duplicating strips (as distinct from copying followed by pasting)
fails to duplicate their animation with them.
3. Deleting a strip does not delete its animation channels with it.
Pull Request: https://projects.blender.org/blender/blender/pulls/128363
The key insertion code is assigning the target Action to the keyed ID,
which isn't allowed while in NLA tweak mode. This is now skipped when
the Action is already assigned.
Pull Request: https://projects.blender.org/blender/blender/pulls/128446
Remove some `#ifdef WITH_ANIM_BAKLAVA` guards to make a unit test
succeed. The code is handling slots when assigning Actions to IDs.
Non-experimental builds will only deal with legacy Actions, and thus the
slot assignment is a no-op anyway.
The unit test is explicitly creating layered Actions and running tests
on them. Since this also covers some new code introduced for Action
assignments (unified code for both legacy & layered Actions), I'd feel
more comfortable keeping the test enabled.
Note that when loading a blend file with layered Actions in a
non-experimental build will actually load them as legacy Actions.
Pull Request: https://projects.blender.org/blender/blender/pulls/128483
Mark these copy constructors as 'explicit' in `blender::animrig`:
- `Slot`
- `StripKeyframeData`
- `ChannelBag`
The copy constructors for the other related classes were already
`explicit` or `deleted`.
This prevents bugs by disallowing implicit copies. For example:
```cpp
ChannelBag cbag = agrp->channel_bag->wrap();
```
This should have been a reference (`ChannelBag &cbag`), an easy mistake
which is now caught by the compiler (and fixed in this commit).
No functional changes. The implicit copy that was removed was just
inefficient, but didn't produce the wrong results.
Pull Request: https://projects.blender.org/blender/blender/pulls/128424
Previously it only filtered on a specific rna path-based criteria.
This commit changes it to take a predicate function instead, splitting
off its previously fixed criteria into a separate function.
This also renames it to `fcurves_in_action_slot_filtered()` to better
reflect its new generalized functionality.
No functional changes intended.
Pull Request: https://projects.blender.org/blender/blender/pulls/128423
Add support for `rna_struct.keyframe_insert(…, group="name")` parameter,
when inserting keys into a layered Action.
This simply was never implemented, and the default channel group name
was always used.
Pull Request: https://projects.blender.org/blender/blender/pulls/128383
Calling `action.fcurves.clear()` would clear the F-Curves, but didn't
update the F-Curve groups. This meant that the groups data still had their
original length & offsets into the F-Curves array, causing subsequent
F-Curve creation to crash Blender.
The unit test for this also covers the previous commit.
Pull Request: https://projects.blender.org/blender/blender/pulls/128375
Fix an issue where Python's `rna_struct.keyframe_insert(path, index=-1)`
would not create any keys. This was caused by the new layered animation
code not taking -1 as an 'all array elements' wild-card.
The accompanying unit test will follow in another commit, as its success
depends on another bugfix as well.
Users of the layered Actions API should never mutate the data via these
spans, and so the functions should always return just a `Span<>`.
No functional changes.
Pull Request: https://projects.blender.org/blender/blender/pulls/128264
`BKE_animdata_fix_paths_remove()` now calls into the new function
`animrig::legacy::action_fcurves_remove()`, which works for both
legacy and slotted Actions.
Also I updated the documentation of the BKE function so that it's clear
what it's actually doing.
No functional changes for legacy Actions.
Ref: #123424
Pull Request: https://projects.blender.org/blender/blender/pulls/128252
When assigning an Action with the new API:
- check the Actions ID type (if it is a legacy Action), and
- check whether the ID is in NLA tweak mode.
This means that action assignment can fail, so the
`animrig::assign_action()` and `animrig::unassign_action()` functions
now return a `[[nodiscard]] bool`.
Part of `generic_assign_action()` has now also been shielded with an
`#ifdef WITH_ANIM_BAKLAVA` just to be sure.
This also includes a change in `BKE_animdata_free()`. That function now
first exits NLA tweak mode (if enabled) before freeing the Actions. This
makes it possible to simply un-assign the assigned Action, as all the
NLA tweakmode stuff has already been taken care of by the responsible
function.
Pull Request: https://projects.blender.org/blender/blender/pulls/128199
This adds an operator that moves slots of slot channels selected in
the channel box to a new action.
All slots are moved together into that new action instead of
moving them into separate actions.
I think that's more reasonable because that way the "move slots into separate actions" is
still possible by selecting the slots one by one.
The "explode action" operator that does just that can be a separate operator.
The new action is named after the slot for the case when only one slot is moved.
When multiple slots are moved, the name "CombinedAction" is used.
This also adds a menu entry "Action" to the action editor.
Pull Request: https://projects.blender.org/blender/blender/pulls/128171
Previously we were using a bespoke hodgepodge of
`Action::is_action_legacy()` and `Action::is_action_layered()`,
sometimes in combination with checking for the Baklava feature flag,
when what we really meant is "Should this action be treated as legacy
or not?"
This commit changes the places where that's semantically what we meant
to use `action_treat_as_legacy()`. Some of those places were already
correct, using a compound conditional, but some of them weren't, and
thus were not always branching correctly. For those latter cases,
this commit is a bug fix.
Importantly, not all uses of bare `Action::is_action_legacy()` or
`Action::is_action_layered()` are semantically incorrect: there are many
places where that is the right thing to do. This commit takes care not
to touch those places.
Pull Request: https://projects.blender.org/blender/blender/pulls/128174
Use the `foreach_action_slot_use(id, callback)` iterator function to find
all action/slot uses of an ID. That already handles the action constraint,
and thus solves the issue.
Pull Request: https://projects.blender.org/blender/blender/pulls/128090
Add slotted/layered Action support to `get_active_actiongroup()` and
`set_active_action_group()`.
Note that there is still a bunch of code around that directly manipulates
the action group flags, instead of using these functions. That's for
another commit to address.
This commit also introduces the functions `action_treat_as_legacy()` and
`channel_groups_all(action)` utility functions in the `animrig::legacy`
namespace.
Ref: #123424
Pull Request: https://projects.blender.org/blender/blender/pulls/128084
The collada export code was directly using `action->curves` in its
export code, which doesn't work with slotted actions. This commit
updates that code to use wrapper functions that provide access to the
correct fcurves regardless of whether the action is slotted or not.
Note that the import code has not yet been updated, and is left for
a future PR.
Ref: #123424
Pull Request: https://projects.blender.org/blender/blender/pulls/128061
Convert the Grease Pencil legacy converter code to the new layered
Action API.
Unfortunately this cannot just work on legacy Actions (and then rely on
the versioning to layered Actions we'll land soon), as the GP conversion
runs as the final step of the versioning process. This cannot be helped,
as the usual versioning is not allowed to create new IDs and this does.
Ref: #123424
Pull Request: https://projects.blender.org/blender/blender/pulls/128064
Rename `action_foreach_fcurve` to `foreach_fcurve_in_action_slot`. This
matches the functionality better, and opens up the floor for an iterator
that actually does loop over all F-Curves in an Action.
No functional changes.
Pull Request: https://projects.blender.org/blender/blender/pulls/128065
`BKE_fcurves_filter()` was only used by the Symmetrize Armature
operator, when dealing with Action Constraints on the bones.
`BKE_fcurves_filter()` is now removed, and has been replaced by the new
`animrig::fcurve_find_in_action_slot_filtered()`. It's functionally
pretty much the same, except that it returns a `Vector<FCurve *>`
instead of manipulating a `ListBase` parameter.
Because it's now implemented using an iterator in
`ANIM_action_iterators.hh` it didn't feel right to keep the function in
BKE.
No functional changes.
Fix the code that decides whether to create a new F-Curve in a legacy or
layered style. The old code went for layered if:
- The experimental feature is enabled AND the Action is already layered.
The new code goes for layered if:
- The experimental feature is enabled AND the Action is empty, OR
- The Action is already layered.
Pull Request: https://projects.blender.org/blender/blender/pulls/128036
Replace those calls to `BKE_fcurve_find()` that are searching in the
curves of an Action with their corresponding call in `animrig`:
- `animrig::fcurve_find_in_action()` when it should really search
through the entire Action,
- `animrig::fcurve_find_in_action_slot()` when only the F-Curves for a
specific slot should be searched, or
- `animrig::fcurve_find_in_assigned_slot()` same as above, searching
through the action slot that is assigned to the given ADT.
This also makes `animrig::fcurve_find_in_action()` compatible with both
layered and legacy Actions.
This makes the naming a bit closer to the `BKE_fcurve_find()` function,
and opens op the naming for two upcoming functions
`fcurve_find_in_action_slot` and `fcurve_find_in_assigned_slot`.
No functional changes.
Pull Request: https://projects.blender.org/blender/blender/pulls/128036
Remove the declaration of `blender::animrig::legacy::action_has_fcurves()`.
Its implementation was never committed to `main`, and the declaration also
shouldn't have.
No functional changes.
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
Pull Request: https://projects.blender.org/blender/blender/pulls/128026
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.
Pull Request: https://projects.blender.org/blender/blender/pulls/128030
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.
Pull Request: https://projects.blender.org/blender/blender/pulls/128030
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.
Pull Request: https://projects.blender.org/blender/blender/pulls/128030
