Baklava: minimal data model #118677

Manually merged
Sybren A. Stüvel merged 6 commits from dr.sybren/blender:pr/baklava-data-model into main 2024-04-08 17:27:15 +02:00

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.

Screenshot of the 3D Viewport

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](https://projects.blender.org/blender/blender/pulls/119077)). 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. ![Screenshot of the 3D Viewport](/attachments/bed9216a-b29b-4d3b-adfc-3ebae449549e)
Sybren A. Stüvel added this to the 4.2 LTS milestone 2024-02-23 17:20:19 +01:00
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-02-23 17:20:19 +01:00
Sybren A. Stüvel added this to the Animation & Rigging project 2024-02-23 17:20:31 +01:00
Falk David reviewed 2024-02-23 21:05:35 +01:00
Falk David left a comment
Member

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!

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.
Member

Might be worth it to name the file temp_anim_layers.py.

Might be worth it to name the file `temp_anim_layers.py`.
dr.sybren marked this conversation as resolved
@ -0,0 +27,4 @@
struct Main;
struct PointerRNA;
namespace blender::animrig {
Member

Maybe just blender::animation might be more appropriate? I see the folder name is also animrig. Does that still seem right?

Maybe just `blender::animation` might be more appropriate? I see the folder name is also `animrig`. Does that still seem right?
Author
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -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)
Member

@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.

@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.
dr.sybren marked this conversation as resolved
@ -177,0 +213,4 @@
}
if (strip->contains_frame(frame_time)) {
/* Found it! */
Member

While I appreciate the humor, it's probably best to leave out comments that don't add value :D

While I appreciate the humor, it's probably best to leave out comments that don't add value :D
dr.sybren marked this conversation as resolved
@ -0,0 +206,4 @@
return {};
}
static float lerp(const float t, const float a, const float b)
Member

Looks like this is just math::interpolate<float>

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.

Or even more, this is just `bke::attribute_math::mix2<float>`!) In case of thinking about this as about attributes.
Author
Member

math::interpolate<float> does a * (1 - t) + b * t
our lerp does a + 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 and t=1, but are not monotonic (i.e. can actually go backwards in value when t increases). The latter only guarantees a is returned with t=0, but not that b is returned for t=1, but it is guaranteed to be monotonic. And then there is std::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().

`math::interpolate<float>` does `a * (1 - t) + b * t` our `lerp` does `a + 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` and `t=1`, but are not monotonic (i.e. can actually go backwards in value when `t` increases). The latter only guarantees `a` is returned with `t=0`, but not that `b` is returned for `t=1`, but it is guaranteed to be monotonic. And then there is `std::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()`.
dr.sybren marked this conversation as resolved
@ -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();
Member

Better to use C++ casts here

Better to use C++ casts here
dr.sybren marked this conversation as resolved
Hans Goudey requested changes 2024-02-24 04:15:55 +01:00
Hans Goudey left a comment
Member

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:

  • stable_index This name wasn't very helpful to me. The fact that it's "stable" felt semantically a bit wrong (it's not the index that's stable, it's the data it references), and more of an implementation necessity (or even something I'd assume about indices) than something worth stating in variable names. To me "output_index" would be a much more helpful name, since it's always quite visible what sort of thing it's referencing.
  • free_data A few of the classes have this method. It feels a bit unnecessary though, like it would be better to use the typical system for this in C++, destructors (which also free owned data but not the class itself). I would keep the method for Animation though, since the topic of constructors and destructors for ID data-blocks brings up complexity and design decisions that we probably don't want to get into now.
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: - **stable_index** This name wasn't very helpful to me. The fact that it's "stable" felt semantically a bit wrong (it's not the index that's stable, it's the data it references), and more of an implementation necessity (or even something I'd assume about indices) than something worth stating in variable names. To me "output_index" would be a much more helpful name, since it's always quite visible what sort of thing it's referencing. - **free_data** A few of the classes have this method. It feels a bit unnecessary though, like it would be better to use the typical system for this in C++, destructors (which also free owned data but not the class itself). I would keep the method for `Animation` though, since the topic of constructors and destructors for `ID` 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.
Member

We hardly have any C files anymore, not sure it's worth including this

We hardly have any C files anymore, not sure it's worth including this
dr.sybren marked this conversation as resolved
@ -0,0 +35,4 @@
class Output;
/* Use an alias for the stable index type. */
using output_index_t = decltype(::AnimationOutput::stable_index);
Member

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")

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")
Author
Member

It's indeed the latter, instead of having int32_t and int64_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 (like size_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.

It's indeed the latter, instead of having `int32_t` and `int64_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 (like `size_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.
dr.sybren marked this conversation as resolved
@ -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. */
Member

*/ is meant to be on a separate line at the end for this comment style, for multi-line doc-strings

`*/` is meant to be on a separate line at the end for this comment style, for multi-line doc-strings
dr.sybren marked this conversation as resolved
@ -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);
Member

If the ID can't be null, it could be a reference. That might apply in other places too

If the ID can't be null, it could be a reference. That might apply in other places too
Author
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -0,0 +125,4 @@
int64_t find_layer_index(const Layer &layer) const;
private:
Output &output_allocate_();
Member

Typically methods don't get the _ suffix

