blender
/
blender
133
398
Fork 579
Code Issues 6.3k Pull Requests 747 Projects 19 Wiki Activity

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
Conversation 8 Commits 224 Files Changed 94 +32 -48
2 changed files with 32 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1d385c9d0f - Show all commits

16
source/blender/blenkernel/BKE_grease_pencil.hh
View File

@ -47,7 +47,7 @@ class TreeNode : public ::GreasePencilLayerTreeNode, NonMovable {
/**
* \returns true if this node is a LayerGroup.
*/
const bool is_group() const
bool is_group() const
Hans Goudey commented 2023-05-02 23:30:37 +02:00
Outdated
Review
Copy Link

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?
Hans Goudey commented 2023-05-03 16:08:15 +02:00
Outdated
Review
Copy Link

The const in the const bool return type means nothing here

The `const` in the `const bool` return type means nothing here
{
return this->type == GP_LAYER_TREE_GROUP;
}
@ -55,7 +55,7 @@ class TreeNode : public ::GreasePencilLayerTreeNode, NonMovable {
/**
* \returns true if this node is a Layer.
*/
const bool is_layer() const
bool is_layer() const
{
return this->type == GP_LAYER_TREE_LEAF;
}
@ -153,23 +153,17 @@ class Layer : public TreeNode, public ::GreasePencilLayer {
/**
* \returns the layer masks.
*/
const Vector<LayerMask> &masks() const;
Span<LayerMask> masks() const;
Hans Goudey commented 2023-05-03 16:10:32 +02:00
Outdated
Review
Copy Link

Better to return a span here maybe?

Better to return a span here maybe?
Vector<LayerMask> &masks_for_write();
/**
* \return true if the layer is visible.
*/
bool is_visible() const;
/**
* \return true if the layer is locked.
*/
bool is_locked() const;
/**
* Inserts the frame into the layer. Fails if there exists a frame at \a frame_number already.
Falk David commented 2023-04-12 10:39:55 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
* \returns true on success.
*/
bool insert_frame(int frame_number, GreasePencilFrame &frame);
bool insert_frame(int frame_number, const GreasePencilFrame &frame);
Hans Goudey commented 2023-05-03 16:11:22 +02:00
Outdated
Review
Copy Link

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.
bool insert_frame(int frame_number, GreasePencilFrame &&frame);
/**
@ -177,7 +171,7 @@ class Layer : public TreeNode, public ::GreasePencilLayer {
* overwritten.
* \returns true on success.
*/
bool overwrite_frame(int frame_number, GreasePencilFrame &frame);
bool overwrite_frame(int frame_number, const GreasePencilFrame &frame);
bool overwrite_frame(int frame_number, GreasePencilFrame &&frame);
/**

64
source/blender/blenkernel/intern/grease_pencil.cc
View File

@ -43,7 +43,7 @@ static void grease_pencil_init_data(ID *id)
{
using namespace blender::bke;
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:44:15 +02:00
Outdated
Review
Copy Link

Remove printfs.

Remove printfs.
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
grease_pencil->runtime = MEM_new<GreasePencilRuntime>(__func__);
}
@ -54,8 +54,8 @@ static void grease_pencil_copy_data(Main * /*bmain*/,
{
using namespace blender;
Hans Goudey commented 2023-05-03 16:26:34 +02:00
Outdated
Review
Copy Link

C++ cast here (and above, I'll stop writing it now)

C++ cast here (and above, I'll stop writing it now)
GreasePencil *grease_pencil_dst = (GreasePencil *)id_dst;
const GreasePencil *grease_pencil_src = (GreasePencil *)id_src;
GreasePencil *grease_pencil_dst = reinterpret_cast<GreasePencil *>(id_dst);
Hans Goudey commented 2023-05-02 23:44:51 +02:00
Outdated
Review
Copy Link

Personally I find grease_pencil_dst a longer name than it needs to be, compared to something like gp_dst, where it's easier to see the logic when reading the function IMO. I understand that's subjective though

Personally I find `grease_pencil_dst` a longer name than it needs to be, compared to something like `gp_dst`, where it's easier to see the logic when reading the function IMO. I understand that's subjective though
Falk David commented 2023-05-03 10:55:44 +02:00
Outdated
Review
Copy Link

This used to be the case in the old grease pencil code, but tbh I find it better when it's more explicit. We ended up with gpf,gpd,gps etc. and it's really unreadable imo.

This used to be the case in the old grease pencil code, but tbh I find it better when it's more explicit. We ended up with `gpf`,`gpd`,`gps` etc. and it's really unreadable imo.
const GreasePencil *grease_pencil_src = reinterpret_cast<const GreasePencil *>(id_src);
/* Duplicate material array. */
grease_pencil_dst->material_array = static_cast<Material **>(
@ -108,7 +108,7 @@ static void grease_pencil_copy_data(Main * /*bmain*/,
static void grease_pencil_free_data(ID *id)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
BKE_animdata_free(&grease_pencil->id, false);
MEM_SAFE_FREE(grease_pencil->material_array);
filedescriptor marked this conversation as resolved Outdated
Bastien Montagne commented 2023-05-26 12:50:28 +02:00
Outdated
Review
Copy Link

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.
@ -124,7 +124,7 @@ static void grease_pencil_free_data(ID *id)
static void grease_pencil_foreach_id(ID *id, LibraryForeachIDData *data)
Bastien Montagne commented 2023-05-25 12:26:10 +02:00
Outdated
Review
Copy Link

These two lines could use MEM_SAFE_FREE macro instead.

These two lines could use `MEM_SAFE_FREE` macro instead.
Hans Goudey commented 2023-05-25 18:50:26 +02:00
Outdated
Review
Copy Link

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.
Falk David commented 2023-05-26 10:19:32 +02:00
Outdated
Review
Copy Link

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.
{
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
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);
}
@ -140,7 +140,7 @@ static void grease_pencil_foreach_id(ID *id, LibraryForeachIDData *data)
static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *id_address)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
/* Free storage if there is one. */
grease_pencil->free_layer_tree_storage();
@ -169,7 +169,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)
{
using namespace blender::bke::greasepencil;
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
Hans Goudey commented 2023-05-03 16:26:06 +02:00
Outdated
Review
Copy Link

C++ cast here

C++ cast here
/* Read animation data. */
BLO_read_data_address(reader, &grease_pencil->adt);
filedescriptor marked this conversation as resolved Outdated
Bastien Montagne commented 2023-05-25 14:48:41 +02:00
Outdated
Review
Copy Link

Code is missing re-reading active_layer address...

Code is missing re-reading `active_layer` address...
@ -180,7 +180,7 @@ static void grease_pencil_blend_read_data(BlendDataReader *reader, ID *id)
/* Read layer tree. */
grease_pencil->read_layer_tree_storage(reader);
/* Read materials. */
BLO_read_pointer_array(reader, (void **)&grease_pencil->material_array);
BLO_read_pointer_array(reader, reinterpret_cast<void **>(&grease_pencil->material_array));
grease_pencil->runtime = MEM_new<blender::bke::GreasePencilRuntime>(__func__);
grease_pencil->load_layer_tree_from_storage();
@ -190,7 +190,7 @@ static void grease_pencil_blend_read_data(BlendDataReader *reader, ID *id)
static void grease_pencil_blend_read_lib(BlendLibReader *reader, ID *id)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
Hans Goudey commented 2023-05-03 16:25:55 +02:00
Outdated
Review
Copy Link

C++ cast here

C++ cast here
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]);
}
@ -206,7 +206,7 @@ static void grease_pencil_blend_read_lib(BlendLibReader *reader, ID *id)
static void grease_pencil_blend_read_expand(BlendExpander *expander, ID *id)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(id);
for (int i = 0; i < grease_pencil->material_array_size; i++) {
BLO_expand(expander, grease_pencil->material_array[i]);
}
@ -381,9 +381,9 @@ Map<int, GreasePencilFrame> &Layer::frames_for_write()
return this->frames_;
}
const Vector<LayerMask> &Layer::masks() const
Span<LayerMask> Layer::masks() const
{
return this->masks_;
return this->masks_.as_span();
}
Vector<LayerMask> &Layer::masks_for_write()
@ -401,7 +401,7 @@ bool Layer::is_locked() const
return (this->flag & GP_LAYER_TREE_NODE_LOCKED) != 0;
}
bool Layer::insert_frame(int frame_number, GreasePencilFrame &frame)
bool Layer::insert_frame(int frame_number, const GreasePencilFrame &frame)
{
return this->frames_for_write().add(frame_number, frame);
}
@ -411,7 +411,7 @@ bool Layer::insert_frame(int frame_number, GreasePencilFrame &&frame)
return this->frames_for_write().add(frame_number, frame);
}
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &frame)
bool Layer::overwrite_frame(int frame_number, const GreasePencilFrame &frame)
Hans Goudey commented 2023-05-03 16:13:17 +02:00
Outdated
Review
Copy Link

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.
{
return this->frames_for_write().add_overwrite(frame_number, frame);
}
@ -675,14 +675,14 @@ int GreasePencilRuntime::active_layer_index() const
void *BKE_grease_pencil_add(Main *bmain, const char *name)
{
GreasePencil *grease_pencil = static_cast<GreasePencil *>(BKE_id_new(bmain, ID_GP, name));
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(BKE_id_new(bmain, ID_GP, name));
return grease_pencil;
}
GreasePencil *BKE_grease_pencil_new_nomain()
{
GreasePencil *grease_pencil = static_cast<GreasePencil *>(BKE_id_new_nomain(ID_GP, nullptr));
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(BKE_id_new_nomain(ID_GP, nullptr));
grease_pencil_init_data(&grease_pencil->id);
return grease_pencil;
Bastien Montagne commented 2023-05-06 19:10:49 +02:00
Outdated
Review
Copy Link

This should not be needed, BKE_id_new_nomain already calls BKE_libblock_init_empty which calls the relevant initi_data callback.

This should not be needed, `BKE_id_new_nomain` already calls `BKE_libblock_init_empty` which calls the relevant `initi_data` callback.
}
@ -720,14 +720,14 @@ void BKE_grease_pencil_data_update(struct Depsgraph * /*depsgraph*/,
/* Free any evaluated data and restore original data. */
BKE_object_free_derived_caches(object);
GreasePencil *grease_pencil = static_cast<GreasePencil *>(object->data);
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(object->data);
/* Evaluate modifiers. */
/* TODO: evaluate modifiers. */
/* Assign evaluated object. */
/* TODO: Get eval from modifiers geometry set. */
GreasePencil *grease_pencil_eval = (GreasePencil *)BKE_id_copy_ex(
nullptr, &grease_pencil->id, nullptr, LIB_ID_COPY_LOCALIZE);
GreasePencil *grease_pencil_eval = reinterpret_cast<GreasePencil *>(
BKE_id_copy_ex(nullptr, &grease_pencil->id, nullptr, LIB_ID_COPY_LOCALIZE));
BKE_object_eval_assign_data(object, &grease_pencil_eval->id, true);
}
@ -1052,7 +1052,7 @@ void GreasePencil::print_layer_tree()
void GreasePencil::read_drawing_array(BlendDataReader *reader)
{
BLO_read_pointer_array(reader, (void **)&this->drawing_array);
BLO_read_pointer_array(reader, reinterpret_cast<void **>(&this->drawing_array));
for (int i = 0; i < this->drawing_array_size; i++) {
BLO_read_data_address(reader, &this->drawing_array[i]);
GreasePencilDrawingBase *drawing_base = this->drawing_array[i];
@ -1334,7 +1334,7 @@ void GreasePencil::load_layer_tree_from_storage()
void GreasePencil::read_layer_tree_storage(BlendDataReader *reader)
{
BLO_read_pointer_array(reader, (void **)&this->layer_tree_storage.nodes);
BLO_read_pointer_array(reader, reinterpret_cast<void **>(&this->layer_tree_storage.nodes));
Hans Goudey commented 2023-05-03 16:24:21 +02:00
Outdated
Review
Copy Link

Use reinterpret_cast over C style cast

Use `reinterpret_cast` over C style cast
for (int i = 0; i < this->layer_tree_storage.nodes_num; i++) {
BLO_read_data_address(reader, &this->layer_tree_storage.nodes[i]);
GreasePencilLayerTreeNode *node = this->layer_tree_storage.nodes[i];
@ -1415,9 +1415,7 @@ void GreasePencil::free_layer_tree_storage()
}
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);
}
MEM_SAFE_FREE(node->name);
Hans Goudey commented 2023-05-03 16:24:42 +02:00
Outdated
Review
Copy Link

MEM_SAFE_FREE(node->name)
Same with below where there are similar null checks

`MEM_SAFE_FREE(node->name)` Same with below where there are similar null checks
switch (node->type) {
case GP_LAYER_TREE_LEAF: {
GreasePencilLayerTreeLeaf *node_leaf = reinterpret_cast<GreasePencilLayerTreeLeaf *>(node);
@ -1425,24 +1423,16 @@ void GreasePencil::free_layer_tree_storage()
MEM_freeN(node_leaf->layer.frames_storage.keys);
MEM_freeN(node_leaf->layer.frames_storage.values);
}
if (node_leaf->layer.parsubstr) {
MEM_freeN(node_leaf->layer.parsubstr);
}
if (node_leaf->layer.viewlayer_name) {
MEM_freeN(node_leaf->layer.viewlayer_name);
}
MEM_SAFE_FREE(node_leaf->layer.parsubstr);
MEM_SAFE_FREE(node_leaf->layer.viewlayer_name);
GreasePencilLayerMaskStorage *masks_storage = &node_leaf->layer.masks_storage;
for (int mask_i = 0; mask_i < masks_storage->masks_size; mask_i++) {
GreasePencilLayerMask &mask = masks_storage->masks[mask_i];
if (mask.layer_name) {
MEM_freeN(mask.layer_name);
}
}
if (masks_storage->masks) {
MEM_freeN(masks_storage->masks);
MEM_SAFE_FREE(mask.layer_name);
}
MEM_SAFE_FREE(masks_storage->masks);
MEM_freeN(node_leaf);
break;
break;
}
case GP_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);