Initial Grease Pencil 3.0 stage #106848

Merged
Falk David merged 224 commits from filedescriptor/blender:grease-pencil-v3 into main 2023-05-30 11:14:22 +02:00
3 changed files with 358 additions and 88 deletions
Showing only changes of commit e31a79f8b7 - Show all commits

View File

@ -9,6 +9,7 @@
#include "BLI_function_ref.hh"
#include "BLI_map.hh"
#include "BLI_math_vector.h"
#include "BLI_stack.hh"
#include "BLI_string.h"
@ -40,10 +41,11 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
this->type = type;
this->name = BLI_strdup(name.c_str());
}
TreeNode(const TreeNode &other)
TreeNode(const TreeNode &other) : TreeNode(GreasePencilLayerTreeNodeType(other.type))
{
this->type = other.type;
this->name = BLI_strdup(other.name);
if (other.name) {
this->name = BLI_strdup(other.name);
}
children_.reserve(other.children_.size());
for (const std::unique_ptr<TreeNode> &elem : other.children_) {

Is defining these as constexpr helpful? I sort of doubt any real computation is done on these nodes at compile time. But maybe?

Is defining these as `constexpr` helpful? I sort of doubt any real computation is done on these nodes at compile time. But maybe?

The const in the const bool return type means nothing here

The `const` in the `const bool` return type means nothing here
children_.append(std::make_unique<TreeNode>(*elem));
@ -156,6 +158,23 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
LayerGroup &as_group();
Layer &as_layer();
int total_num_children() const
{
int total = 0;

Put out of line.

Put out of line.
Stack<TreeNode *> stack;
for (auto it = this->children_.rbegin(); it != this->children_.rend(); it++) {
stack.push((*it).get());

The function is "is_locked" and the comment says "return if it is locked". This sort of comment doesn't add anything IMO, just wastes space. If there is a comment, maybe it should use a different word besides "locked" to describe the state. Or there doesn't need to be a comment at all.

The function is "is_locked" and the comment says "return if it is locked". This sort of comment doesn't add anything IMO, just wastes space. If there is a comment, maybe it should use a different word besides "locked" to describe the state. Or there doesn't need to be a comment at all.
}
while (!stack.is_empty()) {
TreeNode &next_node = *stack.pop();
total++;
for (auto it = next_node.children_.rbegin(); it != next_node.children_.rend(); it++) {
stack.push((*it).get());
}
}
return total;
}
PreOrderRange children_in_pre_order()
{
return PreOrderRange(this);
@ -175,6 +194,16 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
}
}
void save_to_storage(GreasePencilLayerTreeNode *dst)
{
dst->type = this->type;
copy_v3_v3_uchar(dst->color, this->color);

Put out of line.

Put out of line.
dst->flag = this->flag;
if (this->name) {
dst->name = BLI_strdup(this->name);
}
}
private:
void foreach_children_pre_order_recursive_(TreeNodeIterFn function)

Put out of line.

Put out of line.
{
@ -222,6 +251,7 @@ class Layer : public TreeNode, ::GreasePencilLayer {
* drawings, then the last referenced drawing is held for the rest of the duration.
*/
Map<int, int> frames_;
Vector<int> sorted_keys_cache_;
public:
Layer() : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF), frames_()
@ -257,13 +287,57 @@ class Layer : public TreeNode, ::GreasePencilLayer {
bool insert_frame(int frame_number, int index)
{
sorted_keys_cache_.clear_and_shrink();
return frames_.add(frame_number, index);
}
bool overwrite_frame(int frame_number, int index)
{
sorted_keys_cache_.clear_and_shrink();

= {} doesn't change anything here since Vector has a default constructor. It can be removed I think. Same with below

`= {}` doesn't change anything here since `Vector` has a default constructor. It can be removed I think. Same with below
return frames_.add_overwrite(frame_number, index);

Put out of line.

Put out of line.
}
Span<int> sorted_keys()
{
if (sorted_keys_cache_.is_empty() && frames_.size() > 0) {
sorted_keys_cache_.reserve(frames_.size());

Looks like this should be private maybe? It has a _ suffix.

Looks like this should be private maybe? It has a `_` suffix.
for (int16_t key : frames_.keys()) {
sorted_keys_cache_.append(key);
}
std::sort(sorted_keys_cache_.begin(), sorted_keys_cache_.end());
}
return sorted_keys_cache_.as_span();
}

Put out of line.

Put out of line.
void save_to_storage(GreasePencilLayerTreeNode **dst)
{
GreasePencilLayerTreeLeaf *new_leaf = MEM_cnew<GreasePencilLayerTreeLeaf>(__func__);
/* Save properties. */
TreeNode::save_to_storage(&new_leaf->base);
/* Save frames map. */
int frames_size = frames_.size();
new_leaf->layer.frames_storage.size = frames_size;

Put out of line.

Put out of line.
new_leaf->layer.frames_storage.keys = MEM_cnew_array<int>(frames_size, __func__);
new_leaf->layer.frames_storage.values = MEM_cnew_array<int>(frames_size, __func__);
filedescriptor marked this conversation as resolved Outdated

Same remark as above about commented out code.

Same remark as above about commented out code.
MutableSpan<int> keys{new_leaf->layer.frames_storage.keys, frames_size};
MutableSpan<int> values{new_leaf->layer.frames_storage.values, frames_size};
keys.copy_from(sorted_keys());

These two functions should never have to be called outside of the ID callbacks in grease_pencil.cc; IMO they make more sense as static functions there. Probably better to keep that storage for DNA thing as localized as possible.

These two functions should never have to be called outside of the `ID` callbacks in `grease_pencil.cc`; IMO they make more sense as static functions there. Probably better to keep that storage for DNA thing as localized as possible.
for (int i : keys.index_range()) {
values[i] = frames_.lookup(keys[i]);
}
/* Store pointer. */

Member variables come before functions https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Class_Layout

Member variables come before functions https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Class_Layout
*dst = reinterpret_cast<GreasePencilLayerTreeNode *>(new_leaf);
}
void load_from_storage(GreasePencilLayer &node)
{
for (int i = 0; i < node.frames_storage.size; i++) {
this->frames_.add(node.frames_storage.keys[i], node.frames_storage.values[i]);
}
}
};
class LayerGroup : public TreeNode {
@ -326,6 +400,19 @@ class LayerGroup : public TreeNode {
BLI_assert(index >= 0 && index < children_.size());
children_.remove(index);
}
void save_to_storage(GreasePencilLayerTreeNode **dst)
{
GreasePencilLayerTreeGroup *new_group = MEM_cnew<GreasePencilLayerTreeGroup>(__func__);
/* Save properties. */
TreeNode::save_to_storage(&new_group->base);
/* Save number of children. */
new_group->children_num = total_num_children();
/* Store pointer. */
*dst = reinterpret_cast<GreasePencilLayerTreeNode *>(new_group);
}
};
namespace convert {
@ -347,6 +434,9 @@ class GreasePencilRuntime {
GreasePencilRuntime()
{
}
GreasePencilRuntime(const GreasePencilRuntime &other) : root_group_(other.root_group_)
{
}
LayerGroup &root_group()
{

View File

@ -15,6 +15,7 @@
#include "BLI_math_vector_types.hh"
#include "BLI_span.hh"
#include "BLI_stack.hh"
#include "BLO_read_write.h"
@ -58,8 +59,8 @@ static void grease_pencil_free_data(ID *id)
GreasePencil *grease_pencil = (GreasePencil *)id;
BKE_animdata_free(&grease_pencil->id, false);
// TODO: free drawing array
// TODO: free layer tree
grease_pencil->free_drawing_array();
grease_pencil->free_layer_tree_storage();
MEM_SAFE_FREE(grease_pencil->material_array);
@ -73,14 +74,23 @@ static void grease_pencil_foreach_id(ID *id, LibraryForeachIDData *data)
for (int i = 0; i < grease_pencil->material_array_size; i++) {
BKE_LIB_FOREACHID_PROCESS_IDSUPER(data, grease_pencil->material_array[i], IDWALK_CB_USER);
}
/* TODO: walk all the referenced drawings */
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = grease_pencil->drawing_array[i];
if (drawing_or_ref->type == GREASE_PENCIL_DRAWING_REFERENCE) {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BKE_LIB_FOREACHID_PROCESS_IDSUPER(data, drawing_reference->id_reference, IDWALK_CB_USER);
}
}
}
static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *id_address)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
/* TODO: Flush runtime data to storage. */
/* Free storage if there is one. */
grease_pencil->free_layer_tree_storage();
grease_pencil->save_layer_tree_to_storage();
/* Write animation data. */
if (grease_pencil->adt) {
@ -92,46 +102,9 @@ static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *i
BKE_id_blend_write(writer, &grease_pencil->id);
/* Write drawings. */
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = grease_pencil->drawing_array[i];
switch (drawing_or_ref->type) {
case GREASE_PENCIL_DRAWING: {
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_or_ref);
BLO_write_struct(writer, GreasePencilDrawing, drawing);
drawing->geometry.wrap().blend_write(*writer, grease_pencil->id);
break;
}
case GREASE_PENCIL_DRAWING_REFERENCE: {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_write_struct(writer, GreasePencilDrawingReference, drawing_reference);
break;
}
}
}
grease_pencil->write_drawing_array(writer);
/* Write layer tree. */
for (int i = 0; i < grease_pencil->layer_tree_storage.nodes_num; i++) {
GreasePencilLayerTreeNode *node = grease_pencil->layer_tree_storage.nodes[i];
switch (node->type) {
case GREASE_PENCIL_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
BLO_write_struct(writer, GreasePencilLayerTreeLeaf, node_leaf);
/* Write layer data. */
BLO_write_int32_array(
writer, node_leaf->layer.frames_storage.size, node_leaf->layer.frames_storage.keys);
BLO_write_int32_array(
writer, node_leaf->layer.frames_storage.size, node_leaf->layer.frames_storage.values);
break;
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);
BLO_write_struct(writer, GreasePencilLayerTreeGroup, group);
break;
}
}
}
grease_pencil->write_layer_tree_storage(writer);
/* Write materials. */
BLO_write_pointer_array(
writer, grease_pencil->material_array_size, grease_pencil->material_array);
@ -139,6 +112,7 @@ static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *i
static void grease_pencil_blend_read_data(BlendDataReader *reader, ID *id)
{
filedescriptor marked this conversation as resolved Outdated

This cannot be 'just' copied like that, would assign the same batch_cache value to both copies e.g.

In general runtime data should not need to be copied anyway.

This cannot be 'just' copied like that, would assign the same `batch_cache` value to both copies e.g. In general runtime data should not need to be copied anyway.
using namespace blender::bke::gpencil;
GreasePencil *grease_pencil = (GreasePencil *)id;
/* Read animation data. */
@ -146,56 +120,31 @@ static void grease_pencil_blend_read_data(BlendDataReader *reader, ID *id)
BKE_animdata_blend_read_data(reader, grease_pencil->adt);
/* Read drawings. */
BLO_read_pointer_array(reader, (void **)&grease_pencil->drawing_array);
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = grease_pencil->drawing_array[i];
switch (drawing_or_ref->type) {
case GREASE_PENCIL_DRAWING: {
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_or_ref);
BLO_read_data_address(reader, &drawing);
drawing->geometry.wrap().blend_read(*reader);
break;
}
case GREASE_PENCIL_DRAWING_REFERENCE: {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_read_data_address(reader, &drawing_reference);
break;
}
}
}
grease_pencil->read_drawing_array(reader);
/* Read layer tree. */
BLO_read_pointer_array(reader, (void **)&grease_pencil->layer_tree_storage.nodes);
for (int i = 0; i < grease_pencil->layer_tree_storage.nodes_num; i++) {
GreasePencilLayerTreeNode *node = grease_pencil->layer_tree_storage.nodes[i];
switch (node->type) {
case GREASE_PENCIL_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
BLO_read_data_address(reader, &node_leaf);
/* Read layer data. */
BLO_read_int32_array(
reader, node_leaf->layer.frames_storage.size, &node_leaf->layer.frames_storage.keys);
BLO_read_int32_array(
reader, node_leaf->layer.frames_storage.size, &node_leaf->layer.frames_storage.values);
break;
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);
BLO_read_data_address(reader, &group);
break;
}
}
}
grease_pencil->read_layer_tree_storage(reader);

