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 +62 -16
2 changed files with 62 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f133504646 - Show all commits

1
source/blender/blenkernel/intern/curves.cc
View File

@ -154,7 +154,6 @@ static void curves_blend_read_data(BlendDataReader *reader, ID *id)
/* Geometry */
curves->geometry.wrap().blend_read(*reader);
BLO_read_data_address(reader, &curves->surface_uv_map);
curves->geometry.runtime = MEM_new<blender::bke::CurvesGeometryRuntime>(__func__);

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

@ -53,7 +53,7 @@ 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
/* TODO: walk all the referenced drawings */
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)
}
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.
static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *id_address)
@ -62,36 +62,41 @@ static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *i
/* TODO: Flush runtime data to storage. */
/* Write animation data. */
if (grease_pencil->adt) {
BKE_animdata_blend_write(writer, grease_pencil->adt);
}
/* Write LibData */
BLO_write_id_struct(writer, GreasePencil, id_address, &grease_pencil->id);
BKE_id_blend_write(writer, &grease_pencil->id);
/* Drawings. */
/* Write drawings. */
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
GreasePencilDrawingOrReference *drawing_or_ref = grease_pencil->drawing_array[i];
BLO_write_struct(writer, GreasePencilDrawingOrReference, drawing_or_ref);
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_raw(writer, sizeof(void *), drawing_reference->id_reference);
BLO_write_struct(writer, GreasePencilDrawingReference, drawing_reference);
break;
}
}
}
/* 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];
BLO_write_struct(writer, GreasePencilLayerTreeNode, node);
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);
@ -101,30 +106,72 @@ static void grease_pencil_blend_write(BlendWriter *writer, ID *id, const void *i
}
case GREASE_PENCIL_LAYER_TREE_GROUP: {
GreasePencilLayerTreeGroup *group = reinterpret_cast<GreasePencilLayerTreeGroup *>(node);
BLO_write_raw(writer, sizeof(int), &group->children_num);
BLO_write_struct(writer, GreasePencilLayerTreeGroup, group);
break;
}
}
}
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.
/* Materials. */
/* Write materials. */
BLO_write_pointer_array(
writer, grease_pencil->material_array_size, grease_pencil->material_array);
/* Animation data. */
if (grease_pencil->adt) {
BKE_animdata_blend_write(writer, grease_pencil->adt);
}
}
static void grease_pencil_blend_read_data(BlendDataReader *reader, ID *id)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
/* TODO: Convert storage data to runtime structs. */
/* Read animation data. */
BLO_read_data_address(reader, &grease_pencil->adt);
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.
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];
Bastien Montagne commented 2023-05-26 12:51:09 +02:00
Outdated
Review
Copy Link

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.
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;
}
Falk David commented 2023-04-12 10:45:04 +02:00
Outdated
Review
Copy Link

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

Needs to walk the parents of layers too (once added).
case GREASE_PENCIL_DRAWING_REFERENCE: {
GreasePencilDrawingReference *drawing_reference =
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_read_data_address(reader, &drawing_reference);
break;
}
}
}
Bastien Montagne commented 2023-05-06 19:06:50 +02:00
Outdated
Review
Copy Link

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.
/* 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++) {
filedescriptor marked this conversation as resolved Outdated
Bastien Montagne commented 2023-05-25 12:30:53 +02:00
Outdated
Review
Copy Link

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!
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);
Hans Goudey commented 2023-04-25 04:54:27 +02:00
Outdated
Review
Copy Link

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.
Hans Goudey commented 2023-05-02 23:45:36 +02:00
Outdated
Review
Copy Link

Maybe this comment was missed? Or do you disagree?

Maybe this comment was missed? Or do you disagree?
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;
}
}
}
/* Read materials. */
BLO_read_pointer_array(reader, (void **)&grease_pencil->material_array);
Hans Goudey commented 2023-05-03 16:26:06 +02:00
Outdated
Review
Copy Link

C++ cast here

C++ cast here
/* TODO: Convert storage data to runtime structs. */
}
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...
static void grease_pencil_blend_read_lib(BlendLibReader *reader, ID *id)