Anim: DNA for Animation
data-block #119077
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119077
Loading…
Reference in New Issue
No description provided.
Delete Branch "dr.sybren/blender:pr/baklava-data-model-1-dna"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 nameOutput
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 toBinding
.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.
@ -0,0 +1,291 @@
/* SPDX-FileCopyrightText: 2023 Blender Developers
I assume the year should be 2024
@blender-bot build
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
orStrip
Animation
: it is anID
, and so Blender has a bunch of other logic to duplicate those already.Strip
: because of the whacky nature of the inheritance:KeyframeAnimationStrip
"inherits"AnimationStrip" by embedding the latter. This means that any
KeyframeAnimationStrip*can be reinterpreted as
AnimationStrip*`.animrig::Strip
inherits::AnimationStrip
, andanimrig::KeyframeStrip
inherits::KeyframeAnimationStrip
.Because of this,
MEM_new<Strip>(__func__, someKeyframeAnimationStrip)
is going to allocate just enough space fo aStrip
, and then you have something that the rest of Blender simply won't expect. It'll crash once you set that strip'sstrip_type
property toStrip::Type::Keyframe
as it simply won't have enough memory allocated to hold aKeyframeStrip
.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 :)
@ -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 currentlyI'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.
@ -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
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 theChannelBag
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 forI 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.@ -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.
Good point, I think this can be changed to an array of FCurves.
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
@ -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 theN_
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 thoughGood catch. I copied the
Action
IDTypeinfo, and that didn't have theN_()
either. I'll add it toAnimation
.@blender-bot build
@ -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
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.@ -0,0 +119,4 @@
*/
Type type() const
{
return static_cast<Type>(this->strip_type);
Functional style cast is preferred for enum types as well.
@ -0,0 +204,4 @@
template<> bool Strip::is<KeyframeStrip>() const
{
return type() == Type::Keyframe;
type()
->this->type()
@ -0,0 +7,4 @@
*/
#include "BLI_listbase.h"
#include "BLI_listbase_wrapper.hh"
Unused includes here I think
@ -0,0 +27,4 @@
#include "BLT_translation.hh"
using namespace blender;
Instead of
using namespace blender
, you could put all the code except forBKE_animation_add
andIDType_ID_AN
in theblender::bke
namespace.@ -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();
Use C++
reinterpret_cast
Thanks @HooglyBoogly !
06e5bfaa67
todb5d47ce1b
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).@ -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.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.@ -1149,0 +1178,4 @@
* another Animation data-block.
*
* \see AnimationOutput::name */
char output_name[66];
Would add a comment about why
66
and not64
here (i.e.MAX_ID_NAME
instead ofMAX_NAME
).Does it actually have to be same as
AnimationOutput::name
?Yes, this has to be the same as
AnimationOutput::name
. This is enforced in the static assert at the bottom of the file:@ -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 ofMAX_ID_NAME
?👍 I'll do that now that you're done with this review :)
db5d47ce1b
to93bc775822
Thanks, think it's good to go then now.
93bc775822
toafe62f6e9d
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.afe62f6e9d
to93bc775822
93bc775822
to397a4b25d8
397a4b25d8
to8f32ce169a
@blender-bot build