These two lines could use MEM_SAFE_FREE macro instead.

These two lines could use `MEM_SAFE_FREE` macro instead.

MEM_SAFE_FREE does not replace MEM_delete, it replaces MEM_freeN, which is different.

`MEM_SAFE_FREE` does not replace `MEM_delete`, it replaces `MEM_freeN`, which is different.

I was under the impression that any memory allocated with MEM_new should use MEM_delete because we also need to run the destructor.

I was under the impression that any memory allocated with `MEM_new` should use `MEM_delete` because we also need to run the destructor.
/* Read materials. */
BLO_read_pointer_array(reader, (void **)&grease_pencil->material_array);
/* TODO: Convert storage data to runtime structs. */
grease_pencil->runtime = MEM_new<blender::bke::GreasePencilRuntime>(__func__);
grease_pencil->load_layer_tree_from_storage();

How can you be sure here that the potential memory allocated to the batch_cache is freed?

if memory in batch_cache is managed by drawing code, a comment in the runtime struct should specify it.

Would be better to have a proper destructor for these structs anyway imho, even if not really needed right now.

How can you be sure here that the potential memory allocated to the `batch_cache` is freed? if memory in `batch_cache` is managed by drawing code, a comment in the runtime struct should specify it. Would be better to have a proper destructor for these structs anyway imho, even if not really needed right now.
}
static void grease_pencil_blend_read_lib(BlendLibReader *reader, ID *id)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
for (int i = 0; i < grease_pencil->material_array_size; i++) {
BLO_read_id_address(reader, grease_pencil->id.lib, &grease_pencil->material_array[i]);

Needs to walk the parents of layers too (once added).

Needs to walk the parents of layers too (once added).
}
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = grease_pencil->drawing_array[i];
if (drawing_or_ref->type == GREASE_PENCIL_DRAWING_REFERENCE) {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_read_id_address(reader, grease_pencil->id.lib, &drawing_reference->id_reference);
}
}

