Using clang-tidy "cppcoreguidelines-pro-type-cstyle-cast" to find them.
This PR touches `bf_animrig` & `bf_editor_animation` & `bf_editor_armature`
NOTE: Also some case where untouched (not straightforward) same as
expanded macros (e.g. `LISTBASE` or `GS`)
NOTE: a couple of cases of "inconsistent-declaration-parameter" as well
Pull Request: https://projects.blender.org/blender/blender/pulls/136044
In the Action Editor and Dopesheet, Action/Slot summaries erroneously showed
keys that were hidden in their sub-channels.
The root cause was that the functions for building Action/Slot summary key
lists, `action_to_keylist()` and `action_slot_to_keylist()`, weren't doing
animation filtering *at all*, instead just looping over all f-curves.
This PR fixes the bug by replacing `action_slot_to_keylist()` with a new
`action_slot_summary_to_keylist()` function, which calls into the animation
filtering code to determine which f-curves to include in their key list, and
also using it in place of `action_to_keylist()` at the relevant call sites.
Additionally, properly filtering the channels for the summaries requires both a
`bAnimContext` and the ID that's being animated, so this PR threads those
through to `action_slot_summary_to_keylist()`.
Note that `action_to_keylist()` is still used in some places that actually
*want* an unfiltered list of keys. Those call sites have been left as-is, and
`action_to_keylist()` remains for that purpose.
Pull Request: https://projects.blender.org/blender/blender/pulls/134922
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
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
This implements the proposal from #124512. For that it contains the following
changes:
* Remove the global override of `new`/`delete` when `WITH_CXX_GUARDEDALLOC` was
enabled.
* Always use `MEM_CXX_CLASS_ALLOC_FUNCS` where it is currently used. This used
to be guarded by `WITH_CXX_GUARDEDALLOC` in some but not all cases. This means
that a few classes which didn't use our guarded allocator by default before,
are now using it.
Pull Request: https://projects.blender.org/blender/blender/pulls/130181
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
This PR adds channel groups (also known as fcurve groups or action groups) to
layered actions. For layered actions, these groups belong to the `ChannelBag`s
and can vary by bag.
From a user perspective, the goal is for these to function just like channel
groups from legacy actions. However, internally they are implemented a little
differently: legacy actions store both channel groups and fcurves in a listbase,
and groups indicate what fcurves are in them with a listbase that points
directly into the larger fcurve listbase. Layered actions, on the other hand,
store both fcurves and channel groups in an array, and groups indicate what
fcurves are in them by indexing into the fcurve array.
Despite taking this different approach, we still reuse the `bActionGroup` struct
for the new channel groups, just adding the necessary fields for index-based
fcurve membership as described above.
This PR does not implement all of the functionality needed to reach feature
parity with legacy action channel groups, but implements the main core and gets
them basically working.
It's easier to list the things that *haven't* been implemented yet:
- Operators for letting the user manually create/remove/move channel groups.
- Keyframe selection in the action/dopesheet editor on channel group rows
themselves are not yet working correctly.
- Handling channel groups in legacy/layered action conversion operators.
- Making the legacy `action.groups` property work on single-layer-single-strip
layered actions.
Those are left for future PRs. Other than that, in theory everything should be
working now.
Pull Request: https://projects.blender.org/blender/blender/pulls/125774
The issue was caused by blender/blender@f3b393a74a
Before that commit, the keylist would be built including 1 keyframe
outside the range on each side (if there was one).
After that those would be ignored, leading to the following issue
Fixes#125861
The fix is to ensure the keylist is built with 1 key outside the given range.
Pull Request: https://projects.blender.org/blender/blender/pulls/125807
The issue occurred when creating a keylist for an FCurve.
Keys outside the given range are ignored, but the call to `update_keyblocks`
still got the full range leading to the assertion.
The fix is to keep track of the index range for which bezt have been added
to the keycolumns. Note that we have to do that within the loop because
the FCurve might not be ordered (e.g. during transforms in the dope sheet)
Pull Request: https://projects.blender.org/blender/blender/pulls/125678
Rename "Animation data-block" to "Action" or "Layered Action", where
appropriate. Some uses of the term actually refer to the `AnimData`
struct, in which case they were left as-is.
No real functional changes, just changing some messages & descriptions.
Pull Request: https://projects.blender.org/blender/blender/pulls/124170
Rename 'Binding' to 'Slot'. The old term was causing all kind of
confusion, and 'slot' was considered to be a better term for the
intended functionality.
This commit breaks existing blend files that were using the new layered
Action for their animation. The animation data will be lost due to the
rename, as there is no versioning code or DNA renaming logic. At this
time the new system is still marked as experimental, so shouldn't be
used for anything serious anyway.
Pull Request: https://projects.blender.org/blender/blender/pulls/124170
In the Action mode of the Dope Sheet, show the Bindings, optionally
showing all Bindings in that Action. This also works when different
data-block types are animated from the same Action.
Bindings are selectable in the Action editor with box/range select.
Since there is no use for an 'active Binding' yet, this has not been
implemented, and as a result, clicking on a Binding does nothing.
Note on 'Binding' vs 'Slot': this code change was created before the
decision to change the name from 'Bindings' to 'Slots'. To avoid
confusion, and to keep this PR in sync with the code changes, it still
uses the term 'binding'. Renames of 'Binding' to 'Slot' in the code will
happen after this PR lands.
Pull Request: https://projects.blender.org/blender/blender/pulls/122672
The issue was introduced with f06fd85d97 where the building of the keylist used was
restricted to the visible range of the dope sheet.
The optimization uses `BKE_fcurve_bezt_binarysearch_index` which can only work when
the FCurve is sorted. During transformations in the dope sheet this is not the case as the
sorting only happens when the transformation is applied.
The fix is to do a range check in the for loop instead of a binary search.
Testing the range comes with a performance impact though.
| Before optimization | broken with f06fd85d97 | this PR |
| - | - | - |
| 90ms | 6ms | 20ms |
An alternative solution would be to sort FCurves during transformations of the Dope Sheet.
This is done in the Graph Editor and with the recent speedups introduced there this could be
a viable option. However this is out of scope for this fix.
Pull Request: https://projects.blender.org/blender/blender/pulls/123824
The new/experimental, layered `Animation` data-block is merged with the
existing `bAction` data-block.
The `Animation` data-block is considerably newer than `bAction`, so the
supporting code that was written for it is also more modern. When moving
that code into `bAction`, I chose to keep the modernity where possible,
and thus some of the old code has been updated as well. Things like
preferring references over pointers.
The `Animation` data-block is now gone from DNA, the main database, etc.
As this was still an experimental feature, there is no versioning code
to convert any of that to Actions.
The DNA struct `bAction` now has a C++ wrapper `animrig::Action`, that
can be obtained via `some_action->wrap()`.
`animrig::Action` has functions `is_empty()`, `is_action_legacy()`, and
`is_action_layered()`. They **all** return `true` when the Action is
empty, as in that case none of the data that makes an action either
'legacy' or 'layered' is there.
The 'animation filtering' code (for showing things in the dope sheet,
graph editor, etc) that I wrote for `Animation` is intentionally kept
around. These types now target 'layered actions' and the
already-existing ones 'legacy actions'. A future PR may merge these two
together, but given how much work it was to add something new there, I'd
rather wait until the dust has settled on this commit.
There are plenty of variables (and some comments) named `anim` or
`animation` that now are of type `animrig::Action`. I haven't renamed
them all, to keep the noise level low in this commit (it's already big
enough). This can be done in a followup, non-functional PR.
Related task: #121355
Pull Request: https://projects.blender.org/blender/blender/pulls/121357
Add minimal support for the `Animation` data-block to the anim filtering
code. This means:
- F-Curves in the `Animation` data-block are shown in the Dope Sheet and
Graph Editor.
- The `Animation` is shown underneath each animated data-block, just
like an `Action` would be.
- Contrary to Actions, the expanded/collapsed state is stored on
`id->adt` and not on `Animation`. Because an `Animation` is intended
to be used by multiple data-blocks, they each should have their own
flag for this.
- In the filtering code, this PR adds the 'fillanim' channel type. This
is simply mimicking the name from the Action's 'fillact' type.
Limitations:
- Deleting of F-Curves is explicitly blocked, as that'll be introduced
in a later PR. The block prevents Blender from crashing.
- The Dope Sheet doesn't have an Animation mode yet, that'll be for a
later PR too.
Pull Request: https://projects.blender.org/blender/blender/pulls/120654
Instead of using `short key_type`, use `eBezTriple_KeyframeType key_type`,
so that it's clear which type it is, and so that a `switch()` can cause
compiler warnings when it's incomplete.
This also adds missing `case`s to `switch`es where necessary, in a way
that doesn't affect the outcome. There is one change that looks like it
is a functional change, but it should provide the same result:
```diff
- size -= 0.8f * key_type;
+ size *= 0.8f;
```
Since `size = 12` and in this case `key_type = 3`, the numerical values
are the same, but now the code is consistently multiplying and thus should
scale properly.
Furthermore some overly obvious comments are removed and some missing
`const` keywords have been added.
No functional changes.
Pull Request: https://projects.blender.org/blender/blender/pulls/120178
# Issue
Having a lot of keys in your scene can dramatically slow down the Dope Sheet.
That is because everytime the Dope Sheet is redrawn,
the Keylists have to be recomputed.
In the case of the summary channel, that means going through
all keyframes of all the `FCurves` and adding them to the keylist.
That eats up 95% of the time it takes to draw a frame.
# This PR
It's not a perfect solution, rather it solves the performance issue
for the case when you are not displaying all keys.
Instead of going through all the keys, only add the
keys visible in the view to the keylist.
This speeds up the Dope Sheet significantly
depending on the zoom level.
This also improves the responsiveness when selecting and transforming keyframes.
# Performance changes
The function measured is `ED_channel_list_flush`
which is responsible for building the keylists and drawing them.
The test setup contains 62 bones with all
10 channels keyed (location, rot quaternion, scale) on 6000 frames.
So 3.720.000 keys. The heavier the dataset the bigger the performance impact.
The data was recorded with only the Dope Sheet open, and 3 channels visible
* Summary
* Object
* Action
The more channels are visible, the greater the performance gain. This can be seen in the video.
| visible range | before | after |
| - | - | - |
| 200f | 250ms | 10ms |
| 400f | 250ms | 18ms |
| 3000f | 250ms | 130ms |
| 6000f | 250ms | 250ms |
Pull Request: https://projects.blender.org/blender/blender/pulls/114854
No functional changes.
Clean up the code by
* returning early where possible
* removing comments that just repeat what the code does
* Make sure comments follow the style guidelines
Pull Request: https://projects.blender.org/blender/blender/pulls/114851
No functional changes.
While looking at the Dope Sheet drawing code to see if it
can be made faster I came across a bunch of functions with confusing names.
This PR tries to address that by:
* removing the `ED_` prefix from static functions
* adding it to non static functions
* renaming `AnimKeylistDrawListElem` to `ChannelListElement`
* renaming `AnimKeylistDrawList` to `ChannelDrawList`
* all `draw_..._channel` renamed to `ED_add_..._channel`
since the function itself doesn't actually do any drawing
* generally simplifying the function names
Pull Request: https://projects.blender.org/blender/blender/pulls/111510
Update of the following operators to take into account
grease pencil frames :
* SCREEN_OT_keyframe_jump, which jumps to the average
position of the selected frames, and
* ACTION_OT_frame_jump, which jumps to the previous/next
frame in the active channel.
Pull Request: https://projects.blender.org/blender/blender/pulls/111476
Listing the "Blender Foundation" as copyright holder implied the Blender
Foundation holds copyright to files which may include work from many
developers.
While keeping copyright on headers makes sense for isolated libraries,
Blender's own code may be refactored or moved between files in a way
that makes the per file copyright holders less meaningful.
Copyright references to the "Blender Foundation" have been replaced with
"Blender Authors", with the exception of `./extern/` since these this
contains libraries which are more isolated, any changed to license
headers there can be handled on a case-by-case basis.
Some directories in `./intern/` have also been excluded:
- `./intern/cycles/` it's own `AUTHORS` file is planned.
- `./intern/opensubdiv/`.
An "AUTHORS" file has been added, using the chromium projects authors
file as a template.
Design task: #110784
Ref !110783.
Using ClangBuildAnalyzer on the whole Blender build, it was pointing
out that BLI_math.h is the heaviest "header hub" (i.e. non tiny file
that is included a lot).
However, there's very little (actually zero) source files in Blender
that need "all the math" (base, colors, vectors, matrices,
quaternions, intersection, interpolation, statistics, solvers and
time). A common use case is source files needing just vectors, or
just vectors & matrices, or just colors etc. Actually, 181 files
were including the whole math thing without needing it at all.
This change removes BLI_math.h completely, and instead in all the
places that need it, includes BLI_math_vector.h or BLI_math_color.h
and so on.
Change from that:
- BLI_math_color.h was included 1399 times -> now 408 (took 114.0sec
to parse -> now 36.3sec)
- BLI_simd.h 1403 -> 418 (109.7sec -> 34.9sec).
Full rebuild of Blender (Apple M1, Xcode, RelWithDebInfo) is not
affected much (342sec -> 334sec). Most of benefit would be when
someone's changing BLI_simd.h or BLI_math_color.h or similar files,
that now there's 3x fewer files result in a recompile.
Pull Request #110944
This patch includes keyframes for both the summary channel, and grease pencil data-block channels.
It also includes selection in these channels, in the same modes as the ones implemented for grease pencil keyframes.
Pull Request: https://projects.blender.org/blender/blender/pulls/110962
A lot of files were missing copyright field in the header and
the Blender Foundation contributed to them in a sense of bug
fixing and general maintenance.
This change makes it explicit that those files are at least
partially copyrighted by the Blender Foundation.
Note that this does not make it so the Blender Foundation is
the only holder of the copyright in those files, and developers
who do not have a signed contract with the foundation still
hold the copyright as well.
Another aspect of this change is using SPDX format for the
header. We already used it for the license specification,
and now we state it for the copyright as well, following the
FAQ:
https://reuse.software/faq/
Names such as `dummyult` and `dummyet` didn't read very well,
underscore separate these names.
Also rename some variables that used old conventions which missed
being updated (`mpr` for manipulator instead of `gz` for gizmo).
Use a consistent style for declaring the names of struct members
in their declarations. Note that this convention was already used in
many places but not everywhere.
Remove spaces around the text (matching commented arguments) with
the advantage that the the spell checking utility skips these terms.
Making it possible to extract & validate these comments automatically.
Also use struct names for `bAnimChannelType` & `bConstraintTypeInfo`
which were using brief descriptions.
This is the conventional way of dealing with unused arguments in C++,
since it works on all compilers.
Regex find and replace: `UNUSED\((\w+)\)` -> `/*$1*/`
The filter was missing in some places that are using channel data as if it was f-curve channel.
There seems to be no related issue or bug, but still it would be best to have them there.
Reviewed By: sybren
Differential Revision: http://developer.blender.org/D15505
Motion paths can now be initialised to more sensible frame ranges,
rather than simply 1-250:
- Scene Frame Range
- Selected Keyframes
- All Keyframes
Reviewed By: sybren, looch, dfelinto, pablico
Maniphest Tasks: T93047
Differential Revision: https://developer.blender.org/D13687
This reverts commit 1558b270e9.
An earlier commit (rB101fadcf6b93c) introduced some new functionality,
which was overlooked in reviewing this commit & got broken.
Will re-commit after the issue has been fixed.
Ref: D13687
Fix an assert by commenting out the assert.
In normal situations all keyframes are sorted. However, while keys are
transformed, they may change order and then this assertion no longer
holds. The effect is that the drawing isn't perfect during the
transform; the "constant value" bars aren't updated until the
transformation is confirmed. Apart from that, the code runs fine, so it
seems like a workable workaround.