Typically methods don't get the `_` suffix
dr.sybren marked this conversation as resolved
@ -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 {
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -0,0 +207,4 @@
bool is_last_frame(float frame_time) const;
/**
* Set the start and end frame.
Member

frame -> frames

`frame` -> `frames`
Author
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -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);
Member

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 ;)

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 ;)
Author
Member

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 be channels_for_output_for_output, as it returns the ChannelsForOutput for the given output.

Maybe ChannelBag would be better term for the class, and then the function could just be named channelbag_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.

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 be `channels_for_output_for_output`, as it returns the `ChannelsForOutput` for the given output. Maybe `ChannelBag` would be better term for the class, and then the function could just be named `channelbag_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`.
dr.sybren marked this conversation as resolved
@ -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);
Member

const int -> int

The const is meaningless in the declaration (rather than the definition)

`const int` -> `int` The const is meaningless in the declaration (rather than the definition)
dr.sybren marked this conversation as resolved
@ -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)
Member

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 too

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 too
Author
Member

We added a note to the comment, reminding us of this.

We added a note to the comment, reminding us of this.
dr.sybren marked this conversation as resolved
@ -0,0 +279,4 @@
return const_cast<Output *>(out);
}
const Output *Animation::output_for_id(const ID *animated_id) const
Member

const ID * -> const ID &

`const ID *` -> `const ID &`
dr.sybren marked this conversation as resolved
@ -0,0 +280,4 @@
}
const Output *Animation::output_for_id(const ID *animated_id) const
Member

Extra whitespace

Extra whitespace
dr.sybren marked this conversation as resolved
@ -0,0 +421,4 @@
adt->animation = nullptr;
}
/* ----- AnimationLayer C++ implementation ----------- */
Member

The whole thing is implemented in C++, I'd just remove "C++" from the comment

The whole thing is implemented in C++, I'd just remove "C++" from the comment
dr.sybren marked this conversation as resolved
@ -0,0 +435,4 @@
}
const Strip *Layer::strip(const int64_t index) const
{
return &this->strip_array[index]->wrap();
Member

Couldn't these return references?

Couldn't these return references?
dr.sybren marked this conversation as resolved
@ -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 *));
Member

The DNA_array_utils.hh code would hopefully be helpful here too

The `DNA_array_utils.hh` code would hopefully be helpful here too
dr.sybren marked this conversation as resolved
@ -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
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -0,0 +590,4 @@
this->as<animrig::KeyframeStrip>().free_data();
return;
}
BLI_assert(!"unfreeable strip type!");
Member

Just BLI_assert_unreachable() would be pretty self explanatory here

Just `BLI_assert_unreachable()` would be pretty self explanatory here
dr.sybren marked this conversation as resolved
@ -0,0 +701,4 @@
const int array_index)
{
FCurve *fcurve = this->fcurve_find(out, rna_path, array_index);
if (fcurve) {
Member

If you do this:

if (FCurve *fcurve = this->fcurve_find(out, rna_path, array_index)) {
  return fcurve;
}

then you can declare the variable where its value is assigned below :)

If you do this: ``` if (FCurve *fcurve = this->fcurve_find(out, rna_path, array_index)) { return fcurve; } ``` then you can declare the variable where its value is assigned below :)
dr.sybren marked this conversation as resolved
@ -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),
Member
  • min_ff -> std::min
  • fabs -> std::abs
- `min_ff` -> `std::min` - `fabs` -> `std::abs`
dr.sybren marked this conversation as resolved
@ -0,0 +184,4 @@
}
}
/* Returns whether the strip was evaluated. */
Member

Outdated comment?