This has one important implication that should be noted in comments imho: it is crucial to not free that storage until the whole Main has been written (otherwise it could lead to same memory being re-used several time for different data, which would break .blend files in unpredictable ways). AFAICT current code is correct on that regard, but this should still be clearly documented.

There is also a serious issue here with regard to memfile undo: Since this data is re-generated on every file write (i.e. also on every undo step storage), it will automatically always mark the GP ID as modified for all undo steps. Think this should be re-used as much as possible instead.

This has one important implication that should be noted in comments imho: it is crucial to not free that storage until the whole Main has been written (otherwise it could lead to same memory being re-used several time for different data, which would break .blend files in unpredictable ways). AFAICT current code is correct on that regard, but this should still be clearly documented. There is also a serious issue here with regard to memfile undo: Since this data is re-generated on every file write (i.e. also on every undo step storage), it will automatically always mark the GP ID as modified for _all_ undo steps. Think this should be re-used as much as possible instead.
}
static void grease_pencil_blend_read_expand(BlendExpander *expander, ID *id)
filedescriptor marked this conversation as resolved Outdated

EEEEEK! ID data itself should always be written first, before absolutely anything else!

EEEEEK! ID data itself should always be written first, before absolutely anything else!
@ -204,6 +153,14 @@ static void grease_pencil_blend_read_expand(BlendExpander *expander, ID *id)
for (int i = 0; i < grease_pencil->material_array_size; i++) {
BLO_expand(expander, grease_pencil->material_array[i]);
}
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = grease_pencil->drawing_array[i];
if (drawing_or_ref->type == GREASE_PENCIL_DRAWING_REFERENCE) {

Here the comment says "Write drawings" and the code says "write drawing array". That's basically the same thing-- the comment isn't really necessary. Same with the others here IMO.

Here the comment says "Write drawings" and the code says "write drawing array". That's basically the same thing-- the comment isn't really necessary. Same with the others here IMO.

Maybe this comment was missed? Or do you disagree?

Maybe this comment was missed? Or do you disagree?
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_expand(expander, drawing_reference->id_reference);
}
}
}
IDTypeInfo IDType_ID_GP = {
@ -321,3 +278,215 @@ void GreasePencil::foreach_visible_drawing(
}
});
}
void GreasePencil::read_drawing_array(BlendDataReader *reader)
{
BLO_read_pointer_array(reader, (void **)&this->drawing_array);
for (int i = 0; i < this->drawing_array_size; i++) {
BLO_read_data_address(reader, &this->drawing_array[i]);
GreasePencilDrawingOrReference *drawing_or_ref = this->drawing_array[i];
switch (drawing_or_ref->type) {
case GREASE_PENCIL_DRAWING: {
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_or_ref);
drawing->geometry.wrap().blend_read(*reader);
break;
}
case GREASE_PENCIL_DRAWING_REFERENCE: {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);

This should only consider the current frame.

This should only consider the current frame.
BLO_read_data_address(reader, &drawing_reference->id_reference);
break;
}
}
}
}
void GreasePencil::write_drawing_array(BlendWriter *writer)
filedescriptor marked this conversation as resolved Outdated

