Anim: DNA for Animation data-block #119077

Merged
Sybren A. Stüvel merged 1 commits from dr.sybren/blender:pr/baklava-data-model-1-dna into main 2024-03-07 16:41:34 +01:00

This is PR 1 of 7 of a series. All commits in the series are combined in #118677 if you want to test drive the entire thing.

The name Output is likely going to change. For this I want to talk a little bit more with the Blender Studio animators; it is my expectation that we'll settle on 'binding' or 'pairing'. Conceptually it'll remain the same thing. Output has been changed to Binding.

This will be landed as an experimental feature; the flag for this will be introduced in the next commit, when it will actually do something.


Introduce new DNA for the Animation data-block and its sub-data.

This includes the blenkernel code for reading & writing to blend files,
and for memory management (freeing, duplicating). Minimal C++ wrappers
are included, with just the functionality needed for blenkernel to do
its job.

The Outliner code is extended so that it knows about the new data-type,
nothing more.

For more info, see issue #113594.

This is PR 1 of 7 of a series. All commits in the series are combined in #118677 if you want to test drive the entire thing. ~~The name `Output` is likely going to change. For this I want to talk a little bit more with the Blender Studio animators; it is my expectation that we'll settle on 'binding' or 'pairing'. Conceptually it'll remain the same thing.~~ `Output` has been changed to `Binding`. This will be landed as an experimental feature; the flag for this will be introduced in the next commit, when it will actually do something. ------------ Introduce new DNA for the `Animation` data-block and its sub-data. This includes the blenkernel code for reading & writing to blend files, and for memory management (freeing, duplicating). Minimal C++ wrappers are included, with just the functionality needed for blenkernel to do its job. The Outliner code is extended so that it knows about the new data-type, nothing more. For more info, see issue #113594.
Sybren A. Stüvel added 1 commit 2024-03-05 10:12:52 +01:00
ff17b6efba Anim: DNA for `Animation` data-block
Introduce new DNA for the `Animation` data-block and its sub-data.

This includes the blenkernel code for reading & writing to blend files,
and for memory management (freeing, duplicating). Minimal C++ wrappers
are included, with just the functionality needed for blenkernel to do
its job.

The Outliner code is extended so that it knows about the new data-type,
nothing more.

