This updates the layered action data model to store strip data differently. Specifically:
- `Strip` is now just a single, POD type that only stores the data common to all
strips, such as start/end frames.
- The data that might be of a completely different nature between strips (e.g.
keyframe data vs modifier data) is now stored in arrays on the action itself.
- `Strip`s indicate their type with an enum, and specify their data with an
index into the array on the action that stores data for that type.
This approach requires a little more data juggling, but has the advantage of
making `Strip`s themselves super simple POD types, and also opening the door to
trivial strip instancing later on: instances are just strips that point at the
same data.
The intention is that the RNA API remains the same: from RNA's perspective there
is no data storage separate from the strips, and a strip's data is presented as
fields and methods directly on the strip itself. Different strip types will be
presented as different subtypes of `ActionStrip`, each with their own fields and
methods specific to their underlying data's type. However, this PR doesn't
implement that sub-typing, leaving it for a future PR. It does, however, put the
fields and methods of the one strip type we have so far directly on the strip,
which avoids changing the APIs we have so far.
This PR implements the bulk of this new approach, and everything should be
functional and working correctly. However, there are two TODO items left over
that will be implemented in forthcoming PRs:
- Type refinement in the RNA api. This PR actually removes the existing type
refinement code that was implemented in terms of the inheritance tree of the
actual C++ types, and this will need to be reimplemented in terms of the new
data model. The RNA API still works without the type refinement since there
are only keyframe strips right now, but it will be needed in preparation for
more strip types down the road.
- Strip data deletion. This PR only deletes data from the strip data arrays when
the whole action is deleted, and otherwise just accumulates strip data as more
and more strips are added, never removing the data when the corresponding
strips get removed. That's fine in the short term, especially since we only
support single strips right now. But it does need to be implemented in
preparation for proper layered actions.
Pull Request: https://projects.blender.org/blender/blender/pulls/126559
Code would try to use a random ID as a legacy curve one, without any
check on the actual ID type. When trying to open
`tests/data/io_tests/blend_big_endian/2.30/dolphin.blend` recently added
to our collection of regression tests, this ended up 'converting' a
shapekey into a legacy curve ID, leading to invalid memory access
detected by ASAN.
The Build Modifier was not copying the ID pointer to the object.
This resulted in the object field being empty when loading a GPv2
file.
The fix makes sure the ID pointer is copied.
In the `blender::animrig` namespace, add two new functions
`generic_assign_action()` and `generic_assign_action_slot()`. These are
now used as basis for (un)assigning Actions and Action slots, both
directly on the ID and for NLA strips.
The method `Action::assign_id()` has been removed, in favour of free
functions for (un)assigning Actions and slots.
This is almost a non-functional change. The only thing that's different
is when both an Action and a slot should get assigned in one go.
Old behaviour: the slot would be inspected, and if not suitable for the
animated ID, the entire operation would be aborted. In other words, any
previously-assigned Action would still be assigned, making this an
atomic operation.
New behaviour: assigning an Action + a slot is no longer atomic. First
the new Action is assigned (which uses the automatic slot selection
based on the name). If after this the given slot cannot be assigned, the
new Action + the auto-selected slot are still kept.
This makes the Action & slot assignment more uniform in behaviour, where
"assign this Action + this slot" as one operation behaves the same as
two sequential operations "assign this Action" + "assign this slot". If
it turns out to be confusing, we can always improve things.
The return code for slot assignment is now more explicit (enum instead
of boolean), so that the caller can decide what to do in case of which
error (like unassigning the slot if the automatic behaviour is
unwanted).
Pull Request: https://projects.blender.org/blender/blender/pulls/127712
It was possible for a Grease Pencil object to have a material
with no Grease Pencil settings. This can lead to crashes in many
places because it's often assumed that the material has
these settings.
The fix makes sure that `BKE_object_material_get` returns
a Grease Pencil material when called on a Grease Pencil
object. If the Grease Pencil settings don't exist,
it returns `nullptr`.
In the future, it should be possible to have these materials
and code that reads from the settings should fall back to
the default material.
Pull Request: https://projects.blender.org/blender/blender/pulls/127570
Saving files could take ~3-4 seconds on debug builds because of new
imbuf scaling logic.
Even though debug performance usually isn't much of a consideration,
it gets in the way of development.
Since thumbnails don't require the same accuracy as the sequencer or
compositor, use a faster scaling method that uses a box-filter clamped
to integer bounds & integer math.
In practice the difference between the resulting thumbnails isn't
noticeable in my tests.
For debug build with ASAN this gives a ~25x speedup,
for release builds it gives a ~1.4x speedup which is to be
expected with a more approximate scaling method.
Avoid a division by zero when the Action constraint has the same value
for its 'min' and 'max' parameters.
When `min` and `max` are equal, the code will now choose either `min`
(when `target value` ≤ `min`) or max (`target value` > `max`).
Pull Request: https://projects.blender.org/blender/blender/pulls/127583
To avoid unnecessary looping over listbase items the function
`BLI_listbase_count_at_most` was used however it resulting in an awkward
expression: `BLI_listbase_count_at_most(list, count + 1) == count`
replace this with `BLI_listbase_count_is_equal_to(list, count)`.
BKE_id_copy_in_lib took an argument `r_newid` which was was to
initialize `new_id` in the functions body.
While this may not have caused any user visible bugs, it's error prone,
one caller even passed in an uninitialized pointer.
- Rename `r_newid` to `new_id_p` since it's not a return argument.
- Initialize the value from all callers.
The tricky thing here is that this graph is only generated while geometry nodes
is evaluated and is generally only stored temporarily. To make it accessible via
Python, the accessor method will cause a reevaluation specifically to log the
generated graph.
Updating attributes is now done specifically for each attribute,
and topology and visibility updates tag the draw cache data
explicitly too. That makes checking for updates with these
tags unnecessary.
Similar to the previous commits, previously all GPU buffers
were recreated when only masks changed. Now only
that masks are re-uploaded, using the same mechanism.
Part of #118145.
Similar to 7b69c82494.
We didn't make any good use of the time in between changing
mask values and recalculating whether nodes were fully masked
or not masked at all. There was no point in the lazy calculation.
Eager calculation should be faster too, for the same reason as
the above commit.
Part of #118145.
Similar to the previous commit, previously all GPU buffers
were recreated when only a color attribute changed. Now only
that attribute is re-uploaded, using the same mechanism.
Part of #118145.
Similar to the previous commit, previously all GPU buffers
were recreated when only face sets changed. Now only face
sets are re-uploaded, using the same mechanism.
Part of #118145.
Previously tagging node positions dirty set the `PBVH_UpdateDrawBuffers`
flag which was then used by `node_draw_update_mask` and
`tag_all_attributes_dirty`, which, as hinted by the name, causes a refresh
of all the PBVH GPU vertex buffers, not just positions and normals.
Instead of using that flag, add a function to the draw cache to tag only
position-related data dirty. This should give a performance improvement
when there are also face sets, masks, and/or generic attributes being
extracted for drawing.
Part of $118145.
Also remove noisy print when a thumbnail can't be generated from an
animation. Similar prints have been commented and mainly seem useful
for debugging.
Replace the `PBVH_UpdateBB` flag with a separate vector of booleans.
The simplest impact of this is that we can tell when there are no changed
nodes in constant time. Besides that, it also works better with the recent
IndexMask usage for selections of nodes, and allows better const
correctness too, since nodes themselves don't have to be changed
when positions are changed. This also leads into plans to change PBVH
draw data tagging to avoid having to reupload all attributes when only
one changes.
Part of #118145.
Pull Request: https://projects.blender.org/blender/blender/pulls/127587
When syncing the NLA strip length with the Action it uses, use the
length of the assigned action slot. Previously the entire Action was
considered when determining the length.
Pull Request: https://projects.blender.org/blender/blender/pulls/127573
Move the following BKE functions to the `animrig::Action` class. Some of
those will be extended to support slots in a future commit; for now they
still operate on all F-Curves in the Action.
| Old | New |
|---------------------------------|-------------------------------------|
| `BKE_action_frame_range_calc()` | `Action::get_frame_range_of_keys()` |
| `BKE_action_frame_range_get()` | `Action::get_frame_range()` |
| `BKE_action_has_motion()` | `Action::has_keyframes()` |
| `BKE_action_has_single_frame()` | `Action::has_single_frame()` |
| `BKE_action_is_cyclic()` | `Action::is_cyclic()` |
Implementations have been copied from the BKE functions. The frame range
functions now return `float2` instead of requiring two `float *r_…`
return parameters.
The `has_motion` function is now renamed to `has_keyframes`, as that is
what the implementation was actually testing for.
The functions now no longer are null-safe. The BKE functions handled a
null action pointer, but IMO that doesn't make sense, and in none of the
call sites I could find where this would actually be valid.
No functional changes.
Ref: #127489
Pull Request: https://projects.blender.org/blender/blender/pulls/127512