Outdated comment?
dr.sybren marked this conversation as resolved
@ -0,0 +186,4 @@
/* Returns whether the strip was evaluated. */
static EvaluationResult evaluate_strip(PointerRNA *animated_id_ptr,
Strip &strip,
Member

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.

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.
Author
Member

Yup, calculate_fcurve() in source/blender/blenkernel/intern/fcurve.cc writes to fcu->curval. Would be nice to get rid of that at some point, but that's out of scope for this PR.

Yup, `calculate_fcurve()` in `source/blender/blenkernel/intern/fcurve.cc` writes to `fcu->curval`. Would be nice to get rid of that at some point, but that's out of scope for this PR.
dr.sybren marked this conversation as resolved
@ -0,0 +261,4 @@
namespace internal {
EvaluationResult evaluate_layer(PointerRNA *animated_id_ptr,
Member

This can be a reference

This can be a reference
dr.sybren marked this conversation as resolved
@ -0,0 +279,4 @@
continue;
}
const auto strip_result = evaluate_strip(
Member

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 at evaluate_strip to find what it is.

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 at `evaluate_strip` to find what it is.
dr.sybren marked this conversation as resolved
@ -0,0 +50,4 @@
: value(value), prop_rna(prop_rna)
{
}
~AnimatedProperty() = default;
Member

Not sure there's any need to write the defaulted destructor explicitly like this?

Not sure there's any need to write the defaulted destructor explicitly like this?
Author
Member

I'm not sure either, but it's done in other places as well (for example ~Set() = default;).

I'm not sure either, but it's done in other places as well (for example `~Set() = default;`).
Member

Generally I'd recommend not doing it, but here are some exceptions:

  • You want the destructor to be defined in a .cc instead of in the .hh file.
  • There are other non-default special methods (move/assign/...) in the type. E.g. removing Set(const Set &other) = default; would result in compile errors because the compiler would think that Set 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.

Generally I'd recommend not doing it, but here are some exceptions: * You want the destructor to be defined in a `.cc` instead of in the `.hh` file. * There are other non-default special methods (move/assign/...) in the type. E.g. removing `Set(const Set &other) = default;` would result in compile errors because the compiler would think that `Set` 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.
dr.sybren marked this conversation as resolved
@ -0,0 +77,4 @@
public:
operator bool() const
{
return !is_empty();
Member

is_empty() -> this->is_empty()

`is_empty()` -> `this->is_empty()`
dr.sybren marked this conversation as resolved
@ -0,0 +83,4 @@
{
return result_.is_empty();
}
EvaluationResult &operator=(EvaluationResult other)
Member

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.

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.
Author
Member

If I remove it, I get these errors:

blender/source/blender/animrig/intern/evaluation.cc:61:19: error: object of type 'blender::animrig::internal::EvaluationResult' cannot be assigned because its copy assignment operator is implicitly deleted
      last_result = layer_result;
                  ^
blender/source/blender/animrig/intern/evaluation_internal.hh:74:3: note: copy assignment operator is implicitly deleted because 'EvaluationResult' has a user-declared move constructor
  EvaluationResult(EvaluationResult &&other) : result_(std::move(other.result_)) {}
  ^
blender/source/blender/animrig/intern/evaluation.cc:66:17: error: object of type 'blender::animrig::internal::EvaluationResult' cannot be assigned because its copy assignment operator is implicitly deleted
    last_result = blend_layer_results(last_result, layer_result, *layer);
                ^
blender/source/blender/animrig/intern/evaluation_internal.hh:74:3: note: copy assignment operator is implicitly deleted because 'EvaluationResult' has a user-declared move constructor
  EvaluationResult(EvaluationResult &&other) : result_(std::move(other.result_)) {}
  ^
blender/source/blender/animrig/intern/evaluation.cc:286:24: error: object of type 'blender::animrig::internal::EvaluationResult' cannot be assigned because its copy assignment operator is implicitly deleted
      last_weak_result = strip_result;
                       ^
blender/source/blender/animrig/intern/evaluation_internal.hh:74:3: note: copy assignment operator is implicitly deleted because 'EvaluationResult' has a user-declared move constructor
  EvaluationResult(EvaluationResult &&other) : result_(std::move(other.result_)) {}
  ^
3 errors generated.

I do want to have some move semantics, but maybe I'm doing it wrong? Open for suggestions.

If I remove it, I get these errors: ``` blender/source/blender/animrig/intern/evaluation.cc:61:19: error: object of type 'blender::animrig::internal::EvaluationResult' cannot be assigned because its copy assignment operator is implicitly deleted last_result = layer_result; ^ blender/source/blender/animrig/intern/evaluation_internal.hh:74:3: note: copy assignment operator is implicitly deleted because 'EvaluationResult' has a user-declared move constructor EvaluationResult(EvaluationResult &&other) : result_(std::move(other.result_)) {} ^ blender/source/blender/animrig/intern/evaluation.cc:66:17: error: object of type 'blender::animrig::internal::EvaluationResult' cannot be assigned because its copy assignment operator is implicitly deleted last_result = blend_layer_results(last_result, layer_result, *layer); ^ blender/source/blender/animrig/intern/evaluation_internal.hh:74:3: note: copy assignment operator is implicitly deleted because 'EvaluationResult' has a user-declared move constructor EvaluationResult(EvaluationResult &&other) : result_(std::move(other.result_)) {} ^ blender/source/blender/animrig/intern/evaluation.cc:286:24: error: object of type 'blender::animrig::internal::EvaluationResult' cannot be assigned because its copy assignment operator is implicitly deleted last_weak_result = strip_result; ^ blender/source/blender/animrig/intern/evaluation_internal.hh:74:3: note: copy assignment operator is implicitly deleted because 'EvaluationResult' has a user-declared move constructor EvaluationResult(EvaluationResult &&other) : result_(std::move(other.result_)) {} ^ 3 errors generated. ``` I do want to have some move semantics, but maybe I'm doing it wrong? Open for suggestions.
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -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)
Member
  • const char rna_path[] -> const StringRef rna_path
  • BLI_strdup(rna_path) -> BLI_strdupn(rna_path.data(), rna_path.size());
- `const char rna_path[]` -> `const StringRef rna_path` - `BLI_strdup(rna_path)` -> `BLI_strdupn(rna_path.data(), rna_path.size());`
dr.sybren marked this conversation as resolved
@ -0,0 +41,4 @@
const AnimationChannelsForOutput *channels_src);
using anim_strip_duplicator = AnimationStrip *(*)(const AnimationStrip *strip_src);
using anim_strip_freeer = void (*)(AnimationStrip *strip);
Member

anim_strip_freeer is unused

`anim_strip_freeer` is unused
dr.sybren marked this conversation as resolved
@ -0,0 +97,4 @@
static AnimationOutput *anim_output_duplicate(const AnimationOutput *output_src)
{
AnimationOutput *output_dup = static_cast<AnimationOutput *>(MEM_dupallocN(output_src));
Member

Except for Animation (which has all the complexity of being an ID data-block), it feels like these should be implemented as copy constructors. For the calling code it should be as simple as using MEM_new<AnimationOutput>(__func__, src);. I think abstracting it more than that just means you have to dig deeper to understand what's going on.

Except for `Animation` (which has all the complexity of being an `ID` data-block), it feels like these should be implemented as copy constructors. For the calling code it should be as simple as using `MEM_new<AnimationOutput>(__func__, src);`. I think abstracting it more than that just means you have to dig deeper to understand what's going on.
dr.sybren marked this conversation as resolved
@ -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));
Member

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?

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?
dr.sybren marked this conversation as resolved
@ -1643,0 +1675,4 @@
Animation *dna_animation)
{
BLI_assert(id != nullptr);
BLI_assert(operation_from != nullptr);
Member

Use references rather than asserting for null pointers

Use references rather than asserting for null pointers
Author
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -1211,0 +1260,4 @@
typedef struct Animation {
ID id;
struct AnimationLayer **layer_array; /* Array of 'layer_array_num' layers. */
Member

How about this order?

  struct AnimationLayer **layer_array; /* Array of 'layer_array_num' layers. */
  int layer_array_num;
  int layer_active_index; /* Index into layer_array, -1 means 'no active'. */

  struct AnimationOutput **output_array; /* Array of 'output_array_num` outputs. */
  int output_array_num;
  int32_t last_output_stable_index;
How about this order? ``` struct AnimationLayer **layer_array; /* Array of 'layer_array_num' layers. */ int layer_array_num; int layer_active_index; /* Index into layer_array, -1 means 'no active'. */ struct AnimationOutput **output_array; /* Array of 'output_array_num` outputs. */ int output_array_num; int32_t last_output_stable_index; ```
dr.sybren marked this conversation as resolved
@ -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. */
Member

Comment formatting

  /**
   * 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 ``` /** * There is always at least one strip. * If there is only one, it can be infinite. This is the default for new layers. */ ```
dr.sybren marked this conversation as resolved
@ -1211,0 +1345,4 @@
typedef struct AnimationStrip {
/** eAnimationStrip_type */
uint8_t type;
Member

I'd suggest wrapping this with a type() accessor in animrig::Strip that can return the proper enum type. Then this could be renamed strip_type or something more verbose like that so people prefer using the shorter accessor.

I'd suggest wrapping this with a `type()` accessor in `animrig::Strip` that can return the proper enum type. Then this could be renamed `strip_type` or something more verbose like that so people prefer using the shorter accessor.
dr.sybren marked this conversation as resolved
@ -1211,0 +1350,4 @@
float frame_start;
float frame_end;
float frame_offset;
Member

These three could use some comments. I was particularly curious why the offset is necessary when there are already a start and end stored

These three could use some comments. I was particularly curious why the offset is necessary when there are already a start and end stored
dr.sybren marked this conversation as resolved
@ -1211,0 +1358,4 @@
#endif
} AnimationStrip;
typedef enum eAnimationStrip_type {
Member

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 the animrig::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).

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 the `animrig::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).
dr.sybren marked this conversation as resolved
@ -0,0 +35,4 @@
"OFFSET",
0,
"Offset",
"Animation channels in this layer are added, as sequential operations, to the output of "
Member

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

`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
Author
Member

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 ;-)

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 ;-)
Member

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.

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.
Author
Member

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.

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.
Member

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".

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".
Author
Member

No sarcasm here, just a bit of humor.

I understand your points, and I'll see what I can come up with.

No sarcasm here, just a bit of humor. I understand your points, and I'll see what I can come up with.
dr.sybren marked this conversation as resolved
@ -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");
Member

this -> This

`this` -> `This`
Member
  • the first one ;)
* the first one ;)
dr.sybren marked this conversation as resolved
Author
Member

Thanks for all the feedback, this is super useful!

Thanks for all the feedback, this is super useful!
Author
Member

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.

👍

  • stable_index This name wasn't very helpful to me. The fact that it's "stable" felt semantically a bit wrong (it's not the index that's stable, it's the data it references)

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 an Output, regardless of how these Outputs are stored in the array, reshuffled, etc. Your remark makes it clear that this should be named and/or documented better.

, and more of an implementation necessity (or even something I'd assume about indices) than something worth stating in variable names. To me "output_index" would be a much more helpful name, since it's always quite visible what sort of thing it's referencing.

Maybe output_id or output_handle would be better; just naming it plain "index" is too much "this indexes into the storage array" for me.

  • free_data A few of the classes have this method. It feels a bit unnecessary though, like it would be better to use the typical system for this in C++, destructors (which also free owned data but not the class itself). I would keep the method for Animation though, since the topic of constructors and destructors for ID data-blocks brings up complexity and design decisions that we probably don't want to get into now.

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.

> 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. :+1: > - **stable_index** This name wasn't very helpful to me. The fact that it's "stable" felt semantically a bit wrong (it's not the index that's stable, it's the data it references) 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 an `Output`, regardless of how these `Output`s are stored in the array, reshuffled, etc. Your remark makes it clear that this should be named and/or documented better. >, and more of an implementation necessity (or even something I'd assume about indices) than something worth stating in variable names. To me "output_index" would be a much more helpful name, since it's always quite visible what sort of thing it's referencing. Maybe `output_id` or `output_handle` would be better; just naming it plain "index" is too much "this indexes into the storage array" for me. > - **free_data** A few of the classes have this method. It feels a bit unnecessary though, like it would be better to use the typical system for this in C++, destructors (which also free owned data but not the class itself). I would keep the method for `Animation` though, since the topic of constructors and destructors for `ID` data-blocks brings up complexity and design decisions that we probably don't want to get into now. 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.
Member

Maybe output_id or output_handle would be better; just naming it plain "index" is too much "this indexes into the storage array" for 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.

Then again, even the outputs, layers, and strips are stored in DNA, and so can still be allocated through C allocations.

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.

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.

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 `output_id` or `output_handle` would be better; just naming it plain "index" is too much "this indexes into the storage array" for 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. > Then again, even the outputs, layers, and strips are stored in DNA, and so can still be allocated through C allocations. 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. > 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. 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.
Member

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).

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: https://projects.blender.org/blender/blender/pulls/117347#issuecomment-1107453.
Author
Member

Maybe output_id or output_handle would be better; just naming it plain "index" is too much "this indexes into the storage array" for 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.

Nathan and I discussed it a bit more, and feel that output_handle is the better choice here. id is too close to ID, and we feel that this could lead to confusion.

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).

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:

While a pointer contains the address of the item to which it refers, a handle is an abstraction of a reference which is managed externally; its opacity allows the referent to be relocated in memory by the system without invalidating the handle.

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.

> > Maybe `output_id` or `output_handle` would be better; just naming it plain "index" is too much "this indexes into the storage array" for 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. Nathan and I discussed it a bit more, and feel that `output_handle` is the better choice here. `id` is too close to `ID`, and we feel that this could lead to confusion. > 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: https://projects.blender.org/blender/blender/pulls/117347#issuecomment-1107453. That wouldn't apply here, at least not when using [the definition of 'Unique Identifier' from Wikipedia](https://en.wikipedia.org/wiki/Unique_identifier), 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'](https://en.wikipedia.org/wiki/Handle_(computing)) 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: > While a pointer contains the address of the item to which it refers, a handle is an abstraction of a reference which is managed externally; its opacity allows the referent to be relocated in memory by the system without invalidating the handle. 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.
Author
Member

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.

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](https://projects.blender.org/blender/blender/compare/main...dr.sybren:pr/baklava-data-model-partial-pr-feedback).
Sybren A. Stüvel force-pushed pr/baklava-data-model from 0c54f503ec to 1a823d08d5 2024-03-04 18:11:08 +01:00 Compare
Sybren A. Stüvel requested review from Hans Goudey 2024-03-04 18:12:37 +01:00
Author
Member

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.

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.
Author
Member

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.

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.
Sybren A. Stüvel force-pushed pr/baklava-data-model from 1a823d08d5 to ad85a78c45 2024-03-05 12:27:57 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from ad85a78c45 to 836ca1d5a3 2024-03-05 12:43:22 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 836ca1d5a3 to efd90da5b7 2024-03-05 14:03:55 +01:00 Compare
Falk David requested changes 2024-03-05 14:14:18 +01:00
Dismissed
Falk David left a comment
Member

Added some comments.

Added some comments.
@ -0,0 +70,4 @@
col.prop(layer, "influence")
col.prop(layer, "mix_mode")
# for strip_idx, strip in enumerate(layer.strips):
Member

Is this code still needed? Otherwise would be better to just remove it.

Is this code still needed? Otherwise would be better to just remove it.
dr.sybren marked this conversation as resolved
@ -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);
/**
Member

I suppose this could return FCurve & since it should always return an f-curve.

I suppose this could return `FCurve &` since it should always return an f-curve.
Author
Member

Meta remark: Hmm this comment got dislodged a bit. It's now showing underneath this line:

ChannelBag &channelbag_for_output_add(const Output &out);

but I'm guessing it's about

FCurve *fcurve_find_or_create(const Output &out, StringRefNull rna_path, int array_index);
Meta remark: Hmm this comment got dislodged a bit. It's now showing underneath this line: ```cpp ChannelBag &channelbag_for_output_add(const Output &out); ``` but I'm guessing it's about ```cpp FCurve *fcurve_find_or_create(const Output &out, StringRefNull rna_path, int array_index); ```
dr.sybren marked this conversation as resolved
@ -177,0 +226,4 @@
found_on_other_strip = fcu;
found_at_time_distance = this_distance;
}
} break;
Member

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 the case.

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 the `case`.
Author
Member

yeah it is, this is a mistake.

yeah it is, this is a mistake.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel force-pushed pr/baklava-data-model from efd90da5b7 to 968e9b6f43 2024-03-05 14:20:53 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 968e9b6f43 to 3833509cea 2024-03-05 16:49:53 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 3833509cea to 1a135a904e 2024-03-05 17:12:30 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 1a135a904e to d43418fe6d 2024-03-05 17:37:31 +01:00 Compare
Sybren A. Stüvel requested review from Falk David 2024-03-05 17:37:49 +01:00
Sybren A. Stüvel requested review from Bastien Montagne 2024-03-07 11:06:49 +01:00
Sybren A. Stüvel force-pushed pr/baklava-data-model from d43418fe6d to 5f7a715de5 2024-03-07 16:53:29 +01:00 Compare
Author
Member

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.

What was once the first commit of this PR was reviewed in #119077. Now that that has landed in `main` (90ef46baa1b), 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.
Sybren A. Stüvel force-pushed pr/baklava-data-model from 5f7a715de5 to d0cb8e2a20 2024-03-11 12:25:42 +01:00 Compare
Author
Member

The next force-push simplifies the last commit in the stack: Anim, show animated properties with the appropriate color in the GUI. It removes the frame_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.

The next force-push simplifies the last commit in the stack: `Anim, show animated properties with the appropriate color in the GUI`. It removes the `frame_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.
Sybren A. Stüvel force-pushed pr/baklava-data-model from d0cb8e2a20 to 319e132a97 2024-03-11 12:37:12 +01:00 Compare
Author
Member

Next force-push will fix a compiler warning & fix something that got introduced when I rebased on main earlier. No functional changes.

Next force-push will fix a compiler warning & fix something that got introduced when I rebased on `main` earlier. No functional changes.
Sybren A. Stüvel force-pushed pr/baklava-data-model from 319e132a97 to 2f123b6b62 2024-03-11 14:20:19 +01:00 Compare
Falk David requested changes 2024-03-11 14:39:35 +01:00
Dismissed
Falk David left a comment
Member

Added some comments.

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;
Member
Use `this->` for non-private members. https://developer.blender.org/docs/handbook/guidelines/c_cpp/#using-this-
dr.sybren marked this conversation as resolved
@ -0,0 +72,4 @@
~EvaluationResult() = default;
public:
operator bool() const
Member

I'd like to hear others opinion about "syntactic sugar" like this vs. just using is_empty().

I'd like to hear others opinion about "syntactic sugar" like this vs. just using `is_empty()`.
Author
Member

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 with if (!result.is_empty()). Of course that can be avoided with has_data() or something like that, but I quite like the boolean operator for container-like data structures.

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 with `if (!result.is_empty())`. Of course that can be avoided with `has_data()` or something like that, but I quite like the boolean operator for container-like data structures.
Author
Member

In the discussion in Blender Chat there was no major objection to using operator bool(), so for now I'll keep it in.

In the discussion in Blender Chat there was no major objection to using `operator bool()`, so for now I'll keep it in.
dr.sybren marked this conversation as resolved
@ -0,0 +120,4 @@
const int array_index,
const float eval_time)
{
const std::optional<float> eval_value = evaluate_single_property(
Member

I suppose this could be called directly in the if statement since the variable is not needed otherwise.

I suppose this could be called directly in the `if` statement since the variable is not needed otherwise.
Author
Member

You mean like this?

    if (const std::optional<float> eval_value = evaluate_single_property(
            rna_path, array_index, eval_time))
    { ... }

I don't think I find that easier to read. Or maybe I'm misunderstanding something?

You mean like this? ```cpp if (const std::optional<float> eval_value = evaluate_single_property( rna_path, array_index, eval_time)) { ... } ``` I don't think I find that easier to read. Or maybe I'm misunderstanding something?
Member

Yes like that. For std::optionals, 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.

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.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel force-pushed pr/baklava-data-model from 2f123b6b62 to 974deb2c6c 2024-03-11 14:53:48 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 974deb2c6c to 18e41d1a1a 2024-03-14 13:33:45 +01:00 Compare
Author
Member

The next force-push avoids some std::string copies when evaluating animation data.

It also removes the Strip() default constructor; the destructor assumes that Strip::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 to Strip::Type, for example Unset or Invalid, but that would force all switch(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 strip

The next force-push avoids some `std::string` copies when evaluating animation data. It also removes the `Strip()` default constructor; the destructor assumes that `Strip::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 to `Strip::Type`, for example `Unset` or `Invalid`, but that would force all `switch(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 strip
Sybren A. Stüvel force-pushed pr/baklava-data-model from 18e41d1a1a to 8239c859f4 2024-03-14 14:25:32 +01:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel force-pushed pr/baklava-data-model from 8239c859f4 to 145e22fa99 2024-03-19 11:16:43 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 145e22fa99 to a722bd919b 2024-03-19 11:24:14 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from a722bd919b to 5c0ca4c4f6 2024-03-19 16:40:07 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 5c0ca4c4f6 to 15b1710210 2024-03-25 12:32:10 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 15b1710210 to 87bdcb31ff 2024-03-25 12:33:00 +01:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2024-03-25 15:10:29 +01:00
Hans Goudey left a comment
Member

Just a few cleanup comments this time. I don't have any larger comments.

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
Member

Newline before \param

Newline before `\param`
dr.sybren marked this conversation as resolved
@ -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. */
Member

*/ should be on the next line

`*/` should be on the next line
dr.sybren marked this conversation as resolved
@ -76,0 +327,4 @@
}
}
/* Try the binding name from the AnimData, if it is set,*/
Member

, -> .

`,` -> `. `
dr.sybren marked this conversation as resolved
@ -26,6 +27,9 @@
#include "RNA_access.hh"
#include "RNA_path.hh"
#include <algorithm>
Member

Not sure what these new includes are for?

Not sure what these new includes are for?
dr.sybren marked this conversation as resolved
@ -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. */
Member

*/ on newline

`*/` on newline
dr.sybren marked this conversation as resolved
@ -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?",
Member

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

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
dr.sybren marked this conversation as resolved
@ -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. */
Member

stable index -> handle

`stable index` -> `handle`
dr.sybren marked this conversation as resolved
@ -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) {
Member

bindings.first_index_try can simplify this

`bindings.first_index_try` can simplify this
Author
Member

Now 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.

Now 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.
dr.sybren marked this conversation as resolved
@ -0,0 +318,4 @@
for (animrig::Layer *layer : anim.layers()) {
Span<animrig::Strip *> strips = layer->strips();
for (int i = 0; i < strips.size(); ++i) {
Member

same here

same here
dr.sybren marked this conversation as resolved
@ -0,0 +357,4 @@
const float time)
{
if (dna_binding == nullptr) {
BKE_report(reports, RPT_ERROR, "binding cannot be None");
Member

binding -> Binding

`binding` -> `Binding`
dr.sybren marked this conversation as resolved
@ -0,0 +534,4 @@
parm = RNA_def_enum(func,
"type",
rna_enum_strip_type_items,
(int)animrig::Strip::Type::Keyframe,
Member

(int)animrig::Strip::Type::Keyframe -> int(animrig::Strip::Type::Keyframe)

`(int)animrig::Strip::Type::Keyframe` -> `int(animrig::Strip::Type::Keyframe)`
dr.sybren marked this conversation as resolved
@ -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");
Member

We're trying not to use "ID" in the UI, how about multi-data-block?

We're trying not to use "ID" in the UI, how about multi-data-block?
dr.sybren marked this conversation as resolved
Hans Goudey reviewed 2024-03-25 15:22:47 +01:00
@ -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;
Member

Remove const in declaration

Remove const in declaration
dr.sybren marked this conversation as resolved
@ -0,0 +13,4 @@
class PropIdentifier {
public:
StringRefNull rna_path;
Member

It's probably worth having a comment about where these strings are owned

It's probably worth having a comment about where these strings are owned
dr.sybren marked this conversation as resolved
Sybren A. Stüvel force-pushed pr/baklava-data-model from 87bdcb31ff to d511f85db6 2024-03-25 16:53:11 +01:00 Compare
Author
Member

Thanks for the review @HooglyBoogly !

The next force-push will have your feedback incorporated.

Thanks for the review @HooglyBoogly ! The next force-push will have your feedback incorporated.
Sybren A. Stüvel force-pushed pr/baklava-data-model from d511f85db6 to caf1379e82 2024-03-25 17:35:14 +01:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Falk David approved these changes 2024-03-25 18:34:08 +01:00
Falk David left a comment
Member

Looked through this once more. Don't have any comments. Accepting.

Looked through this once more. Don't have any comments. Accepting.
Sybren A. Stüvel force-pushed pr/baklava-data-model from caf1379e82 to 050cf201ac 2024-03-28 12:17:56 +01:00 Compare
Bastien Montagne requested changes 2024-04-02 15:39:27 +02:00
Dismissed
Bastien Montagne left a comment
Owner

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.

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:

void binding_name_define(Binding &binding, StringRefNull new_name);
void binding_name_propagate(Main &bmain, std::span<const Binding &> bindings);

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).

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: ``` void binding_name_define(Binding &binding, StringRefNull new_name); void binding_name_propagate(Main &bmain, std::span<const Binding &> bindings); ``` 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).
mont29 marked this conversation as resolved
@ -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 on nullptr value?

Could use an `std::optional` to a Binding reference instead of relying on `nullptr` value?
Author
Member

What would semantically be the difference?

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.

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.
Member

std::optional<T &> does not exist in C++ currently.

There are no optional references; a program is ill-formed if it instantiates an optional with a reference type

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.

`std::optional<T &>` does not exist in C++ currently. > There are no optional references; a program is ill-formed if it instantiates an optional with a reference type 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.

Ah... my bad, indeed... Then agree that raw pointer is fine here.
Author
Member

Thanks for confirming my suspicion, Hans.

it's easy when editing existing code to forget when a pointer may or may not be null

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.

Thanks for confirming my suspicion, Hans. > it's easy when editing existing code to forget when a pointer may or may not be null 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.
mont29 marked this conversation as resolved
@ -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

`Note that this is different`
dr.sybren marked this conversation as resolved
@ -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

`matching against the binding`
Author
Member

I think you're suggesting this:

Find an FCurve matching against the binding + RNA path + array index combination.

I don't find that significantly clearer, though.

I think you're suggesting this: > Find an FCurve matching against the binding + RNA path + array index combination. 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 in against), search for agains the binding IIRC

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` in `against`), search for `agains 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

`creating a binding`
Author
Member

Not sure what you're suggesting here. I think Gitea lost the context of where this note had to be.

Not sure what you're suggesting here. I think Gitea lost the context of where this note had to be.
Member

Looks like the last force push caused all comments to be outdated and in the wrong place. Ouch :/

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,

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.

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.
Author
Member

Good find. I'm getting too used to Go's "initialise to the zero value" behaviour ;-)

Good find. I'm getting too used to Go's "initialise to the zero value" behaviour ;-)
dr.sybren marked this conversation as resolved
@ -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?

The part of the comment about `shrink_array` seems outdated, since it's defined below?
dr.sybren marked this conversation as resolved
@ -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); ?

`MEM_SAFE_FREE(*array);` ?
dr.sybren marked this conversation as resolved
@ -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'' define

Technically, since this is a signed int32, it's a 31-bit overflow ;) Also, usage of 'magic number' `0` instead of proper 'unset'' define
Author
Member

Good 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.

Good 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.
dr.sybren marked this conversation as resolved
@ -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 in DNA_anim_types.h!

Found two places where this magic 0 value is used, but fairly sure there are more to uncover.

Eeeeeeeeek! Magic Number! I think that an `UNSET_BINDING_HANDLE` define or constexpr is required in `DNA_anim_types.h`! Found two places where this magic `0` value is used, but fairly sure there are more to uncover.
Author
Member

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 the animrig::Binding class.

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 the `animrig::Binding` class.
dr.sybren marked this conversation as resolved
@ -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 is nullptr.

Should check and early-return in case the `adt` is `nullptr`.
Author
Member

It shouldn't, because calling anim.unassign_id(id_not_animated_by_anim) is a bug. I've added an extra BLI_assert_msg(adt, ...) though.

It shouldn't, because calling `anim.unassign_id(id_not_animated_by_anim)` is a bug. I've added an extra `BLI_assert_msg(adt, ...)` though.
Author
Member

And added this restriction to the documentation of the function.

And added this restriction to the documentation of the function.
dr.sybren marked this conversation as resolved
@ -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.

This (trying to work around unknown potential future bug) is really not a good practice. It should rather assert that the binding names match.
Author
Member

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:

  /* Before unassigning, make sure that the stored Binding name is up to date. The binding name
   * might have changed in a way that wasn't copied into the ADT yet (for example when the
   * Animation data-block is linked from another file), so better copy the name to be sure that it
   * can be transparently reassigned later. */
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: ```cpp /* Before unassigning, make sure that the stored Binding name is up to date. The binding name * might have changed in a way that wasn't copied into the ADT yet (for example when the * Animation data-block is linked from another file), so better copy the name to be sure that it * can be transparently reassigned later. */ ```

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.

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.
dr.sybren marked this conversation as resolved
@ -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?

Why this `#ifndef NDEBUG` here? `BLI_assert_msg` is a no-op in non-debug context already?
dr.sybren marked this conversation as resolved
@ -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...

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...
Author
Member

Allocating and freeing the IDs was already there, before bmain was introduced. I've removed the 'manual' free calls.

Allocating and freeing the IDs was already there, before `bmain` was introduced. I've removed the 'manual' free calls.
dr.sybren marked this conversation as resolved
@ -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:

  1. It fully by-passes the rest of the checks (drivers are particularly important to not skip here I think?).
  2. It is not accurate enough compared to the check on adt->action, think it should also check that the channel bag for this binding does have fcurves?
I don't think this code is correct: 1. It fully by-passes the rest of the checks (drivers are particularly important to not skip here I think?). 2. It is not accurate enough compared to the check on `adt->action`, think it should also check that the channel bag for this binding does have fcurves?
Author
Member

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.

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.
dr.sybren marked this conversation as resolved
@ -224,0 +255,4 @@
return;
}
if (new_binding_handle == 0) {

More magic number used for unset binding

More magic number used for unset binding
dr.sybren marked this conversation as resolved
@ -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

More magic number used for unset binding
dr.sybren marked this conversation as resolved
@ -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).

Missing API doc (aka tooltip).
dr.sybren marked this conversation as resolved
@ -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).

