Instead of using `BKE_library_foreach_ID_link()` as a way to find
embedded IDs in a generic way, explicitly just get the embedded node
tree. That's the only animatable embeddable ID anyway. And calling
`BKE_library_foreach_ID_link()` can have some unwanted side-effects
(especially when the rebuilding happens while already using a similar
function to loop over IDs).
Pull Request: https://projects.blender.org/blender/blender/pulls/131807
`animrig::Slot::users_remove()` now also works when there are multiple
occurrences of the same ID. Because of the above-mentioned pointer
remapping now working correctly, the pointers in the slot user list
are not 100% under control of the Slot class, and thus there could
hypothetically be duplicates there.
This doesn't fix any concrete issues, it's just a safety measure.
Pull Request: https://projects.blender.org/blender/blender/pulls/131806
The generic Action assignment function tries to find a slot to
auto-assign. This would always look at the last-used slot identifier on
the ID's `AnimData` struct, even when assigning to an NLA strip or
Action constraint.
This commit removes the `Action::find_suitable_slot(ID)` method, and
replaces it with a `generic_slot_for_autoassign(ID, action,
last_slot_identifier)` function. That function basically copies the
behaviour of `find_suitable_slot()`, except that it gets the
`last_slot_identifier` from the caller.
Another difference is that it no longer checks whether the Action is
already assigned, and so also never uses the currently-assigned slot
handle. In the only code flow that calls `generic_slot_for_autoassign()`
this situation would never occur, and thus it's better to delete this
dead code.
Pull Request: https://projects.blender.org/blender/blender/pulls/131491
When a new Action slot is created by keying a property, it is now
named after the last-assigned slot. This is in support of the
following scenario:
- Action `A` is assigned, with slot `Legacy Slot`.
- The slot is renamed to `Main Light`, because that's what being
animated by it.
- Animator wants to try an alternative animation, and unassigns the
Action.
- Animator starts keying the light, which creates Action `B` and a
slot.
- This slot is now also named `Main Light`, independently of the
actual name of the light being animated.
- Animator can switch between actions `A` and `B`, and because the
slots have the same name, the auto-assignment Just Works™.
Pull Request: https://projects.blender.org/blender/blender/pulls/131600
The `Action::last_slot_handle` field is set to a high-ish value, to
disambiguate slot handles from array indices.
Slot handles are opaque integers, and are just used to find a slot
with that particular handle. Its exact value is irrelevant; Blender
only ensures that slot handles are never reused within the same
Action.
This particular value was obtained by taking the 31 most significant
bits of the TentHash value (Nathan's hash function) for the string
"Quercus&Laksa" (Sybren's cats).
Pull Request: https://projects.blender.org/blender/blender/pulls/131310
We've had a bunch of inconsistency between `channel_bag` and `channelbag` in the
code base. After discussion with @dr.sybren, we decided to standardize on
`channelbag` and also rename the camelcase `ChannelBag` to `Channelbag` to be
consistent with that.
This PR implements those changes.
Note that the reason we standardized on `channelbag` rather than `channel_bag`
is because it makes things clearer when stringing multiple terms together in
type and function names. For example, in `channelbag_fcurves_move()` it makes
it clear that `channelbag` describes one thing, rather than `channel` and `bag`
being two separate things.
No functional changes intended.
Pull Request: https://projects.blender.org/blender/blender/pulls/130988
`Action.slots.new()` in the Python API previously took either an ID or nothing
as a parameter. In the former case it would create a slot with the appropriate
`id_root` and name for that ID. In the latter case it would create a default
slot with an unspecified `id_root` and default name.
This had several issues:
1. You couldn't create a slot with a specific `id_root` without already having
an ID of that type. In theory this isn't a problem, but in practice in larger
scripts/addons you don't necessarily have such an ID on hand at the call
site.
2. You couldn't directly create a slot with a desired name without an existing
ID with that name. This isn't so important, since you can always just set the
name afterwards. But it's a bit annoying.
3. Most other `new()` APIs in Blender *require* you to specify the name of the
item being created. So calling this with no parameters was violating that
norm.
4. Ideally, we want to eliminate unspecified `id_root`s, since they cause other
weirdness in the API such as slot identifiers changing upon slot assignment.
To resolve these issues, and just generally to make the API more
straightforward, this PR changes `slots.new()` to take two required parameters:
an ID type and a name. For example:
`slots.new(id_type='CAMERA', name="My Camera Data Slot")`.
This fully specifies everything needed for the slot identifier upon creation,
and doesn't require any outside data items to create a slot with the desired
type and name.
In the future if we decide we still want a `for_id`-style slot creation API, we
can reintroduce it as a separate function.
Ref: #130892
Pull Request: https://projects.blender.org/blender/blender/pulls/130970
`AnimData`, NLA strips, and action constraints all have an `action_slot_name`
field in RNA. The purpose of this field is to store the identifier of the most
recently assigned slot, so that it can be used for auto-assignment when later
assigning different actions.
However, this field name is misleading in two ways:
1. In accordance with #130740, it's actually the slot *identifier*, not name.
2. It could be mistaken as a way to rename the currently assigned slot, which it
is not.
To resolve both of those issues, we're renaming the field to
`last_slot_identifier`, which better communicates its actual nature.
As a bonus, this also ends up decluttering Python autocomplete when looking
for things related to action_slot.
Ref: #130892
Pull Request: https://projects.blender.org/blender/blender/pulls/130911
This PR renames `ActionSlot::name` to `ActionSlot::identifier` for both DNA and
RNA, and also renames related methods, functions, constants, and comments.
The purpose of this rename is to help make it clear that this is not a "name"
in the traditional sense, but rather is a composite of the slot name + id type
for lookup purposes.
Ref: #130892
Pull Request: https://projects.blender.org/blender/blender/pulls/130740
No functional changes intended.
Modify the code so that no `ED_` includes are needed in animrig.
The function `reevaluate_fcurve_errors` was just moved to its only used
place and made static.
For `animdata_fcurve_delete` the `bAnimContext` argument
is no longer needed because it was only used to check if an `FCurve` is
for a driver which you can also get from `FCurve.driver`.
**Note** that this still leaves `../editors/include` in `CMakeLists.txt`
but that is needed for bone colors.
Pull Request: https://projects.blender.org/blender/blender/pulls/130338
The root cause of this bug can be traced to:
- `ANIM_nla_mapping_get(ac, ale)` conditionally returns an `AnimData *adt`.
- This can be `nullptr` in various cases, depending on the editor (in `ac`) and
the type & source of data (in `ale`).
- This `nullptr` has different meanings:
1. There is not enough information to return an `adt` (like `ac` or `ale`
being `nullptr` themselves).
2. NLA time remapping should not be done. For example for NLA control F-Curves
(like animated strip influence), or Grease Pencil (because that doesn't use
the NLA).
- The above-returned `adt` is passed to other functions. Some of them are aware
of the "`nullptr` means no NLA time remapping" scenario, and gracefully handle
it. Other code, however, just gets "an adt" from the caller and handles it as
normal (and likely crashes on `nullptr`). Other cases start out as the first,
but somewhere in the call stack shift over to the second.
The approach taken in this PR to fix the bug is to (generally) stop signaling
"do not use NLA time remapping" via `adt = nullptr`, and instead explicitly
indicate/check whether remapping should be done.
In most cases this means passing a `bAnimListElem *` instead of an `AnimData *`,
because the former has the information needed to determine if time remapping
should be done or not. However, in some cases there is no `bAnimListElem *` to
pass, and instead other information determines whether remapping is needed. In
those cases we add a `bool` parameter or field in the appropriate place so that
calling code can explicitly indicate whether remapping should be done or not.
To accomplish this a variety of functions have been added to help handle things
correctly. Of particular note:
- `AnimData *ANIM_nla_mapping_get(ac, ale)` (that conditionally returned an
`adt`) has been removed entirely in favor of the new
`bool ANIM_nla_mapping_allowed(ale)` function that simply returns whether
nla remapping should be done or not.
- `ANIM_nla_tweakedit_remap(ale, …)` has been added, which wraps
`BKE_nla_tweakedit_remap(adt, …)` and only performs the remapping when
`ANIM_nla_mapping_allowed()` indicates that it's allowed.
- `ANIM_nla_mapping_apply_if_needed_fcurve(ale, …)` has been added, which is an
alternative to `ANIM_nla_mapping_apply_fcurve(adt, …)` that also only performs
the remapping when `ANIM_nla_mapping_allowed()` indicates that it's allowed.
Note that even with this PR there are still a couple of places remaining that
use `adt = nullptr` to indicate "don't remap", because they appear to be correct
and would require larger changes to make explicit. In those cases comments have
been added to explain the situation, with a reference to this PR. In the future
we way want to take the time to change those as well.
Also of minor note: this PR moves the definition of the type `slot_handle_t`
from ANIM_action.hh to BKE_action.hh. This is due to `BKE_nla.hh` (which needs
that definition) now being included directly and indirectly in a lot more
places. Moving the definition to BKE_action.hh prevents all of those new places
from gaining dependencies on the animrig module.
Co-authored-by: Sybren A. Stüvel <sybren@blender.org>
Pull Request: https://projects.blender.org/blender/blender/pulls/130440
The `fmt::format` can process the format string at compile time. Currently, we
don't seem to be using that as we don't use `FMT_STRING`. Starting with C++20,
that will be the default though, and one has to explicitly opt out in places
where the string is not known at compile time using `fmt::runtime(...)`.
Currently, our code does not compile as C++20 because of that. Unfortunately, we
have many places with runtime format strings, because of i18n.
Pull Request: https://projects.blender.org/blender/blender/pulls/130392
No functional changes intended.
After moving the keying set code to animrig with b38d8ecb86,
a few things needed cleaning up.
* Improving comments
* adding `const` where possible
* simplify code in some areas
Pull Request: https://projects.blender.org/blender/blender/pulls/130314
Fix an assertion that an embedded data-block has a zero 'real user'
count. Apparently it's possible for the shader node tree (embedded by
the material) to have a user count of 1.
Since that looks valid to me (only one user, namely the material itself)
I think it's fine to extend the assertion to that.
I did keep the assertion, to ensure that the embedded data-block is not
shared by multiple users. That shouldn't be possible, in any case.
Pull Request: https://projects.blender.org/blender/blender/pulls/130281
No functional changes intended.
This patch moves the relevant keying set code from editors to animrig.
All functions are in the animrig namespace, and as such have lost their
`ANIM_` prefix.
Other than that, the code has been moved as is into `animrig/intern/keyingsets.cc`
Note that I also had to move `id_frame_has_keyframe` and `fcurve_frame_has_keyframe`.
I moved that into `ANIM_keyframing.hh` and `ANIM_fcurve.hh` since I found that more fitting.
Due to Windows defining `DELETE` as macro I had to rename `ModifyKeyMode::DELETE`
to `ModifyKeyMode::DELETE_KEY`
As a result of this two includes from animrig to editors were removed.
This is part of #121336
Pull Request: https://projects.blender.org/blender/blender/pulls/129980
For C/C++ doc-strings should be located in headers,
move function comments into the headers, in some cases merging
with existing doc-strings, in other cases, moving implementation
notes into the function body.
The playback FPS drop was caused by a debug `printf()` call. Its' been
replaced with a call to `CLOG_INFO()` with a log level of `4`, so that it's
available when necessary but no longer eats up performance due to the I/O.
Pull Request: https://projects.blender.org/blender/blender/pulls/129685
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