For more info, see issue #113594.
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-03-05 10:13:09 +01:00
Sybren A. Stüvel added this to the 4.2 LTS milestone 2024-03-05 10:13:13 +01:00
Sybren A. Stüvel requested review from Christoph Lendenfeld 2024-03-05 10:46:08 +01:00
Christoph Lendenfeld reviewed 2024-03-05 10:56:03 +01:00
@ -0,0 +1,291 @@
/* SPDX-FileCopyrightText: 2023 Blender Developers

I assume the year should be 2024

I assume the year should be 2024
dr.sybren marked this conversation as resolved
Sybren A. Stüvel requested review from Bastien Montagne 2024-03-05 11:07:40 +01:00
Author
Member

@blender-bot build

@blender-bot build
Christoph Lendenfeld approved these changes 2024-03-05 11:39:23 +01:00
Christoph Lendenfeld left a comment
Member

there are some things that are not clear to me, but nothing showstopping

there are some things that are not clear to me, but nothing showstopping
@ -0,0 +132,4 @@
class Layer : public ::AnimationLayer {
public:
Layer() = default;
Layer(const Layer &other);

why can layers be copied using the copy constructor but not Animation or Strip

why can layers be copied using the copy constructor but not `Animation` or `Strip`
Author
Member

Animation: it is an ID, and so Blender has a bunch of other logic to duplicate those already.

Strip: because of the whacky nature of the inheritance:

  • C-style inheritance: KeyframeAnimationStrip "inherits" AnimationStrip" by embedding the latter. This means that any KeyframeAnimationStrip*can be reinterpreted asAnimationStrip*`.
  • C++-style inheritance: the C++ wrappers inherit the DNA structs, so animrig::Strip inherits ::AnimationStrip, and animrig::KeyframeStrip inherits ::KeyframeAnimationStrip.

Because of this, MEM_new<Strip>(__func__, someKeyframeAnimationStrip) is going to allocate just enough space fo a Strip, and then you have something that the rest of Blender simply won't expect. It'll crash once you set that strip's strip_type property to Strip::Type::Keyframe as it simply won't have enough memory allocated to hold a KeyframeStrip.

`Animation`: it is an `ID`, and so Blender has a bunch of other logic to duplicate those already. `Strip`: because of the whacky nature of the inheritance: - C-style inheritance: `KeyframeAnimationStrip` "inherits" `AnimationStrip" by embedding the latter. This means that any `KeyframeAnimationStrip*` can be reinterpreted as `AnimationStrip*`. - C++-style inheritance: the C++ wrappers inherit the DNA structs, so `animrig::Strip` inherits `::AnimationStrip`, and `animrig::KeyframeStrip` inherits `::KeyframeAnimationStrip`. Because of this, `MEM_new<Strip>(__func__, someKeyframeAnimationStrip)` is going to allocate just enough space fo a `Strip`, and then you have something that the rest of Blender simply won't expect. It'll crash once you set that strip's `strip_type` property to `Strip::Type::Keyframe` as it simply won't have enough memory allocated to hold a `KeyframeStrip`.

Ah ok so ideally we allow the copy constructor but due to weird interactions between C and C++ its not always possible.
make sense thanks :)

Ah ok so ideally we allow the copy constructor but due to weird interactions between C and C++ its not always possible. make sense thanks :)
dr.sybren marked this conversation as resolved
@ -0,0 +148,4 @@
enum class MixMode : int8_t {
Replace = 0,
Offset = 1,

what does Offset do. Is that a time offset?
Or is that what Combine does in the NLA currently

what does `Offset` do. Is that a time offset? Or is that what `Combine` does in the NLA currently
Author
Member

I've added some documentation. Yeah, it does what 'combine' does in the NLA. @nathanvegdahl pointed out that basically all options are some form of "combining" in a way (including "Replace" with a <100% influence), and we figured "Offset" would be clearer (as in, a rotational offset, translational offset, etc.). I hope the comments I added clarify this.

I've added some documentation. Yeah, it does what 'combine' does in the NLA. @nathanvegdahl pointed out that basically all options are some form of "combining" in a way (including "Replace" with a <100% influence), and we figured "Offset" would be clearer (as in, a rotational offset, translational offset, etc.). I hope the comments I added clarify this.
dr.sybren marked this conversation as resolved
@ -0,0 +215,4 @@
/**
* Bag of F-Curves for a specific Output handle.
*/
class ChannelBag : public ::AnimationChannelBag {

I know you've changed the name based on feedback, but reading through the code it wasn't entirely clear to me what a bag is.
Just to clarify: A bag holds the FCurves for a specific output, right.
In that case mentioning output in the name would communicate the purpose better. e.g. OutputStream (hehe because animation channels combine into a stream...)
But given that we are going to change the term output anyway, I am not considering this a showstopper. Just something to keep in mind for later

I know you've changed the name based on feedback, but reading through the code it wasn't entirely clear to me what a bag is. Just to clarify: A bag holds the FCurves for a specific output, right. In that case mentioning output in the name would communicate the purpose better. e.g. `OutputStream` (hehe because animation channels combine into a stream...) But given that we are going to change the term output anyway, I am not considering this a showstopper. Just something to keep in mind for later
Author
Member

We intentionally removed the "for output" part of the name, as that caused more confusion when it came to naming related functions (like KeyframeStrip::channelbag_for_output, which returns the ChannelBag for a specific output). I'll at least rephrase the docstring so that it doesn't repeat the word "bag".

We intentionally removed the "for output" part of the name, as that caused more confusion when it came to naming related functions (like `KeyframeStrip::channelbag_for_output`, which returns the `ChannelBag` for a specific output). I'll at least rephrase the docstring so that it doesn't repeat the word "bag".

I think my gripe with ChannelBag is that it implies that it just holds the data, while in reality it knows which output it is for

I think my gripe with `ChannelBag` is that it implies that it just holds the data, while in reality it knows which output it is for
Author
Member

I agree. It has to be DNA-compatible, though, so that means either we store the key (output handle) with the value (channelbag), or we construct a different way to store the mapping. Ideally we'd use a std::map<output_handle_t, ChannelBag *>, but that's just not an option now.

I agree. It has to be DNA-compatible, though, so that means either we store the key (output handle) with the value (channelbag), or we construct a different way to store the mapping. Ideally we'd use a `std::map<output_handle_t, ChannelBag *>`, but that's just not an option now.
dr.sybren marked this conversation as resolved
@ -1211,0 +1401,4 @@
int32_t output_handle;
int fcurve_array_num;
FCurve **fcurve_array; /* Array of 'fcurve_array_num' FCurves. */

I am curious why this is an array of pointers instead of just an FCurve array.
My first thought is that the bag doesn't own the data, but then who owns it. I didn't see any other new datastructure holding FCurves.

I am curious why this is an array of pointers instead of just an `FCurve` array. My first thought is that the bag doesn't own the data, but then who owns it. I didn't see any other new datastructure holding FCurves.
Author
Member

Good point, I think this can be changed to an array of FCurves.

Good point, I think this can be changed to an array of FCurves.
Author
Member

On second thought, it's better to keep these as pointers. This means that the array itself can be reallocated or reshuffled, while keeping the FCurves themselves untouched. This also means that references from Python remain valid, and that reorganising things is a bit more efficient.

On second thought, it's better to keep these as pointers. This means that the array itself can be reallocated or reshuffled, while keeping the FCurves themselves untouched. This also means that references from Python remain valid, and that reorganising things is a bit more efficient.

makes sense, thanks for clarifying

makes sense, thanks for clarifying
dr.sybren marked this conversation as resolved
Christoph Lendenfeld reviewed 2024-03-05 13:10:37 +01:00
@ -0,0 +205,4 @@
/*main_listbase_index*/ INDEX_ID_AN,
/*struct_size*/ sizeof(Animation),
/*name*/ "Animation",
/*name_plural*/ "animations",

in all other occurrences of IDTypeInfo that I checked this was written with the N_ macro, e.g. N_("animations")
If I read the macro right this is for translation purposes. Not sure if this is still required.

For context: I found this because I was looking for the reason why in the outliner the category is named "animations", lowercase, instead of "Animations" which would be consistent with the other categories. Adding N_ does NOT fix that though

in all other occurrences of `IDTypeInfo` that I checked this was written with the `N_` macro, e.g. `N_("animations")` If I read the macro right this is for translation purposes. Not sure if this is still required. For context: I found this because I was looking for the reason why in the outliner the category is named "animations", lowercase, instead of "Animations" which would be consistent with the other categories. Adding `N_` does NOT fix that though
Author
Member

Good catch. I copied the Action IDTypeinfo, and that didn't have the N_() either. I'll add it to Animation.

Good catch. I copied the `Action` IDTypeinfo, and that didn't have the `N_()` either. I'll add it to `Animation`.
dr.sybren marked this conversation as resolved
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey reviewed 2024-03-05 15:36:38 +01:00
@ -0,0 +59,4 @@
/**
* Duplicate all Animation data from 'other' into `this`.
*
* Only used by the ID datablock management, other code should use regular ID
Member

Only used by the ID datablock management

Does it need to be exposed in the class then? That combination seems a bit misleading. Just putting the code in animation_copy_data is fine maybe.

> Only used by the ID datablock management Does it need to be exposed in the class then? That combination seems a bit misleading. Just putting the code in `animation_copy_data` is fine maybe.
dr.sybren marked this conversation as resolved
@ -0,0 +119,4 @@
*/
Type type() const
{
return static_cast<Type>(this->strip_type);
Member

Functional style cast is preferred for enum types as well.

Functional style cast is preferred for enum types as well.
dr.sybren marked this conversation as resolved
@ -0,0 +204,4 @@
template<> bool Strip::is<KeyframeStrip>() const
{
return type() == Type::Keyframe;
Member

type() -> this->type()

`type()` -> `this->type()`
dr.sybren marked this conversation as resolved
@ -0,0 +7,4 @@
*/
#include "BLI_listbase.h"
#include "BLI_listbase_wrapper.hh"
Member

Unused includes here I think

Unused includes here I think
dr.sybren marked this conversation as resolved
@ -0,0 +27,4 @@
#include "BLT_translation.hh"
using namespace blender;
Member

Instead of using namespace blender, you could put all the code except for BKE_animation_add and IDType_ID_AN in the blender::bke namespace.

Instead of `using namespace blender`, you could put all the code except for `BKE_animation_add` and `IDType_ID_AN` in the `blender::bke` namespace.
dr.sybren marked this conversation as resolved
@ -0,0 +47,4 @@
/** Free (or release) any data used by this animation (does not free the animation itself). */
static void animation_free_data(ID *id)
{
((Animation *)id)->wrap().free_data();
Member

Use C++ reinterpret_cast

Use C++ `reinterpret_cast`
dr.sybren marked this conversation as resolved
Author
Member

Thanks @HooglyBoogly !

Thanks @HooglyBoogly !
Sybren A. Stüvel force-pushed pr/baklava-data-model-1-dna from 06e5bfaa67 to db5d47ce1b 2024-03-05 17:17:02 +01:00 Compare
Bastien Montagne requested changes 2024-03-07 12:54:50 +01:00
Dismissed
Bastien Montagne left a comment
Owner

Generally looks good from a technical PoV, besides a few comments and explanations requested below.

Am slightly worried about the complexity involved by so many indirection levels in the new data model (Animation -> Layer -> Strip -> ChannelBag -> FCurve, with the Output mapping to IDs using the Animation). But I guess this is the cost of designing a flexible & powerful system.

Also, I'd rather see the 'Output' name topic sorted out before this lands in main, don't really see the point of landing something when there is a known incoming massive renaming just around the corner?

Generally looks good from a technical PoV, besides a few comments and explanations requested below. Am slightly worried about the complexity involved by so many indirection levels in the new data model (Animation -> Layer -> Strip -> ChannelBag -> FCurve, with the Output mapping to IDs using the Animation). But I guess this is the cost of designing a flexible & powerful system. Also, I'd rather see the 'Output' name topic sorted out before this lands in main, don't really see the point of landing something when there is a known incoming massive renaming just around the corner?
@ -0,0 +30,4 @@
namespace blender::bke {
/** Deep copy an Animation data-block. */

I'd rather not use the term 'deep copy' for this callback.

In ID management area, 'deep copy' is for when you duplicate an ID and all of its ID dependencies.

'Copy' is when you only duplicate the ID, while keeping it linked to the same ID dependencies as the source.

Shallow copy is when you only copy the ID struct itself, but none of its owned/managed sub-data (e.g. what we do when writing a blendfile).

I'd rather not use the term 'deep copy' for this callback. In ID management area, 'deep copy' is for when you duplicate an ID _and all of its ID dependencies_. 'Copy' is when you only duplicate the ID, while keeping it linked to the same ID dependencies as the source. `Shallow copy` is when you only copy the ID struct itself, but none of its owned/managed sub-data (e.g. what we do when writing a blendfile).
dr.sybren marked this conversation as resolved
@ -0,0 +31,4 @@
namespace blender::bke {
/** Deep copy an Animation data-block. */
static void animation_copy_data(Main * /*bmain*/, ID *id_dst, const ID *id_src, const int /*flag*/)

This actually will have to be updated, a681f5d896 from yesterday changed the signature of these callbacks.

This actually will have to be updated, a681f5d896 from yesterday changed the signature of these callbacks.
Author
Member

Thanks for the warning. I'll address that when landing the PR. If I were to rebase now, the 'show changes' button (on a force-push) would include all the changes on main as well, making it very hard to follow.

Thanks for the warning. I'll address that when landing the PR. If I were to rebase now, the 'show changes' button (on a force-push) would include all the changes on `main` as well, making it very hard to follow.
dr.sybren marked this conversation as resolved
@ -1149,0 +1178,4 @@
* another Animation data-block.
*
* \see AnimationOutput::name */
char output_name[66];

Would add a comment about why 66 and not 64 here (i.e. MAX_ID_NAME instead of MAX_NAME).

Does it actually have to be same as AnimationOutput::name?

Would add a comment about why `66` and not `64` here (i.e. `MAX_ID_NAME` instead of `MAX_NAME`). Does it actually have to be same as `AnimationOutput::name`?
Author
Member

Yes, this has to be the same as AnimationOutput::name. This is enforced in the static assert at the bottom of the file:

static_assert(std::is_same_v<decltype(AnimationBinding::name), decltype(AnimData::binding_name)>);
Yes, this has to be the same as `AnimationOutput::name`. This is enforced in the static assert at the bottom of the file: ```cpp static_assert(std::is_same_v<decltype(AnimationBinding::name), decltype(AnimData::binding_name)>); ```
dr.sybren marked this conversation as resolved
@ -1211,0 +1282,4 @@
* \see blender::animrig::Layer
*/
typedef struct AnimationLayer {
/** User-Visible identifier, unique within the Animation. `MAX_ID_NAME - 2`. */

Just reference MAX_NAME instead of MAX_ID_NAME ?

Just reference `MAX_NAME` instead of `MAX_ID_NAME` ?
dr.sybren marked this conversation as resolved
Author
Member

Also, I'd rather see the 'Output' name topic sorted out before this lands in main, don't really see the point of landing something when there is a known incoming massive renaming just around the corner?

👍 I'll do that now that you're done with this review :)

> Also, I'd rather see the 'Output' name topic sorted out before this lands in main, don't really see the point of landing something when there is a known incoming massive renaming just around the corner? :+1: I'll do that now that you're done with this review :)
Sybren A. Stüvel force-pushed pr/baklava-data-model-1-dna from db5d47ce1b to 93bc775822 2024-03-07 15:00:11 +01:00 Compare
Sybren A. Stüvel requested review from Bastien Montagne 2024-03-07 15:00:50 +01:00
Bastien Montagne approved these changes 2024-03-07 15:06:59 +01:00
Bastien Montagne left a comment
Owner

Thanks, think it's good to go then now.

Thanks, think it's good to go then now.
Sybren A. Stüvel force-pushed pr/baklava-data-model-1-dna from 93bc775822 to afe62f6e9d 2024-03-07 16:01:23 +01:00 Compare
Author
Member

My force-push made things unclear, as I changed things and rebased in the same force-push. So I'm going to redo that, first force-pushing back to the state that was reviewed, then doing the rebase to main, and then the last changes necessary to make the rebase actually work.

My force-push made things unclear, as I changed things *and* rebased in the same force-push. So I'm going to redo that, first force-pushing back to the state that was reviewed, then doing the rebase to `main`, and then the last changes necessary to make the rebase actually work.
Sybren A. Stüvel force-pushed pr/baklava-data-model-1-dna from afe62f6e9d to 93bc775822 2024-03-07 16:06:22 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model-1-dna from 93bc775822 to 397a4b25d8 2024-03-07 16:06:58 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model-1-dna from 397a4b25d8 to 8f32ce169a 2024-03-07 16:09:48 +01:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel merged commit 90ef46baa1 into main 2024-03-07 16:41:34 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#119077
No description provided.