Missing API doc (aka tooltip).
dr.sybren marked this conversation as resolved
@ -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

`"A binding `
dr.sybren marked this conversation as resolved
@ -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?

This comment sentence does not look valid, maybe the last `binding` word should not be there?
dr.sybren marked this conversation as resolved
@ -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?

Same as above, looks like the `binding` word does not belong here?
dr.sybren marked this conversation as resolved
@ -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

`"This strip `
dr.sybren marked this conversation as resolved
@ -0,0 +374,4 @@
}
static AnimationChannelBag *rna_KeyframeAnimationStrip_channels(
KeyframeAnimationStrip *self, const animrig::binding_handle_t binding_handle)

"Add a binding

`"Add a binding `
dr.sybren marked this conversation as resolved
@ -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.

Empty line is wrongly located here, should be after next line, or even completely removed.
dr.sybren marked this conversation as resolved
@ -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 ?

`"Name of the layer, will be made unique within the ` ?
dr.sybren marked this conversation as resolved
@ -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)?

Is this description really accurate? I thought several IDs could share the same binding (at least in the future)?
Author
Member

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.

I thought several IDs could share the same binding (at least in the future)?

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.

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. > I thought several IDs could share the same binding (at least in the future)? 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.
dr.sybren marked this conversation as resolved
@ -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?