Make sure to not overwrite min and max but account any value already present. E.g. use math::min_max(...).

Make sure to not overwrite `min` and `max` but account any value already present. E.g. use `math::min_max(...)`.
{
BLO_write_pointer_array(writer, this->drawing_array_size, this->drawing_array);
for (int i = 0; i < this->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = this->drawing_array[i];
switch (drawing_or_ref->type) {
case GREASE_PENCIL_DRAWING: {
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_or_ref);
BLO_write_struct(writer, GreasePencilDrawing, drawing);
drawing->geometry.wrap().blend_write(*writer, this->id);
break;
}
case GREASE_PENCIL_DRAWING_REFERENCE: {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_write_struct(writer, GreasePencilDrawingReference, drawing_reference);
break;
}
}
}
}
void GreasePencil::free_drawing_array()
{
for (int i = 0; i < this->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = this->drawing_array[i];
switch (drawing_or_ref->type) {
case GREASE_PENCIL_DRAWING: {
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_or_ref);
drawing->geometry.wrap().~CurvesGeometry();
MEM_freeN(drawing);
break;
}
case GREASE_PENCIL_DRAWING_REFERENCE: {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
MEM_freeN(drawing_reference);
break;
}
}
}
MEM_freeN(this->drawing_array);
}
void GreasePencil::save_layer_tree_to_storage()
{
using namespace blender::bke::gpencil;
/* We always store the root group, so we have to add one here. */
int num_tree_nodes = this->runtime->root_group().total_num_children() + 1;
this->layer_tree_storage.nodes_num = num_tree_nodes;
this->layer_tree_storage.nodes = MEM_cnew_array<GreasePencilLayerTreeNode *>(num_tree_nodes,
__func__);
int i = 0;
this->runtime->root_group().save_to_storage(&this->layer_tree_storage.nodes[i++]);
for (TreeNode &node : this->runtime->root_group().children_in_pre_order()) {
GreasePencilLayerTreeNode **dst = &this->layer_tree_storage.nodes[i];
if (node.is_group()) {
LayerGroup &group = node.as_group();
group.save_to_storage(dst);
}
else if (node.is_layer()) {
Layer &layer = node.as_layer();
layer.save_to_storage(dst);
}
i++;
}
}
static void read_layer_node_recursive(blender::bke::gpencil::LayerGroup &current_group,
GreasePencilLayerTreeNode **nodes,
int index)
{
using namespace blender::bke::gpencil;
GreasePencilLayerTreeNode *node = nodes[index];
switch (node->type) {
case GREASE_PENCIL_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
current_group.add_layer(Layer(node_leaf->base.name));
break;
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);
LayerGroup new_group(group->base.name);
for (int i = 0; i < group->children_num; i++) {
read_layer_node_recursive(new_group, nodes, index + i + 1);
}
current_group.add_group(std::move(new_group));
break;
}
}
}
void GreasePencil::load_layer_tree_from_storage()
{
using namespace blender::bke::gpencil;
if (this->layer_tree_storage.nodes_num == 0 || !this->layer_tree_storage.nodes) {
return;
}
/* The first node should be the root group. */
BLI_assert(
reinterpret_cast<GreasePencilLayerTreeNode *>(this->layer_tree_storage.nodes[0])->type ==
GREASE_PENCIL_LAYER_TREE_GROUP);
read_layer_node_recursive(this->runtime->root_group(), this->layer_tree_storage.nodes, 1);
}
void GreasePencil::read_layer_tree_storage(BlendDataReader *reader)
{
BLO_read_pointer_array(reader, (void **)&this->layer_tree_storage.nodes);
for (int i = 0; i < this->layer_tree_storage.nodes_num; i++) {
BLO_read_data_address(reader, &this->layer_tree_storage.nodes[i]);

Since this is making a copy of the frame, the argument might as well be a const reference rather than a non-const one.

Since this is making a copy of the frame, the argument might as well be a const reference rather than a non-const one.
GreasePencilLayerTreeNode *node = this->layer_tree_storage.nodes[i];
switch (node->type) {
case GREASE_PENCIL_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
BLO_read_data_address(reader, &node_leaf->base.name);
/* Read layer data. */
BLO_read_int32_array(

Forget if we talked about this already, but without std::move in the function, there doesn't seem to be much of a point to this second override. Usually there is one for a const reference and one for an rvalue, I haven't seen a non-const reference override like this before.

Forget if we talked about this already, but without `std::move` in the function, there doesn't seem to be much of a point to this second override. Usually there is one for a const reference and one for an rvalue, I haven't seen a non-const reference override like this before.

Maybe @JacquesLucke can clarify here, but I believe sine add_overwrite has an implementation with an r-value reference that does the std::move I think this is fine?

Maybe @JacquesLucke can clarify here, but I believe sine `add_overwrite` has an implementation with an r-value reference that does the `std::move` I think this is fine?

I recommend just using a debugger to see if this calls the function you intend to call (the one with an rvalue reference). Haven't tried right now, but it will probably call the const reference version without std::move.

I recommend just using a debugger to see if this calls the function you intend to call (the one with an rvalue reference). Haven't tried right now, but it will probably call the const reference version without `std::move`.
reader, node_leaf->layer.frames_storage.size, &node_leaf->layer.frames_storage.keys);
BLO_read_int32_array(
reader, node_leaf->layer.frames_storage.size, &node_leaf->layer.frames_storage.values);
break;
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {

Any reason to call clear_and_shrink here? Might as well call reinitialize if not

Any reason to call `clear_and_shrink` here? Might as well call `reinitialize` if not
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);
BLO_read_data_address(reader, &group->base.name);
break;
}
}
}
}

