A pass at cleaning up / improving the code documentation for the layered action
C++ classes.
There is still more work to do here, but this should be good for an initial
first pass. I've focused on clarifying the behavior of and updating stale docs
of the methods in the action-related C++ classes.
Pull Request: https://projects.blender.org/blender/blender/pulls/131318
The issue was that we were passing a pointer-to-a-pointer to
`BLI_addtail()`, which expects a pointer to something castable to a
`Link`. This in turn led to an invalid memory access when trying to
access the fields of the supposed `Link`.
This fixes the issue by passing a pointer to a zero-initialized `Link`
instead.
This also takes the opportunity to more simply zero-initialize the
`bAction` structs used in the tests as well.
Pull Request: https://projects.blender.org/blender/blender/pulls/132860
Empty legacy Actions were skipped from versioning, because they are
technically valid layered Actions as well. However, this created an
issue where there was no "Legacy Slot" created for those. Furthermore,
their `idroot` could still be set to a non-zero value, which is not
allowed on layered Actions.
`bAction::idroot` is not a very reliable field. It can be zero on legacy
Actions as well, and it can in certain cases be downright wrong (can
happen in certain linking scenarios). Because of this, the field is not
included in any "is this a layered or a legacy Action?" checks.
Pull Request: https://projects.blender.org/blender/blender/pulls/132757
Split the versioning of legacy Actions to slotted ones into two steps:
1. Versioning the Actions themselves, in the regular versioning code
(`do_versions_after_linking_400`). This has to happen in the 'after
linking' stage, and not in the 'before linking' stage, as there's
older 'after linking' code that will break when it gets fed slotted
actions. Any ID that is using a legacy Action will get tagged.
2. Versioning Action assignments, where the correct Action Slot has to
be chosen & assigned for each tagged ID. This has to happen in the
`do_versions_after_setup` stage, as choosing a slot requires that the
actions (both local & linked) have been converted already.
This also includes some necessary changes to the pre-2.50 Action versioning
code.
Note that this change does not handle library overrides. That's dealt
with in !131426.
Pull Request: https://projects.blender.org/blender/blender/pulls/131627
The code for converting pre-Animato actions was not getting run properly
because `chanbase` (where animation data used to be stored) was
erroneously getting cleared before the relevant versioning code was run.
The root cause was that the code checking whether an action was already
valid as a layered action or not was NOT confirming that `chanbase` was
empty as part of that check (as it is a DNA-deprecated field), which in
turn triggered code that defensively clears `chanbase` (among other
things) when an action is identified as layered.
Note that the conversion of IPO curves and other pre-Animato data
happens quite late in the versioning, even _after_ the "versioning after
linking" stage. This is not introduced in this commit, this is just to
illuminate pre-existing design that might not be entirely obvious.
Pull Request: https://projects.blender.org/blender/blender/pulls/131975
When using clangd or running clang-tidy on headers there are
currently many errors. These are noisy in IDEs, make auto fixes
impossible, and break features like code completion, refactoring
and navigation.
This makes source/blender headers work by themselves, which is
generally the goal anyway. But #includes and forward declarations
were often incomplete.
* Add #includes and forward declarations
* Add IWYU pragma: export in a few places
* Remove some unused #includes (but there are many more)
* Tweak ShaderCreateInfo macros to work better with clangd
Some types of headers still have errors, these could be fixed or
worked around with more investigation. Mostly preprocessor
template headers like NOD_static_types.h.
Note that that disabling WITH_UNITY_BUILD is required for clangd to
work properly, otherwise compile_commands.json does not contain
the information for the relevant source files.
For more details see the developer docs:
https://developer.blender.org/docs/handbook/tooling/clangd/
Pull Request: https://projects.blender.org/blender/blender/pulls/132608
This caused build errors on the docs builder, I can't seem to reproduce
locally, so revert for now and have another look at some point in the
future.
Sadly as these changes usually go, this took 5c515e26bb and
2f0fc7fc9f with it as well.
Pull Request: https://projects.blender.org/blender/blender/pulls/132559
The issue is that certain RNA paths cannot be generated to come
from an ID and `RNA_path_from_ID_to_property` will return no value.
We are using that function in the keyframing code to allow passing
in a pointer to a bone and a path relative to that bone.
Since there is currently no good way to find the path from the ID to an arbitrary
struct pointer (see #122427), this patch is a workaround that uses
the struct_pointer IF that happens to be an ID pointer.
Of course that still has the core limitation in place but until a
better solution is available on the RNA side this is the best we can do.
Pull Request: https://projects.blender.org/blender/blender/pulls/132552
This patch makes the internal functions for the pose library aware of action slots.
* Allows to apply poses with more than 1 slot.
* The slot is chosen based on a best guess, with a fallback to the first slot.
Not in this patch:
There is no straightforward way to create multi slot pose assets yet. That will come later.
It is possible to manually tag an action with more than 1 slot as an asset though.
When applying poses, only the active object is modified. Multi object editing support will come later.
Part of Design #131840
Pull Request: https://projects.blender.org/blender/blender/pulls/132161
Action Slots point to the IDs they animate, and after swapping IDs they
also need some swappage (as the Action that first animated `id_a` will
now animate `id_b`, and vice versa).
Instead of doing this in the `id_swap()` function (and requiring
knowledge of how that's supposed to be done), just mark these pointers
as dirty so that they're rebuilt at first use.
This commit contains a little more code than strictly necessary, to add
a function in BKE for this cache invalidation. This avoids having to
have a dependency on the animrig module just for this purpose.
Fixes: #130136
Pull Request: https://projects.blender.org/blender/blender/pulls/131809
The crash happened because `bNodeSocket.runtime` was a nullptr.
That was because the struct pointer was passed instead of the
resolved pointer. Passing the resolved pointer into `BKE_animsys_nla_remap_keyframe_values`
fixes the issue
Pull Request: https://projects.blender.org/blender/blender/pulls/132012
The autokeying code for cameras used the keyingset code to insert keys.
In the case of "Only Insert Available" turned on this would use the "Available" keyingset.
However, in the case of looking through the camera and moving the viewport
when the camera is not active, the poll function of that keyingset would return false.
Instead of modifying the poll function, the fix is to use the more direct keying code
using `RNAPath`.
This can be backported to 4.2 but not 3.6 due to the changes to the keying code done in 4.0
Pull Request: https://projects.blender.org/blender/blender/pulls/131796
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