Again, as far as I understand the current design, this seems like over-simplified description of what is a binding?
dr.sybren marked this conversation as resolved
@ -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!

This property really needs a description!
dr.sybren marked this conversation as resolved
@ -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.

Also missing a proper description. Critical for API docs.
dr.sybren marked this conversation as resolved
@ -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)"

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)"`
dr.sybren marked this conversation as resolved
@ -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 to binding ?

A case of over-renaming of `output` to `binding` ?
dr.sybren marked this conversation as resolved
@ -0,0 +543,4 @@
static void rna_def_animation_layer(BlenderRNA *brna)
{
StructRNA *srna;
PropertyRNA *prop;

Another case of over-renaming of output to binding...

Another case of over-renaming of `output` to `binding`...
dr.sybren marked this conversation as resolved
@ -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), or FCurve. ;)

Should decide if official name is `F-Curve` (as used by RNA code below), or `FCurve`. ;)
dr.sybren marked this conversation as resolved
@ -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...

Name looks incorrectly put to plural here? Should be `rna_def_animation_channelbag` I think, since it's not defining a RNA collection type...
dr.sybren marked this conversation as resolved

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. :(

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. :(
Author
Member

Thanks for the review Bastien.

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. :(

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.

Thanks for the review Bastien. > 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. :( 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`.
Sybren A. Stüvel force-pushed pr/baklava-data-model from 050cf201ac to 32875e8b87 2024-04-02 17:41:27 +02:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 32875e8b87 to e9960842bb 2024-04-04 16:54:09 +02:00 Compare
Author
Member

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.

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.
Bastien Montagne requested changes 2024-04-05 04:19:31 +02:00
Dismissed
Bastien Montagne left a comment
Owner

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 :|

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...

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

an binding
Author
Member

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 :|

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.

> Still needs a proper search and replace on `an binding`, `an 'binding`, etc. > For the rest think things are OK :+1: > though am still unhappy about the hack in unbinding animation code :| 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.
Sybren A. Stüvel force-pushed pr/baklava-data-model from e9960842bb to 9bfc4f7619 2024-04-05 12:49:58 +02:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 9bfc4f7619 to faf425f60e 2024-04-05 12:54:59 +02:00 Compare
Author
Member

@blender-bot build

@blender-bot build

though am still unhappy about the hack in unbinding animation code :|

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.

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 ;)

> > though am still unhappy about the hack in unbinding animation code :| > > 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. 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 ;)
Bastien Montagne approved these changes 2024-04-08 02:13:56 +02:00
Sybren A. Stüvel force-pushed pr/baklava-data-model from faf425f60e to 614cc0b90e 2024-04-08 12:25:14 +02:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-data-model from 614cc0b90e to 178b0dccd0 2024-04-08 12:28:04 +02:00 Compare
Author
Member

The previous two force-pushes were, respectively, a rebase onto main and Bastien's comment suggestion.

The previous two force-pushes were, respectively, a rebase onto `main` and Bastien's comment suggestion.
Sybren A. Stüvel force-pushed pr/baklava-data-model from 178b0dccd0 to f328dc994a 2024-04-08 12:30:11 +02:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel manually merged commit a5d0ae9644 into main 2024-04-08 17:27:15 +02:00
Sign in to join this conversation.
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 Assignees
6 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#118677
No description provided.