Baklava: minimal data model #118677
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#118677
Loading…
Reference in New Issue
No description provided.
Delete Branch "dr.sybren/blender:pr/baklava-data-model"
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 PR contains all the commits that make up the current state of Project Baklava. A separate PRs was made for the first commit (#119077: Anim: DNA for Animation data-block). The others are still in this PR, and will be landed all at once. Reviewers are invited to use the 'Commits' view of the 'Files Changed' tab to review on a per-commit basis.
This PR introduces a new
Animation
data-block that is meant for multi-ID, layered animation.Design docs are collected in #113594.
Here's a screenshot of what the UI looks like now. This is very much a developer-centric UI, and not intended to be anything like the final UI for animators.
Wow! Congrats on getting this far :) I only skimmed through the code and will go back to read things more carefully, but I for the code I saw thus far, things seemed pretty clear. Nice work!
@ -0,0 +2,4 @@
#
# SPDX-License-Identifier: GPL-2.0-or-later
"""NOTE: this is temporary UI code to show animation layers.
Might be worth it to name the file
temp_anim_layers.py
.@ -0,0 +27,4 @@
struct Main;
struct PointerRNA;
namespace blender::animrig {
Maybe just
blender::animation
might be more appropriate? I see the folder name is alsoanimrig
. Does that still seem right?Might be worth splitting up at some point. For now we're more focused on reducing the jumble of locations in which the code is spread to a single module. Just to name one example: deleting a keyframe was in
blenkernel
, creating one was in one of the editors.Once we move forward with this we can always revisit.
@ -0,0 +58,4 @@
/* Copied from source/blender/blenkernel/intern/grease_pencil.cc. It also has a shrink_array()
* function, if we ever need one (we will). */
template<typename T> static void grow_array(T **array, int *num, const int add_num)
@JacquesLucke recently added
blender/source/blender/makesdna/DNA_array_utils.hh
. Might be worth to note that this is probably where this function (and the grease pencil one) should end up.@ -177,0 +213,4 @@
}
if (strip->contains_frame(frame_time)) {
/* Found it! */
While I appreciate the humor, it's probably best to leave out comments that don't add value :D
@ -0,0 +206,4 @@
return {};
}
static float lerp(const float t, const float a, const float b)
Looks like this is just
math::interpolate<float>
Or even more, this is just
bke::attribute_math::mix2<float>
!)In case of thinking about this as about attributes.
math::interpolate<float>
doesa * (1 - t) + b * t
our
lerp
doesa + t * (b - a)
These are subtly different when it comes to round-off errors. IIRC the first case guarantees that the endpoints are met at
t=0
andt=1
, but are not monotonic (i.e. can actually go backwards in value whent
increases). The latter only guaranteesa
is returned witht=0
, but not thatb
is returned fort=1
, but it is guaranteed to be monotonic. And then there isstd::lerp
, which apparently has all these nice properties but has branches in the code, which we don't want either.For now it likely won't matter in practice, and I'll change the code to use
math::interpolate()
.@ -0,0 +153,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();
Better to use C++ casts here
Reading over this, it looks pretty nice. I left a range of comments. Some are probably nitpicky. Or maybe some are subjective-- I didn't hold back too much.
Just two general comments:
Animation
though, since the topic of constructors and destructors forID
data-blocks brings up complexity and design decisions that we probably don't want to get into now.@ -0,0 +10,4 @@
#pragma once
#ifndef __cplusplus
# error This is a C++ header.
We hardly have any C files anymore, not sure it's worth including this
@ -0,0 +35,4 @@
class Output;
/* Use an alias for the stable index type. */
using output_index_t = decltype(::AnimationOutput::stable_index);
Hmm, I'd maybe choose a name more like
OutputIndex
. I think the "_t" is meant to be reserved for lower level types (or I guess if the point is to add some more semantics to the type, it might as well "look like it")It's indeed the latter, instead of having
int32_t
andint64_t
, we wanted to have something that is specific for the output "stable index". Especially since we were at some point waivering back and forth between 64- and 32-bit integers. Since others (likesize_t
) also are written like this, this seemed appropriate. It also, for me, makes it a bit clearer it's a number, and not some arbitrary class.@ -0,0 +57,4 @@
* After this call, the passed reference is no longer valid, as the memory
* will have been freed. Any strips on the layer will be freed too.
*
* \return true when the layer was found & removed, false if it wasn't found. */
*/
is meant to be on a separate line at the end for this comment style, for multi-line doc-strings@ -0,0 +100,4 @@
* later. In that case, the ID will not actually receive any animation.
* \param animated_id The ID that should be animated by this Animation data-block.
*/
bool assign_id(Output *output, ID *animated_id);
If the ID can't be null, it could be a reference. That might apply in other places too
I like the clarity that it gives to the function itself, and its intent. It does have the downside that at the call site you don't see whether it's passed by (mutable) reference or by value, though. If this is a Blender wide preference, to prefer using references instead of should-not-be-null-pointers, I'd be happy to switch over, but then it might be good to add this to the C++ style guide as well.
In any case, I personally slightly prefer the references over 'nonnullable-pointers', so I'll just change this regardless.
@ -0,0 +125,4 @@
int64_t find_layer_index(const Layer &layer) const;
private:
Output &output_allocate_();
Typically methods don't get the
_
suffix@ -0,0 +130,4 @@
static_assert(sizeof(Animation) == sizeof(::Animation),
"DNA struct and its C++ wrapper must have the same size");
class Layer : public ::AnimationLayer {
As someone who hasn't internalized the design of the new animation system fully yet, it would be helpful to see a brief description of what each of these classes is for. Just a sentence or two would be really helpful.
@ -0,0 +207,4 @@
bool is_last_frame(float frame_time) const;
/**
* Set the start and end frame.
frame
->frames
I think singular is better, as it sets the start frame and the end frame. There are no start frames, nor are there end frames. I think English allows for both.
@ -0,0 +240,4 @@
* \return nullptr if there is none yet for this output.
*/
const ChannelsForOutput *chans_for_out(const Output &out) const;
ChannelsForOutput *chans_for_out(const Output &out);
Unless you think the brevity is very important, it might be nice to use the full "channels" word consistently. Feels a wee bit like something from 2 decades ago when characters were limited ;)
One thing we were struggling with a bit is the naming.
ChannelsForOutput
as a class name makes sense, as it literally contains the animation channels for a given output. An "actually correct" function name here would bechannels_for_output_for_output
, as it returns theChannelsForOutput
for the given output.Maybe
ChannelBag
would be better term for the class, and then the function could just be namedchannelbag_for_output
or something like that.Writing this down, I think I actually like the idea, because it resolves the nasty "it looks plural but is a singluar object" aspect of the name
ChannelsForOutput
.@ -34,0 +34,4 @@
/**
* Create an fcurve for a specific channel, pre-set-up with default flags and interpolation mode.
*/
FCurve *create_fcurve_for_channel(const char rna_path[], const int array_index);
const int
->int
The const is meaningless in the declaration (rather than the definition)
@ -0,0 +58,4 @@
/* Copied from source/blender/blenkernel/intern/grease_pencil.cc. It also has a shrink_array()
* function, if we ever need one (we will). */
template<typename T> static void grow_array(T **array, int *num, const int add_num)
Jacques recently added this abstraction to DNA. It would be nice to use
DNA_array_utils.hh
directly here. The idea is to move more code to those functions soon, so we can extend them if necessary tooWe added a note to the comment, reminding us of this.
@ -0,0 +279,4 @@
return const_cast<Output *>(out);
}
const Output *Animation::output_for_id(const ID *animated_id) const
const ID *
->const ID &
@ -0,0 +280,4 @@
}
const Output *Animation::output_for_id(const ID *animated_id) const
Extra whitespace
@ -0,0 +421,4 @@
adt->animation = nullptr;
}
/* ----- AnimationLayer C++ implementation ----------- */
The whole thing is implemented in C++, I'd just remove "C++" from the comment
@ -0,0 +435,4 @@
}
const Strip *Layer::strip(const int64_t index) const
{
return &this->strip_array[index]->wrap();
Couldn't these return references?
@ -0,0 +467,4 @@
if (strip_index < this->strip_array_num - 1) {
::AnimationStrip **start = this->strip_array + strip_index;
const int64_t num_to_move = this->strip_array_num - strip_index - 1;
memmove((void *)start, (void *)(start + 1), num_to_move * sizeof(::AnimationStrip *));
The
DNA_array_utils.hh
code would hopefully be helpful here too@ -0,0 +583,4 @@
void Strip::free_data()
{
/* This could be a map lookup, but a `switch` will emit a compiler warning when a new strip type
Not sure a map lookup would ever be helpful here TBH. And also I don't think this will give a warning, since the switch isn't using the actual enum type. That's why I suggested adding an accessor method for the type in another comment.
@ -0,0 +590,4 @@
this->as<animrig::KeyframeStrip>().free_data();
return;
}
BLI_assert(!"unfreeable strip type!");
Just
BLI_assert_unreachable()
would be pretty self explanatory here@ -0,0 +701,4 @@
const int array_index)
{
FCurve *fcurve = this->fcurve_find(out, rna_path, array_index);
if (fcurve) {
If you do this:
then you can declare the variable where its value is assigned below :)
@ -177,0 +218,4 @@
}
/* See if this at least a better match for any previously-found FCurve. */
const float this_distance = min_ff(fabs(frame_time - strip->frame_start),
min_ff
->std::min
fabs
->std::abs
@ -0,0 +184,4 @@
}
}
/* Returns whether the strip was evaluated. */
Outdated comment?
@ -0,0 +186,4 @@
/* Returns whether the strip was evaluated. */
static EvaluationResult evaluate_strip(PointerRNA *animated_id_ptr,
Strip &strip,
Maybe you're limited by the old code here, but ideally
strip
would be const here-- ideally you wouldn't need to change the strip to evaluate it.Yup,
calculate_fcurve()
insource/blender/blenkernel/intern/fcurve.cc
writes tofcu->curval
. Would be nice to get rid of that at some point, but that's out of scope for this PR.@ -0,0 +261,4 @@
namespace internal {
EvaluationResult evaluate_layer(PointerRNA *animated_id_ptr,
This can be a reference
@ -0,0 +279,4 @@
continue;
}
const auto strip_result = evaluate_strip(
Not sure this is written in the style guide yet, but usually
auto
isn't used in a case like this. As a reader if would be helpful for me to see the type name so I don't have to go look atevaluate_strip
to find what it is.@ -0,0 +50,4 @@
: value(value), prop_rna(prop_rna)
{
}
~AnimatedProperty() = default;
Not sure there's any need to write the defaulted destructor explicitly like this?
I'm not sure either, but it's done in other places as well (for example
~Set() = default;
).Generally I'd recommend not doing it, but here are some exceptions:
.cc
instead of in the.hh
file.Set(const Set &other) = default;
would result in compile errors because the compiler would think thatSet
is not copyable because there is an explicit move constructor.In your case here, it should not be necessary, because the copy/move-constructor, copy/move-assignment-operator and destructor are all defaulted.
@ -0,0 +77,4 @@
public:
operator bool() const
{
return !is_empty();
is_empty()
->this->is_empty()
@ -0,0 +83,4 @@
{
return result_.is_empty();
}
EvaluationResult &operator=(EvaluationResult other)
Was is necessary to write this? And the other RAII methods? I'd expect it to work fine not writing any of them for a simple class like this.
If I remove it, I get these errors:
I do want to have some move semantics, but maybe I'm doing it wrong? Open for suggestions.
Ah funny, that's actually exactly the problem I mentioned in my previous commit. In this case you should just remove all the special methods because they are all defaulted. By explicitly defining the move constructor you tell the compiler that something special is going on in this time, so it won't generate the other special methods automatically.
@ -32,6 +33,17 @@ KeyframeSettings get_keyframe_settings(const bool from_userprefs)
return settings;
}
FCurve *create_fcurve_for_channel(const char rna_path[], const int array_index)
const char rna_path[]
->const StringRef rna_path
BLI_strdup(rna_path)
->BLI_strdupn(rna_path.data(), rna_path.size());
@ -0,0 +41,4 @@
const AnimationChannelsForOutput *channels_src);
using anim_strip_duplicator = AnimationStrip *(*)(const AnimationStrip *strip_src);
using anim_strip_freeer = void (*)(AnimationStrip *strip);
anim_strip_freeer
is unused@ -0,0 +97,4 @@
static AnimationOutput *anim_output_duplicate(const AnimationOutput *output_src)
{
AnimationOutput *output_dup = static_cast<AnimationOutput *>(MEM_dupallocN(output_src));
Except for
Animation
(which has all the complexity of being anID
data-block), it feels like these should be implemented as copy constructors. For the calling code it should be as simple as usingMEM_new<AnimationOutput>(__func__, src);
. I think abstracting it more than that just means you have to dig deeper to understand what's going on.@ -0,0 +103,4 @@
static AnimationStrip *anim_strip_duplicate(const AnimationStrip *strip_src)
{
anim_strip_duplicator duplicator = get_strip_duplicator(eAnimationStrip_type(strip_src->type));
This indirection doesn't seem helpful to me. Seems much simpler to just switch based on the type here directly rather than deal with function pointers and make this unnecessarily extra object oriented. Am I missing something?
@ -1643,0 +1675,4 @@
Animation *dna_animation)
{
BLI_assert(id != nullptr);
BLI_assert(operation_from != nullptr);
Use references rather than asserting for null pointers
This is now done consistently with the rest of the
deg_builder_relations.cc
file. I think it's better to address that all at once, rather than this one function being the only one using references.@ -1211,0 +1260,4 @@
typedef struct Animation {
ID id;
struct AnimationLayer **layer_array; /* Array of 'layer_array_num' layers. */
How about this order?
@ -1211,0 +1290,4 @@
/**
* There is always at least one strip.
* If there is only one, it can be infinite. This is the default for new layers. */
Comment formatting
@ -1211,0 +1345,4 @@
typedef struct AnimationStrip {
/** eAnimationStrip_type */
uint8_t type;
I'd suggest wrapping this with a
type()
accessor inanimrig::Strip
that can return the proper enum type. Then this could be renamedstrip_type
or something more verbose like that so people prefer using the shorter accessor.@ -1211,0 +1350,4 @@
float frame_start;
float frame_end;
float frame_offset;
These three could use some comments. I was particularly curious why the offset is necessary when there are already a start and end stored
@ -1211,0 +1358,4 @@
#endif
} AnimationStrip;
typedef enum eAnimationStrip_type {
How about
AnimationStripType
?Last time there was significant discussion about it, was general agreement that people didn't like the "e" prefix.
Personally I'd also consider moving this to
enum class Type : int8_t { Keyframe=0; }
in theanimrig::Strip
wrapper. I don't think it helps so much to have the enum declared in DNA-- what's important is the storage type (which makes more sense signed BTW, usually unsigned is just used for flags).@ -0,0 +35,4 @@
"OFFSET",
0,
"Offset",
"Animation channels in this layer are added, as sequential operations, to the output of "
added, as
->added as
Maybe this is how it would be spoken, but grammatically it doesn't seem right. Same with the commas in the other dscriptions
AFAIK it's just the use of commas to separate main clause from sub-clause. It's gramatically correct, at least according to what I remember from the English Writing course I did while doing my PhD ;-)
Fair! My comment was lazy-- the feedback was more about how it's worded and making it natural to read. The separated sub-clause breaks the flow of the sentence, making it harder to understand. The tone feels more like a scientific paper than typical UI text. Obviously this isn't a big deal, I'm just thinking about consistency and friendliness of UI text overall.
Hah, I quite like the switchover from "how it would be spoken" to "feels more like a scientific paper" ;-)
If you have a proposal for another way to word things, I'm all ears.
Not sure if that's supposed to be a sarcastic gotcha or not... These sentences are hard to parse, it's hard to describe why. But there are too many clauses that are hard to piece together. One thing that doesn't help is the indirection in the concepts. For example, what does it mean that "animation channels ... are added to the output"? "Output" is a vague term for the user. "Animation" is also an extra word that's clear from the context.
I'd try "Channels in this layer are added sequentially to underlying layers".
No sarcasm here, just a bit of humor.
I understand your points, and I'll see what I can come up with.
@ -0,0 +192,4 @@
animrig::Animation &anim = dna_animation->wrap();
animrig::Layer &layer = dna_layer->wrap();
if (!anim.layer_remove(layer)) {
BKE_report(reports, RPT_ERROR, "this layer does not belong to this animation");
this
->This
Thanks for all the feedback, this is super useful!
🥳
👍
Here the 'stable' does reference the index itself: it does not index into the array, but it's a number that's guaranteed to not be reused (within the owning
Animation
). As such, it's a stable number for identifying anOutput
, regardless of how theseOutput
s are stored in the array, reshuffled, etc. Your remark makes it clear that this should be named and/or documented better.Maybe
output_id
oroutput_handle
would be better; just naming it plain "index" is too much "this indexes into the storage array" for me.We went for a bit more uniform approach, but I agree that it'll be better to just use the destructors for this wherever possible. Then again, even the outputs, layers, and strips are stored in DNA, and so can still be allocated through C allocations. What would happen when a C++
Layer
wrapper goes out of scope? That shouldn't call the destructor (because we should only be using pointers and references), but still it feels a bit iffy to me.Great, I wanted to suggest
output_id
as a name too. The stability is implied by its usage requirements and the word "id" which has that meaning across Blender, it's used in a fair amount of similar cases currently.That should be fine, since allocation and initialization can be separate using placement new. That should only be necessary for file reading though. Otherwise to copy the a value into a new allocation you can do
MEM_new<T>(__func__, src);
I think using the standard RAII semantics for the wrapper types will make things simpler in the end.I mean whether it's just a pointer/reference or a value is a pretty essential difference. Being able to predict the behavior based on common C++ value semantics seems nice.
Maybe we can also consider using
persistent_uid
for the output ids. Brecht suggested that we might want to use this name in more places: #117347 (comment).Nathan and I discussed it a bit more, and feel that
output_handle
is the better choice here.id
is too close toID
, and we feel that this could lead to confusion.That wouldn't apply here, at least not when using the definition of 'Unique Identifier' from Wikipedia, where a UID is expected to be unique within the group of objects identified by it. The Output ids/handles are not unique within the entire blend file, but are scoped to the owning
Animation
.Wikipedia's definition of 'handle' is also not a perfect match, unfortunately, as it notes that it's about identifying data that is managed by an external system. The following property (also described on that page) is something I quite like, though:
This is exactly what we're using the thing for: identifying something without having to keep a pointer to it. This makes it trivial to deal with CoE copies and the like. And we avoid having a pointer between IDs that points to sub-data of the ID (instead of the ID itself).
More importantly, there is no expectation that the number is unique, but it's rather just an abstract numerical value that you get from something & use to access things. I think it fits quite well.
Thanks for all the review comments. I'm going to force-push to this branch soon, with all the notes addressed. The force-push is a necessity to keep the commits as I'd like to land them. If you want to see the original 7 commits + all the feedback commits, check my pr/baklava-data-model-partial-pr-feedback branch.
0c54f503ec
to1a823d08d5
Force-push is done. Note that the PR description has a new blend file, as I also changed the structure of the DNA slightly. The ugly "create a temporary
ListBase
for the FCurves just to save to disk" is gone, and they are now properly written as pointer array.BTW @HooglyBoogly I re-requested review, but do know that I'm going to split out this PR into a single PR per commit. That way it'll be easier to gradually review / alter / accept.
1a823d08d5
toad85a78c45
ad85a78c45
to836ca1d5a3
836ca1d5a3
toefd90da5b7
Added some comments.
@ -0,0 +70,4 @@
col.prop(layer, "influence")
col.prop(layer, "mix_mode")
# for strip_idx, strip in enumerate(layer.strips):
Is this code still needed? Otherwise would be better to just remove it.
@ -0,0 +354,4 @@
* Should only be called when there is no `ChannelBag` for this output yet.
*/
ChannelBag &channelbag_for_output_add(const Output &out);
/**
I suppose this could return
FCurve &
since it should always return an f-curve.Meta remark: Hmm this comment got dislodged a bit. It's now showing underneath this line:
but I'm guessing it's about
@ -177,0 +226,4 @@
found_on_other_strip = fcu;
found_at_time_distance = this_distance;
}
} break;
Not sure if this is in the style-guide, but I found that most code puts the
break;
inside the code block if there is one for thecase
.yeah it is, this is a mistake.
efd90da5b7
to968e9b6f43
968e9b6f43
to3833509cea
3833509cea
to1a135a904e
1a135a904e
tod43418fe6d
d43418fe6d
to5f7a715de5
What was once the first commit of this PR was reviewed in #119077. Now that that has landed in
main
(90ef46baa1
), I've rebased this PR on top of it (hence the force-push above, which reduced the number commits in this PR from 7 to 6). The remaining commits can be reviewed here.5f7a715de5
tod0cb8e2a20
The next force-push simplifies the last commit in the stack:
Anim, show animated properties with the appropriate color in the GUI
. It removes theframe_time
parameter that was added to various functions. It was necessary to check for animation strip boundaries, but since those arent' going to be supported, I documented the assumption that there's only going to be one, and simplified the rest of the code. Now this assumption just lives in one place, and is well documented.d0cb8e2a20
to319e132a97
Next force-push will fix a compiler warning & fix something that got introduced when I rebased on
main
earlier. No functional changes.319e132a97
to2f123b6b62
Added some comments.
@ -160,1 +581,4 @@
bool Strip::contains_frame(const float frame_time) const
{
return frame_start <= frame_time && frame_time <= frame_end;
Use
this->
for non-private members. https://developer.blender.org/docs/handbook/guidelines/c_cpp/#using-this-@ -0,0 +72,4 @@
~EvaluationResult() = default;
public:
operator bool() const
I'd like to hear others opinion about "syntactic sugar" like this vs. just using
is_empty()
.The reason I did it like this was in a change meant to avoid the need for
std::optional<EvaluationResult>
, i.e. to abolish the difference between 'no result' and 'empty result'.Also I like the positivity here, where
if (result)
means it's usable, rather than the negation necessary withif (!result.is_empty())
. Of course that can be avoided withhas_data()
or something like that, but I quite like the boolean operator for container-like data structures.In the discussion in Blender Chat there was no major objection to using
operator bool()
, so for now I'll keep it in.@ -0,0 +120,4 @@
const int array_index,
const float eval_time)
{
const std::optional<float> eval_value = evaluate_single_property(
I suppose this could be called directly in the
if
statement since the variable is not needed otherwise.You mean like this?
I don't think I find that easier to read. Or maybe I'm misunderstanding something?
Yes like that. For
std::optional
s, we use this pattern quite a lot already. Makes the scope of the varaible a tiny bit smaller :)This is just a short function, so it doesn't matter, really. Either is fine.
2f123b6b62
to974deb2c6c
974deb2c6c
to18e41d1a1a
The next force-push avoids some
std::string
copies when evaluating animation data.It also removes the
Strip()
default constructor; the destructor assumes thatStrip::type()
returns a valid strip type; constructing a Strip with the default constructor voids that assumption. This can be worked around by adding another enum value toStrip::Type
, for exampleUnset
orInvalid
, but that would force allswitch(strip.type()))
to handle that case as well.Right now the only use of the default
Strip
constructor is in a unit test, and that can easily be rewritten to properly allocate a keyframe strip18e41d1a1a
to8239c859f4
@blender-bot build
8239c859f4
to145e22fa99
145e22fa99
toa722bd919b
a722bd919b
to5c0ca4c4f6
5c0ca4c4f6
to15b1710210
15b1710210
to87bdcb31ff
@blender-bot build
Just a few cleanup comments this time. I don't have any larger comments.
@ -71,0 +112,4 @@
/** Assign this animation to the ID.
*
* \param binding The binding this ID should be animated by, may be nullptr if it is to be
* assigned later. In that case, the ID will not actually receive any animation. \param
Newline before
\param
@ -175,0 +273,4 @@
* After this call, the passed reference is no longer valid, as the memory
* will have been freed.
*
* \return true when the strip was found & removed, false if it wasn't found. */
*/
should be on the next line@ -76,0 +327,4 @@
}
}
/* Try the binding name from the AnimData, if it is set,*/
,
->.
@ -26,6 +27,9 @@
#include "RNA_access.hh"
#include "RNA_path.hh"
#include <algorithm>
Not sure what these new includes are for?
@ -0,0 +113,4 @@
/**
* Evaluate the animation data on the given layer, for the given binding. This
* just returns the evaluation result, without taking any other layers,
* blending, influence, etc. into account. */
*/
on newline@ -224,0 +250,4 @@
BLI_assert_msg(adt, "ID.animation_data is unexpectedly empty");
if (!adt) {
WM_reportf(RPT_ERROR,
"Data-block '%s' does not have any animation data, how did you set this property?",
I realize this is probably rarely seen, but ending the error message with a question seems pretty non-standard. Maybe there's another way to make the point
@ -224,0 +263,4 @@
blender::animrig::Animation *anim = blender::animrig::get_animation(animated_id);
if (!anim) {
/* No animation to verify the stable index is valid. Just set it, it'll be ignored anyway. */
stable index
->handle
@ -0,0 +213,4 @@
animrig::Binding &binding_to_find = rna_data_binding(ptr);
Span<animrig::Binding *> bindings = anim.bindings();
for (int i = 0; i < bindings.size(); ++i) {
bindings.first_index_try
can simplify thisNow that I look at this again, I'll replace it by using the binding name, instead of the index. Those are unique, and make things like library overrides easier, later.
@ -0,0 +318,4 @@
for (animrig::Layer *layer : anim.layers()) {
Span<animrig::Strip *> strips = layer->strips();
for (int i = 0; i < strips.size(); ++i) {
same here
@ -0,0 +357,4 @@
const float time)
{
if (dna_binding == nullptr) {
BKE_report(reports, RPT_ERROR, "binding cannot be None");
binding
->Binding
@ -0,0 +534,4 @@
parm = RNA_def_enum(func,
"type",
rna_enum_strip_type_items,
(int)animrig::Strip::Type::Keyframe,
(int)animrig::Strip::Type::Keyframe
->int(animrig::Strip::Type::Keyframe)
@ -7221,0 +7222,4 @@
prop = RNA_def_property(srna, "use_animation_baklava", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "use_animation_baklava", 1);
RNA_def_property_ui_text(
prop, "Animation: Project Baklava", "Enable the new multi-id, layered animation system");
We're trying not to use "ID" in the UI, how about multi-data-block?
@ -235,2 +399,4 @@
const FCurve *fcurve(int64_t index) const;
FCurve *fcurve(int64_t index);
const FCurve *fcurve_find(const StringRefNull rna_path, const int array_index) const;
Remove const in declaration
@ -0,0 +13,4 @@
class PropIdentifier {
public:
StringRefNull rna_path;
It's probably worth having a comment about where these strings are owned
87bdcb31ff
tod511f85db6
Thanks for the review @HooglyBoogly !
The next force-push will have your feedback incorporated.
d511f85db6
tocaf1379e82
@blender-bot build
Looked through this once more. Don't have any comments. Accepting.
caf1379e82
to050cf201ac
Only reviewed the first two commits. Noted a lot of small details below, and a few not-so-small ones.
Did not check the depsgraph part of 6dc5129056 btw.
@ -71,0 +91,4 @@
*
* \see Animation::binding_name_propagate
*/
void binding_name_set(Binding &binding, StringRefNull new_name);
API-wise, I find it weird to only expose a two-steps process, for a single-case technical reason (RNA limitations), when it is known that there should be virtually no cases where you don't want to call both together.
I would rather see a single, most-common usage method that does everything at once:
void binding_name_set(Main &bmain, Binding &binding, StringRefNull new_name);
And then two lower-level methods, explicitly documented as such. These would be used directly when the 'all in one' cannot be called (technical limitation of RNA), or also when performances can become an issue. Indeed, if a large number of bindings need to be renamed, it could make sense to only change their names first, and then to process all IDs from Main at once. Something like that maybe:
Then later on, if needed,
binding_name_propagate
could be optimized for cases where it gets lots of bindings to process at once (e.g. by building a map between the handles and their bindings).@ -71,0 +116,4 @@
* assigned later. In that case, the ID will not actually receive any animation.
* \param animated_id The ID that should be animated by this Animation data-block.
*/
bool assign_id(Binding *binding, ID &animated_id);
Could use an
std::optional
to a Binding reference instead of relying onnullptr
value?What would semantically be the difference?
I'm not sure I understand the question... semantically, both approaches are the same.
But using
nullptr
is a 'C way' of doing it, which needs to be documented explicitly, and can be dangerous (it's easy when editing existing code to forget when a pointer may or may not be null).On the other hand,
std::optional
is designed for such usages, and is a clearer and safer alternative in modern C++. Just by using it, you declare that this parameter may or may not be given. By using a reference as type of managed data, you can be certain to always get valid data if the optional is set.std::optional<T &>
does not exist in C++ currently.https://en.cppreference.com/w/cpp/utility/optional
It's being discussed for C++26, but anyway..
A pointer is fine here, semantically it just means it must be checked for null, which is exactly what we want.
Ah... my bad, indeed... Then agree that raw pointer is fine here.
Thanks for confirming my suspicion, Hans.
This is why I'm trying to use as many references as possible, so basically if you see a pointer it's there because it can be null.
@ -71,0 +129,4 @@
* - `animated_id.adt->binding_name`,
* - `animated_id.name`.
*
* Note that this different from #binding_for_id, which does not use the
Note that this is different
@ -216,0 +371,4 @@
* Find an FCurve for this binding + RNA path + array index combination.
*
* If it cannot be found, a new one is created.
*/
matching against the binding
I think you're suggesting this:
I don't find that significantly clearer, though.
Force-pushing during a review is a bad idea, it breaks completely comments locations. Another reason to only review one commit per PR...
This comment was about a typo (missing
t
inagainst
), search foragains the binding
IIRC@ -216,0 +373,4 @@
* If it cannot be found, a new one is created.
*/
FCurve &fcurve_find_or_create(const Binding &binding, StringRefNull rna_path, int array_index);
creating a binding
Not sure what you're suggesting here. I think Gitea lost the context of where this note had to be.
Looks like the last force push caused all comments to be outdated and in the wrong place. Ouch :/
indeed, after the rename from output to binding, there was a lot of
an binding
left in comments, this was one of these I noticed,@ -36,0 +41,4 @@
}
static animrig::Strip &animationstrip_alloc_infinite(const Strip::Type type)
{
AnimationStrip *strip;
I believe that this should be initialized explicitly? AFAIK C++ and C have same behavior here, otherwise this has undefined value, and won't be initialized to anything when reaching the
BLI_assert_message
below in case an unknown type is requested.Good find. I'm getting too used to Go's "initialise to the zero value" behaviour ;-)
@ -36,0 +57,4 @@
return strip->wrap();
}
/* Copied from source/blender/blenkernel/intern/grease_pencil.cc. It also has a shrink_array()
The part of the comment about
shrink_array
seems outdated, since it's defined below?@ -36,0 +68,4 @@
MEM_cnew_array<T *>(new_array_num, "animrig::animation/grow_array"));
blender::uninitialized_relocate_n(*array, *num, new_array);
if (*array != nullptr) {
MEM_SAFE_FREE(*array);
?@ -76,0 +298,4 @@
{
Binding &binding = MEM_new<AnimationBinding>(__func__)->wrap();
this->last_binding_handle++;
BLI_assert_msg(this->last_binding_handle > 0, "Animation Binding handle 32-bit overflow");
Technically, since this is a signed int32, it's a 31-bit overflow ;)
Also, usage of 'magic number'
0
instead of proper 'unset'' defineGood catch.
This is not a magic number
0
though, this is actually "after incrementing this number should still be positive".And I'll remove the "32-bit" from the sentence, the bit depth doesn't matter, the overflow is the relevant part here.
@ -93,0 +387,4 @@
STRNCPY_UTF8(adt->binding_name, binding->name);
}
else {
adt->binding_handle = 0;
Eeeeeeeeek! Magic Number!
I think that an
UNSET_BINDING_HANDLE
define or constexpr is required inDNA_anim_types.h
!Found two places where this magic
0
value is used, but fairly sure there are more to uncover.Although I get your point (and will address it), I don't agree this should be in DNA. I'll put it as a
constexpr static
member in theanimrig::Binding
class.@ -93,0 +403,4 @@
void Animation::unassign_id(ID &animated_id)
{
AnimData *adt = BKE_animdata_from_id(&animated_id);
BLI_assert_msg(adt->animation == this, "ID is not assigned to this Animation");
Should check and early-return in case the
adt
isnullptr
.It shouldn't, because calling
anim.unassign_id(id_not_animated_by_anim)
is a bug. I've added an extraBLI_assert_msg(adt, ...)
though.And added this restriction to the documentation of the function.
@ -93,0 +409,4 @@
* If Blender would be bug-free, and we could assume that `Animation::binding_name_propagate()`
* would always be called when appropriate, this code could be removed. */
const Binding *binding = this->binding_for_handle(adt->binding_handle);
if (binding) {
This (trying to work around unknown potential future bug) is really not a good practice. It should rather assert that the binding names match.
I think there are other reasons beyond what I wrote here, so I've changed the comment to explain those, but retained the code. This is not a hot path (i.e. no animation evaluation or something, just unassigning, which is cheap) so copying a few extra bytes is not a big issue. The new comment:
This still feels like a weak hack to me... I would expect the code to ensure that the name is always in sync with the current assigned binding.
@ -135,0 +531,4 @@
Binding *binding = anim.find_suitable_binding_for(animated_id);
return anim.assign_id(binding, animated_id);
}
Why this
#ifndef NDEBUG
here?BLI_assert_msg
is a no-op in non-debug context already?@ -0,0 +55,4 @@
void TearDown() override
{
BKE_id_free(bmain, &cube->id);
Not sure why you are manually freeing these IDs here?
BKE_main_free
will do it for you since they all belong to the main...Allocating and freeing the IDs was already there, before
bmain
was introduced. I've removed the 'manual' free calls.@ -280,1 +282,4 @@
if (adt->animation) {
blender::animrig::Animation &anim = adt->animation->wrap();
return anim.binding_for_id(*id) != nullptr;
I don't think this code is correct:
adt->action
, think it should also check that the channel bag for this binding does have fcurves?Good call. I'll pull a utility function I actually wrote for #119875 into this PR, as that'll make this check considerably easier to write.
@ -224,0 +255,4 @@
return;
}
if (new_binding_handle == 0) {
More magic number used for unset binding
@ -224,0 +257,4 @@
if (new_binding_handle == 0) {
/* No need to check with the Animation, as 'no binding' is always valid. */
adt->binding_handle = 0;
More magic number used for unset binding
@ -1484,0 +1549,4 @@
RNA_def_property_ui_text(prop, "Animation", "Active Animation for this data-block");
RNA_def_property_update(prop, NC_ANIMATION | ND_ANIMCHAN, "rna_AnimData_dependency_update");
prop = RNA_def_property(srna, "animation_binding_handle", PROP_INT, PROP_NONE);
Missing API doc (aka tooltip).
@ -1484,0 +1554,4 @@
RNA_def_property_int_funcs(prop, nullptr, "rna_AnimData_animation_binding_handle_set", nullptr);
RNA_def_property_update(prop, NC_ANIMATION | ND_ANIMCHAN, "rna_AnimData_dependency_update");
prop = RNA_def_property(srna, "animation_binding_name", PROP_STRING, PROP_NONE);
Missing API doc (aka tooltip).
@ -0,0 +132,4 @@
if (animated_id == nullptr) {
BKE_report(reports,
RPT_ERROR,
"An binding without animated ID cannot be created at the moment; if you need it, "
"A binding
@ -0,0 +167,4 @@
if (anim.layers().size() >= 1) {
/* Not allowed to have more than one layer, for now. This limitation is in
* place until working with multiple animated IDs is fleshed binding better. */
This comment sentence does not look valid, maybe the last
binding
word should not be there?@ -0,0 +265,4 @@
if (layer.strips().size() >= 1) {
/* Not allowed to have more than one strip, for now. This limitation is in
* place until working with layers is fleshed binding better. */
Same as above, looks like the
binding
word does not belong here?@ -0,0 +285,4 @@
animrig::Layer &layer = dna_layer->wrap();
animrig::Strip &strip = dna_strip->wrap();
if (!layer.strip_remove(strip)) {
BKE_report(reports, RPT_ERROR, "this strip does not belong to this layer");
"This strip
@ -0,0 +374,4 @@
}
static AnimationChannelBag *rna_KeyframeAnimationStrip_channels(
KeyframeAnimationStrip *self, const animrig::binding_handle_t binding_handle)
"Add a binding
@ -0,0 +378,4 @@
{
animrig::KeyframeStrip &key_strip = self->wrap();
return key_strip.channelbag_for_binding(binding_handle);
}
Empty line is wrongly located here, should be after next line, or even completely removed.
@ -0,0 +406,4 @@
func, "binding", "AnimationBinding", "", "Newly created animation binding");
RNA_def_function_return(func, parm);
}
"Name of the layer, will be made unique within the
?@ -0,0 +445,4 @@
static void rna_def_animation(BlenderRNA *brna)
{
StructRNA *srna;
PropertyRNA *prop;
Is this description really accurate? I thought several IDs could share the same binding (at least in the future)?
We've improved the descriptions:
Binding
: "A set of channels in this Animation, that can be used by a data-block to specify what it gets animated by"Animation.bindings
: "The list of bindings in this animation data-block"They are not perfect yet, but more accurate than they were.
They can, and in the new description this is no longer excluded. It is considered to be a corner case, though, but still one that should be supported.
@ -0,0 +472,4 @@
prop = RNA_def_property(srna, "layers", PROP_COLLECTION, PROP_NONE);
RNA_def_property_struct_type(prop, "AnimationLayer");
RNA_def_property_collection_funcs(prop,
Again, as far as I understand the current design, this seems like over-simplified description of what is a binding?
@ -0,0 +474,4 @@
RNA_def_property_struct_type(prop, "AnimationLayer");
RNA_def_property_collection_funcs(prop,
"rna_iterator_animation_layers_begin",
"rna_iterator_array_next",
This property really needs a description!
@ -0,0 +479,4 @@
"rna_iterator_array_dereference_get",
"rna_iterator_animation_layers_length",
nullptr,
nullptr,
Also missing a proper description. Critical for API docs.
@ -0,0 +497,4 @@
"Reference to a data-block that will be animated by this Animation");
prop = RNA_def_property(srna, "name", PROP_STRING, PROP_NONE);
RNA_def_struct_name_property(srna, prop);
I would rephrase this to specify current limitations of the implementation aside from the generic info, something like that maybe:
"Add a new strip to the layer (currently, a layer can only have one strip, with infinite boundaries)"
@ -0,0 +535,4 @@
func = RNA_def_function(srna, "remove", "rna_AnimationStrips_remove");
RNA_def_function_flag(func, FUNC_USE_SELF_ID | FUNC_USE_CONTEXT | FUNC_USE_REPORTS);
RNA_def_function_ui_description(func, "Remove the strip from the animation layer");
parm = RNA_def_pointer(
A case of over-renaming of
output
tobinding
?@ -0,0 +543,4 @@
static void rna_def_animation_layer(BlenderRNA *brna)
{
StructRNA *srna;
PropertyRNA *prop;
Another case of over-renaming of
output
tobinding
...@ -0,0 +584,4 @@
nullptr);
RNA_def_property_ui_text(prop, "Strips", "The list of strips that are on this animation layer");
rna_def_animationlayer_strips(brna, prop);
Should decide if official name is
F-Curve
(as used by RNA code below), orFCurve
. ;)@ -0,0 +659,4 @@
"array_index",
-1,
-INT_MAX,
INT_MAX,
Name looks incorrectly put to plural here? Should be
rna_def_animation_channelbag
I think, since it's not defining a RNA collection type...On a meta-level, am not convinced by this multi-commit review thing. Already spent over 5h on this single initial review.... just reading code! The PR is monstrously big, it takes forever to process it fully. :(
Thanks for the review Bastien.
The PR is big, I agree. I don't see how splitting it up into several PRs would help though, as then handling review notes on the earlier commits is going to be a serious hassle to get into the other PRs. With everything in one, the code is always consistent, and you can see the bigger picture in action by building Blender and playing with it, while still being able to focus the review on a single commit.
The next force-push is just a rebase onto current
main
.050cf201ac
to32875e8b87
32875e8b87
toe9960842bb
Thanks for the thorough review @mont29! I've gone through all your notes, and force-pushed the resulting changes. The 'Compare' button above this comment should show what I changed.
Still needs a proper search and replace on
an binding
,an 'binding
, etc.For the rest think things are OK, though am still unhappy about the hack in unbinding animation code :|
@ -186,3 +319,3 @@
* be animated by.
*
* This is called an 'binding' because it acts like an binding socket of the
* This is called an 'binding' because it acts like a binding socket of the
another
an binding
...@ -241,0 +445,4 @@
* - By fallback string.
* - By the ID's name (matching agains the binding name).
* - If the above do not find a suitable binding, the animated ID will not
* receive any animation and the calller is responsible for creating an binding
an binding
👍
I somewhat agree (although feel less strongly about this). I'd rather wait removing that code until we have more actual practical experience with the new data-block. This is still an experimental feature, and I think it's fine to have some more experimental code in there.
e9960842bb
to9bfc4f7619
9bfc4f7619
tofaf425f60e
@blender-bot build
Would be good to add a
TODO: check on how to get rid of this hack
type of comment there then, to reduce the chances that this stays there forever ;)faf425f60e
to614cc0b90e
614cc0b90e
to178b0dccd0
The previous two force-pushes were, respectively, a rebase onto
main
and Bastien's comment suggestion.178b0dccd0
tof328dc994a
@blender-bot build