.as_span() shouldn't be necessary here.

`.as_span()` shouldn't be necessary here.
void GreasePencil::write_layer_tree_storage(BlendWriter *writer)
{

int frame -> const int frame

`int frame` -> `const int frame`
BLO_write_pointer_array(
writer, this->layer_tree_storage.nodes_num, this->layer_tree_storage.nodes);
for (int i = 0; i < this->layer_tree_storage.nodes_num; i++) {
GreasePencilLayerTreeNode *node = this->layer_tree_storage.nodes[i];
switch (node->type) {
case GREASE_PENCIL_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
BLO_write_struct(writer, GreasePencilLayerTreeLeaf, node_leaf);
BLO_write_string(writer, node_leaf->base.name);
/* Write layer data. */
BLO_write_int32_array(
writer, node_leaf->layer.frames_storage.size, node_leaf->layer.frames_storage.keys);
BLO_write_int32_array(
writer, node_leaf->layer.frames_storage.size, node_leaf->layer.frames_storage.values);
break;
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);
BLO_write_struct(writer, GreasePencilLayerTreeGroup, group);
BLO_write_string(writer, group->base.name);
break;
}
}
}
}
void GreasePencil::free_layer_tree_storage()
{
if (this->layer_tree_storage.nodes_num == 0 || !this->layer_tree_storage.nodes) {
return;
}
for (int i = 0; i < this->layer_tree_storage.nodes_num; i++) {
GreasePencilLayerTreeNode *node = this->layer_tree_storage.nodes[i];
if (node->name) {
MEM_freeN(node->name);
}
switch (node->type) {
case GREASE_PENCIL_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
if (node_leaf->layer.frames_storage.size > 0) {
MEM_freeN(node_leaf->layer.frames_storage.keys);
MEM_freeN(node_leaf->layer.frames_storage.values);
}
MEM_freeN(node_leaf);
break;
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);

I believe it's standard to std::move from a T &&

Is there a need for overloads for T & and T &&? What's the benefit of that?

I believe it's standard to `std::move` from a `T &&` Is there a need for overloads for `T &` and `T &&`? What's the benefit of that?
MEM_freeN(group);
break;
}
}
}
MEM_freeN(this->layer_tree_storage.nodes);
}

View File

@ -30,6 +30,8 @@ extern "C" {
#endif
struct GreasePencil;
struct BlendDataReader;
struct BlendWriter;
typedef enum GreasePencilDrawingType {
GREASE_PENCIL_DRAWING = 0,
@ -162,10 +164,19 @@ typedef struct GreasePencil {
void foreach_visible_drawing(
int frame,
blender::FunctionRef<void(GreasePencilDrawing &, blender::bke::gpencil::Layer &)> function);
void read_drawing_array(BlendDataReader *reader);
void write_drawing_array(BlendWriter *writer);
void free_drawing_array();
#endif
/* Only used for storage in the .blend file. */
GreasePencilLayerTreeStorage layer_tree_storage;
#ifdef __cplusplus
void save_layer_tree_to_storage();
void load_layer_tree_from_storage();
void read_layer_tree_storage(BlendDataReader *reader);
void write_layer_tree_storage(BlendWriter *writer);
void free_layer_tree_storage();
#endif
/**
* An array of materials.
*/