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
Member

Adds the initial stage for the grease pencil 3.0 project.

This patch includes:

  • New ID and new object type.
  • New DNA structures.
  • New drawing engine for grease pencil.
  • Object conversion from the old grease pencil to the new one.
  • A new (very basic) drawing operator.

Exposed to the user:

  • An experimental option to switch to the new grease pencil.
    • Changing this option requires a restart of blender currently.
  • A conversion setting in the Object > Convert menu.
  • A drawing operator in Draw Mode.
Adds the initial stage for the grease pencil 3.0 project. This patch includes: * New ID and new object type. * New DNA structures. * New drawing engine for grease pencil. * Object conversion from the old grease pencil to the new one. * A new (very basic) drawing operator. Exposed to the user: * An experimental option to switch to the new grease pencil. * Changing this option requires a restart of blender currently. * A conversion setting in the `Object` > `Convert` menu. * A drawing operator in `Draw Mode`.
Falk David added 111 commits 2023-04-12 10:35:39 +02:00
This renames the `BKE_gpencil_*` as well as the `DNA_gpencil_types.h`
files to indicate that it's the legacy grease pencil.
Merge branch 'main' into refactor-gpencil-rename-files-legacy
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
db99c202b4
Falk David added this to the (deleted) project 2023-04-12 10:35:49 +02:00
Falk David reviewed 2023-04-12 11:09:31 +02:00
Falk David left a comment
Author
Member

Some comments for myself reading through the entire thing.

Some comments for myself reading through the entire thing.
@ -0,0 +1,28 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
Author
Member

Needs copyright.

Needs copyright.
filedescriptor marked this conversation as resolved
@ -0,0 +1,480 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
Author
Member

Needs copyright.

Needs copyright.
filedescriptor marked this conversation as resolved
@ -0,0 +20,4 @@
namespace blender::bke {
namespace gpencil {
Author
Member

Decide on the namespace here. Should it maybe be just gp?

Decide on the namespace here. Should it maybe be just `gp`?
filedescriptor marked this conversation as resolved
@ -0,0 +70,4 @@
}
public:
class PreOrderRange {
Author
Member

Try putting this class out of line.

Try putting this class out of line.
@ -0,0 +160,4 @@
LayerGroup &as_group();
Layer &as_layer();
int total_num_children() const
Author
Member

Put out of line.

Put out of line.
@ -0,0 +197,4 @@
}
private:
void foreach_children_pre_order_recursive_(TreeNodeIterFn function)
Author
Member

Put out of line.

Put out of line.
@ -0,0 +205,4 @@
}
}
void foreach_layer_pre_order_recursive_(LayerIterFn function)
Author
Member

Put out of line.

Put out of line.
@ -0,0 +246,4 @@
* referenced drawings are discarded. If the frame is longer than the number of referenced
* drawings, then the last referenced drawing is held for the rest of the duration.
*/
Map<int, int> frames_;
Author
Member

Since the map will have to change to store more than just an index as the value, make sure to update the comment too.

Since the map will have to change to store more than just an index as the value, make sure to update the comment too.
filedescriptor marked this conversation as resolved
@ -0,0 +294,4 @@
return frames_for_write().add_overwrite(frame_number, index);
}
Span<int> sorted_keys()
Author
Member

Put out of line.

Put out of line.
@ -0,0 +307,4 @@
return sorted_keys_cache_.data().as_span();
}
void load_from_storage(GreasePencilLayer &node)
Author
Member

Put out of line.

Put out of line.
@ -0,0 +317,4 @@
/**
* Return the index of the drawing at frame \a frame or -1 if there is no drawing.
*/
int drawing_at(int frame)
Author
Member

Put out of line.

Put out of line.
@ -0,0 +344,4 @@
public:
LayerGroup() : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP) {}
LayerGroup(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, name) {}
LayerGroup(const LayerGroup &other) : TreeNode(other)
Author
Member

Put out of line.

Put out of line.
@ -0,0 +356,4 @@
}
}
}
LayerGroup(LayerGroup &&other) : TreeNode(std::move(other))
Author
Member

This needs to be implemented.

This needs to be implemented.
@ -0,0 +461,4 @@
}
public:
void *batch_cache = nullptr;
Author
Member

Could this be private?

Could this be private?
@ -200,6 +200,7 @@ typedef struct Main {
ListBase pointclouds;
ListBase volumes;
ListBase simulations;
ListBase greasepencils;
Author
Member

Maybe grease_pencils?

Maybe `grease_pencils`?
filedescriptor marked this conversation as resolved
@ -1580,6 +1580,7 @@ GVArray CurvesGeometry::adapt_domain(const GVArray &varray,
/** \name File reading/writing.
* \{ */
Author
Member

Remove blank line.

Remove blank line.
filedescriptor marked this conversation as resolved
@ -0,0 +41,4 @@
{
using namespace blender::bke;
// printf("grease_pencil_init_data\n");
Author
Member

Remove printfs.

Remove printfs.
filedescriptor marked this conversation as resolved
@ -0,0 +135,4 @@
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BKE_LIB_FOREACHID_PROCESS_IDSUPER(data, drawing_reference->id_reference, IDWALK_CB_USER);
}
}
Author
Member

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

Needs to walk the parents of layers too (once added).
@ -0,0 +201,4 @@
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_read_id_address(reader, grease_pencil->id.lib, &drawing_reference->id_reference);
}
}
Author
Member

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

Needs to walk the parents of layers too (once added).
@ -0,0 +217,4 @@
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_expand(expander, drawing_reference->id_reference);
}
}
Author
Member

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

Needs to walk the parents of layers too (once added).
@ -0,0 +293,4 @@
float3 min(FLT_MAX);
float3 max(-FLT_MAX);
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
Author
Member

This should only consider the current frame.

This should only consider the current frame.
@ -0,0 +301,4 @@
const blender::bke::CurvesGeometry &curves = drawing->geometry.wrap();
if (!curves.bounds_min_max(min, max)) {
min = float3(-1);
Author
Member

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(...)`.
filedescriptor marked this conversation as resolved
@ -0,0 +16,4 @@
namespace blender::bke::gpencil::convert {
void legacy_gpencil_frame_to_curves_geometry(GreasePencilDrawing &drawing, bGPDframe &gpf)
Author
Member

legacy_gpencil_frame_to_grease_pencil_drawing

`legacy_gpencil_frame_to_grease_pencil_drawing`
filedescriptor marked this conversation as resolved
@ -0,0 +38,4 @@
EXPECT_EQ(grease_pencil.drawings().size(), 0);
EXPECT_EQ(grease_pencil.root_group().total_num_children(), 0);
}
Author
Member

Add tests for load_layer_tree_from_storage and save_layer_tree_to_storage.

Add tests for `load_layer_tree_from_storage` and `save_layer_tree_to_storage`.
filedescriptor marked this conversation as resolved
@ -3936,7 +3950,6 @@ void BKE_object_minmax(Object *ob, float r_min[3], float r_max[3], const bool us
changed = true;
break;
}
Author
Member

Fix line

Fix line
filedescriptor marked this conversation as resolved
@ -315,2 +320,4 @@
BKE_volume_batch_cache_dirty_tag((struct Volume *)ob->data, BKE_VOLUME_BATCH_DIRTY_ALL);
break;
case OB_GREASE_PENCIL:
/* TODO: tag batches. */
Author
Member

Remove todo.

Remove todo.
filedescriptor marked this conversation as resolved
@ -2514,2 +2516,4 @@
break;
}
case ID_GP:
break;
Author
Member

This needs to build the relations for the the parent of layers.

This needs to build the relations for the the parent of layers.
@ -0,0 +31,4 @@
/** Instancing Data */
GPUVertBuf *vbo;
GPUVertBuf *vbo_col;
/** Indices in material order, then stroke order with fill first.
Author
Member

Update this comment. Out of date.

Update this comment. Out of date.
filedescriptor marked this conversation as resolved
@ -0,0 +38,4 @@
/** Batches */
GPUBatch *geom_batch;
/** Stroke lines only */
GPUBatch *lines_batch;
Author
Member

Remove while unused?

Remove while unused?
filedescriptor marked this conversation as resolved
@ -0,0 +66,4 @@
* The stroke data for this drawing.
*/
CurvesGeometry geometry;
#ifdef __cplusplus
Author
Member

Move below runtime data pointer.

Move below runtime data pointer.
filedescriptor marked this conversation as resolved
@ -6525,1 +6525,4 @@
prop = RNA_def_property(srna, "use_grease_pencil_version3", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, NULL, "use_grease_pencil_version3", 1);
RNA_def_property_ui_text(prop, "Grease Pencil 3.0", "Enable the new grease pencil 3.0 codebase");
Author
Member

Enable the new Grease Pencil 3.0 object ?

`Enable the new Grease Pencil 3.0 object` ?
filedescriptor marked this conversation as resolved
Falk David added 2 commits 2023-04-12 12:02:05 +02:00
Add GreasePencilFrame in the layer map
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
0b9a5381f9
This struct now holds more information than just the index into the
drawing array. In the future, we can have more fields if we want to.
Author
Member

@blender-bot build

@blender-bot build
Falk David added 1 commit 2023-04-12 14:43:24 +02:00
Falk David added 1 commit 2023-04-12 16:14:51 +02:00
Falk David added 1 commit 2023-04-12 16:41:47 +02:00
Falk David added 1 commit 2023-04-12 16:55:12 +02:00
Falk David added 2 commits 2023-04-13 13:33:26 +02:00
Falk David added 1 commit 2023-04-13 14:52:30 +02:00
Falk David added 3 commits 2023-04-13 15:54:46 +02:00
Falk David added 3 commits 2023-04-14 11:49:29 +02:00
* Move functions out of line.
* Remove custom iterators
* Implement a cache of the flattend layer tree
Falk David added 1 commit 2023-04-14 12:11:29 +02:00
Falk David added 1 commit 2023-04-14 13:40:59 +02:00
Falk David added 2 commits 2023-04-14 17:48:24 +02:00
Falk David added 3 commits 2023-04-17 12:44:20 +02:00
Falk David added 1 commit 2023-04-17 12:48:14 +02:00
Falk David added 1 commit 2023-04-17 13:21:28 +02:00
Falk David added 1 commit 2023-04-17 13:24:57 +02:00
Falk David added 1 commit 2023-04-17 13:32:58 +02:00
Falk David added 1 commit 2023-04-17 14:10:19 +02:00
Falk David added 1 commit 2023-04-17 17:37:35 +02:00
Falk David added 1 commit 2023-04-17 18:51:33 +02:00
Falk David added 1 commit 2023-04-17 19:01:56 +02:00
Falk David added 3 commits 2023-04-18 12:30:55 +02:00
Falk David added 2 commits 2023-04-18 13:33:38 +02:00
Falk David added 1 commit 2023-04-18 13:37:13 +02:00
Falk David reviewed 2023-04-18 16:10:24 +02:00
@ -0,0 +252,4 @@
*/
float tint_color[4];
/**
* Layer transfrom.
Author
Member

typo transform

typo `transform`
Falk David added 1 commit 2023-04-24 10:59:03 +02:00
Falk David added 1 commit 2023-04-24 13:22:01 +02:00
Falk David added 5 commits 2023-04-24 15:55:50 +02:00
Falk David added 1 commit 2023-04-24 15:59:56 +02:00
Falk David added 1 commit 2023-04-24 17:19:20 +02:00
Falk David added 4 commits 2023-04-24 18:43:05 +02:00
When the experimental option was turned off, old grease pencil
objects would no longer render.
Make draw keymap dependent of experimental setting
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
b2f8f066c0
Falk David changed title from WIP: Initial Grease Pencil 3.0 stage to Initial Grease Pencil 3.0 stage 2023-04-24 18:43:24 +02:00
Falk David requested review from Brecht Van Lommel 2023-04-24 18:44:38 +02:00
Falk David requested review from Sergey Sharybin 2023-04-24 18:44:38 +02:00
Falk David requested review from Bastien Montagne 2023-04-24 18:44:38 +02:00
Falk David requested review from Antonio Vazquez 2023-04-24 18:44:38 +02:00
Falk David requested review from Hans Goudey 2023-04-24 18:44:38 +02:00
Falk David added 1 commit 2023-04-24 19:26:23 +02:00
Fix keymap test failing
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
228d817743
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2023-04-25 05:19:43 +02:00
@ -0,0 +219,4 @@
* Adds a layer at the end of this group and returns it.
*/
Layer &add_layer(Layer &layer);
Layer &add_layer(Layer &&layer);
Member

Returning an element added to a vector by reference is quite dangerous since they're invalidated by further additions, we usually don't do that. Either returning an index or not having these wrapper functions seems like the best choices IMO

Returning an element added to a vector by reference is quite dangerous since they're invalidated by further additions, we usually don't do that. Either returning an index or not having these wrapper functions seems like the best choices IMO

From quick look it seems that it should actually be emplace_layer() or something like this.
And for that it is typically very handy to return reference. And not only that, since C++17 it is expected behavior for vector type containers as well.

From quick look it seems that it should actually be `emplace_layer()` or something like this. And for that it is typically very handy to return reference. And not only that, since C++17 it is expected behavior for vector type containers as well.
Author
Member

I think emplace_layer would be a better name indeed.

I think `emplace_layer` would be a better name indeed.
@ -0,0 +300,4 @@
class GreasePencilRuntime {
public:
mutable SharedCache<Vector<greasepencil::Layer *>> layer_cache_;
Member

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

Looks like this should be private maybe? It has a `_` suffix.
@ -0,0 +323,4 @@
void ensure_layer_cache() const;
void load_layer_tree_from_storage(GreasePencilLayerTreeStorage &storage);
void save_layer_tree_to_storage(GreasePencilLayerTreeStorage &storage);
Member

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.
@ -0,0 +328,4 @@
void tag_layer_tree_topology_changed();
public:
void *batch_cache = nullptr;
Member

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
@ -0,0 +155,4 @@
BLO_write_id_struct(writer, GreasePencil, id_address, &grease_pencil->id);
BKE_id_blend_write(writer, &grease_pencil->id);
/* Write drawings. */
Member

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

Maybe this comment was missed? Or do you disagree?

Maybe this comment was missed? Or do you disagree?
@ -0,0 +424,4 @@
Span<int> Layer::sorted_keys() const
{
this->sorted_keys_cache_.ensure([&](Vector<int> &r_data) {
r_data.clear_and_shrink();
Member

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
@ -0,0 +431,4 @@
}
std::sort(r_data.begin(), r_data.end());
});
return this->sorted_keys_cache_.data().as_span();
Member

.as_span() shouldn't be necessary here.

`.as_span()` shouldn't be necessary here.
@ -0,0 +482,4 @@
void LayerGroup::add_group(LayerGroup &&group)
{
this->children.append(std::make_unique<LayerGroup>(group));
Member

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?
@ -0,0 +594,4 @@
GreasePencilRuntime::GreasePencilRuntime(const GreasePencilRuntime &other)
: root_group_(other.root_group_), active_layer_index_(other.active_layer_index_)
{
/* We cannot copy this cache, because the pointers in that cache become invalid. */
Member

It probably shouldn't be a SharedCache then. Either that or it should cache indices rather than pointers?

It probably shouldn't be a `SharedCache` then. Either that or it should cache indices rather than pointers?
Author
Member

Ah yes, I don't see a way it can be a shared cache then.

Ah yes, I don't see a way it can be a shared cache then.
@ -0,0 +956,4 @@
{
using namespace blender;
const bke::GreasePencilDrawingRuntime &runtime = *this->runtime;
runtime.triangles_cache.ensure([&](Vector<uint3> &cache) {
Member

cache is named r_data in other shared cache "ensure" functions, might as well be consistent with that

`cache` is named `r_data` in other shared cache "ensure" functions, might as well be consistent with that
@ -0,0 +964,4 @@
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
int total_triangles = 0;
for (int curve_i : curves.curves_range()) {
Member

int curve_i -> const int curve_i

`int curve_i` -> `const int curve_i`
@ -0,0 +965,4 @@
int total_triangles = 0;
for (int curve_i : curves.curves_range()) {
IndexRange points = points_by_curve[curve_i];
Member

IndexRange points -> const IndexRange points

`IndexRange points` -> `const IndexRange points`
@ -0,0 +984,4 @@
const int num_trinagles = points.size() - 2;
uint(*tris)[3] = static_cast<uint(*)[3]>(
BLI_memarena_alloc(pf_arena, sizeof(*tris) * size_t(num_trinagles)));
Member

Pass a slice of the result vector to BLI_polyfill_calc_arena directly rather than allocating a temporary array here

Pass a slice of the result vector to `BLI_polyfill_calc_arena` directly rather than allocating a temporary array here
@ -0,0 +1034,4 @@
/** \name Grease Pencil data-block API
* \{ */
static void grease_pencil_grow_drawing_array_by(GreasePencil &self, const int add_capacity)
Member

What about replacing these with slightly more generic functions like this?

template<typename T> static void grow_array(T **array,  int *size, const int add_size) {}
template<typename T> static void shring_array(T **array,  int *size, const int shrink_size) {}

That would allow for simpler variable naming inside, and would tell the reader "there's nothing special going on here, just resizing some arrays" which makes it easier to skim

What about replacing these with slightly more generic functions like this? ``` template<typename T> static void grow_array(T **array, int *size, const int add_size) {} template<typename T> static void shring_array(T **array, int *size, const int shrink_size) {} ``` That would allow for simpler variable naming inside, and would tell the reader "there's nothing special going on here, just resizing some arrays" which makes it easier to skim
@ -0,0 +1073,4 @@
this->drawing_array_size};
}
void GreasePencil::add_empty_drawings(int n)
Member

int n -> const int add_size

`int n` -> `const int add_size`
@ -0,0 +1081,4 @@
grease_pencil_grow_drawing_array_by(*this, n);
MutableSpan<GreasePencilDrawingBase *> new_drawings = this->drawings_for_write().drop_front(
prev_size);
for (const int i : IndexRange(new_drawings.size())) {
Member

IndexRange(new_drawings.size()) -> new_drawings.index_range()

Maybe regex this one? It's probably in this patch more than once

`IndexRange(new_drawings.size())` -> `new_drawings.index_range()` Maybe regex this one? It's probably in this patch more than once
@ -0,0 +1091,4 @@
}
}
void GreasePencil::remove_drawing(int index_to_remove)
Member

Declare all arguments const in definitions

Declare all arguments const in definitions
@ -0,0 +1096,4 @@
using namespace blender::bke::greasepencil;
/* In order to not change the indices of the drawings, we do the following to the drawing to be
* removed:
* - If the drawing (A) is not the last one:
Member

Is the - meant to be a 1)? I think 1. is a bit more common in code comments than 1)

Is the `-` meant to be a `1)`? I think `1.` is a bit more common in code comments than `1)`
@ -0,0 +1230,4 @@
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_base);
drawing->geometry.wrap().blend_read(*reader);
/* Initialize runtime data. */
drawing->geometry.runtime = MEM_new<blender::bke::CurvesGeometryRuntime>(__func__);
Member

These two things are handled by bke::CurvesGeometry::blend_read() now.

These two things are handled by `bke::CurvesGeometry::blend_read()` now.
@ -0,0 +1233,4 @@
drawing->geometry.runtime = MEM_new<blender::bke::CurvesGeometryRuntime>(__func__);
drawing->geometry.wrap().update_curve_types();
drawing->runtime = MEM_new<blender::bke::GreasePencilDrawingRuntime>(__func__);
drawing->runtime->triangles_cache.tag_dirty();
Member

Dirty is the initial state of a SharedCache, no need to tag it that way explicitly here.

Dirty is the initial state of a `SharedCache`, no need to tag it that way explicitly here.
@ -0,0 +1281,4 @@
drawing->geometry.wrap().~CurvesGeometry();
MEM_delete(drawing->runtime);
drawing->runtime = nullptr;
MEM_freeN(drawing);
Member

Looks like drawings are allocated with MEM_new above, which means they should be deallocated with MEM_delete.

Looks like drawings are allocated with `MEM_new` above, which means they should be deallocated with `MEM_delete`.
@ -0,0 +195,4 @@
int total_points_num = 0;
int total_triangles_num = 0;
int v = 0;
Vector<Vector<int>> verts_start_offsets_per_visible_drawing;
Member

Looks like this can be Vector<Array<int>>, which is better since they don't need to be resized anyway

Looks like this can be `Vector<Array<int>>`, which is better since they don't need to be resized anyway
@ -0,0 +197,4 @@
int v = 0;
Vector<Vector<int>> verts_start_offsets_per_visible_drawing;
Vector<Vector<int>> tris_start_offsets_per_visible_drawing;
grease_pencil.foreach_visible_drawing(cfra, [&](GreasePencilDrawing &drawing) {
Member

What about building a vector of the drawings once, to avoid putting most of the code inside the lambda here and the need to call "foreach_visible_drawing" twice:

  Vector<const GreasePencilDrawing *> drawings;
  grease_pencil.foreach_visible_drawing(
      cfra, [&](GreasePencilDrawing &drawing) { drawings.append(&drawing); });

...

  Array<Array<int>> verts_start_offsets_per_visible_drawing;
  Array<Array<int>> tris_start_offsets_per_visible_drawing;

  for (const int drawing_i : drawings.index_range()) {
    const GreasePencilDrawing &drawing = *drawings[drawing_i];

...

  /* Fill buffers with data. */
  for (const int drawing_i : drawings.index_range()) {
    const GreasePencilDrawing &drawing = *drawings[drawing_i];

What about building a vector of the drawings once, to avoid putting most of the code inside the lambda here and the need to call "foreach_visible_drawing" twice: ``` Vector<const GreasePencilDrawing *> drawings; grease_pencil.foreach_visible_drawing( cfra, [&](GreasePencilDrawing &drawing) { drawings.append(&drawing); }); ... Array<Array<int>> verts_start_offsets_per_visible_drawing; Array<Array<int>> tris_start_offsets_per_visible_drawing; for (const int drawing_i : drawings.index_range()) { const GreasePencilDrawing &drawing = *drawings[drawing_i]; ... /* Fill buffers with data. */ for (const int drawing_i : drawings.index_range()) { const GreasePencilDrawing &drawing = *drawings[drawing_i]; ```
@ -0,0 +205,4 @@
Vector<int> tris_start_offsets(curves.curves_num());
/* Calculate the vertex and triangle offsets for all the curves. */
int t = 0;
Member

More specific/helpful variable names than t and v would be nice here

More specific/helpful variable names than `t` and `v` would be nice here
@ -0,0 +221,4 @@
}
verts_start_offsets[curve_i] = v;
v += 1 + points.size() + (is_cyclic ? 1 : 0) + 1;
Member

Not clear what's going on here-- 1 + ... + 1?

Not clear what's going on here-- `1 + ... + 1`?
Author
Member

Looks like my comment about this got lost somehow. There is one extra vertex before and after each stroke. It's written in this way to make it more clear where that padding is added.

Looks like my comment about this got lost somehow. There is one extra vertex before and after each stroke. It's written in this way to make it more clear where that padding is added.
@ -0,0 +271,4 @@
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
const Span<float3> positions = curves.positions();
const VArray<bool> cyclic = curves.cyclic();
const VArray<float> radii = attributes.lookup_or_default<float>(
Member

Use the * overload rather than adding .varray at the end

Use the `*` overload rather than adding `.varray` at the end
@ -0,0 +283,4 @@
"material_index", ATTR_DOMAIN_CURVE, -1).varray;
const Span<uint3> triangles = drawing.triangles();
const Span<int> verts_start_offsets =
verts_start_offsets_per_visible_drawing[drawing_i].as_span();
Member

These .as_span() calls shouldn't be necessary

These `.as_span()` calls shouldn't be necessary
@ -688,6 +688,9 @@ static bool ED_object_editmode_load_free_ex(Main *bmain,
else if (obedit->type == OB_CURVES) {
/* Curves don't have specific edit mode data, so pass. */
}
else if (obedit->type == OB_GREASE_PENCIL) {
Member

This could be added to the existing OB_CURVES check with ELEM

This could be added to the existing `OB_CURVES` check with `ELEM`
@ -0,0 +44,4 @@
Scene *scene = CTX_data_scene(&C);
ARegion *region = CTX_wm_region(&C);
Object *obact = CTX_data_active_object(&C);
Object *ob_eval = DEG_get_evaluated_object(depsgraph, obact);
Member

It seems a bit weird to modifier the evaluated object here. Is that purposeful? Or am I missing something?

It seems a bit weird to modifier the evaluated object here. Is that purposeful? Or am I missing something?
Author
Member

Yes, this is done on purpose so that the extra copy from orig to eval for rendering is not needed every update. Once drawing is done, the stroke is copied from the eval buffer to orig.

Yes, this is done on purpose so that the extra copy from orig to eval for rendering is not needed every update. Once drawing is done, the stroke is copied from the eval buffer to orig.
Member

Okay, makes sense. This definitely deserves a comment, since typically changing the evaluated data isn't a good idea.

Okay, makes sense. This definitely deserves a comment, since typically changing the evaluated data isn't a good idea.
@ -0,0 +109,4 @@
curves.offsets_for_write()[num_old_curves] = num_old_points;
curves.offsets_for_write()[num_old_curves + 1] = num_old_points + stroke_points.size();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
Member

offset_indices::OffsetIndices -> OffsetIndices

`offset_indices::OffsetIndices` -> `OffsetIndices`
filedescriptor marked this conversation as resolved
@ -0,0 +82,4 @@
/**
* Number of users of this drawing in the layer tree.
*/
int32_t user_count;
Member

I wonder if this user count could be runtime data? Or if not, maybe it's worth mentioning why it isn't in a comment.

I wonder if this user count could be runtime data? Or if not, maybe it's worth mentioning why it isn't in a comment.
Author
Member

I thought about this, but I don't think it can be runtime data. Since there will be keyframe instances, we need to make the user count is saved.

I thought about this, but I don't think it can be runtime data. Since there will be keyframe instances, we need to make the user count is saved.
Member

The connection between keyframe instances and saving the user count isn't super clear to me. Maybe a comment about what the user count is used for here would help?

The connection between keyframe instances and saving the user count isn't super clear to me. Maybe a comment about what the user count is used for here would help?

Maybe it's still better to have this as runtime data. If there is ever a bug where this doesn't get counted correctly, at least a file read could fix it. It should be possible to read this as zero, and increment it for every user?

This is easy to change afterwards though.

Maybe it's still better to have this as runtime data. If there is ever a bug where this doesn't get counted correctly, at least a file read could fix it. It should be possible to read this as zero, and increment it for every user? This is easy to change afterwards though.
Author
Member

I see, re-creating the user count when reading could work indeed.

I see, re-creating the user count when reading could work indeed.
@ -0,0 +109,4 @@
/**
* A buffer for a single stroke while drawing.
*/
blender::Span<blender::bke::StrokePoint> stroke_buffer();
Member

What about a single function that returned std::optional rather than a separate "has_" function?

What about a single function that returned `std::optional` rather than a separate "has_" function?
@ -0,0 +110,4 @@
* A buffer for a single stroke while drawing.
*/
blender::Span<blender::bke::StrokePoint> stroke_buffer();
bool has_stroke_buffer();
Member

Looks like stroke_buffer() can be const

Looks like `stroke_buffer()` can be `const`
@ -0,0 +180,4 @@
struct GreasePencilLayerMask *next, *prev;
/**
* The name of the layer that is the mask.
* \note Null-terminated.
Member

Pretty sure null-terminated goes without saying for char * pointers in DNA, I don't think it's worth mentioning here

Pretty sure null-terminated goes without saying for `char *` pointers in DNA, I don't think it's worth mentioning here

Not sure about dynamic names for strings. It is not something typically used in the DNA.

Not sure about dynamic names for strings. It is not something typically used in the DNA.
Member

It's been done more recently I think. It's properly integrated with RNA now too, to avoid that boilerplate. It's nice not to have to worry about choosing a future-proof length.

It's been done more recently I think. It's properly integrated with RNA now too, to avoid that boilerplate. It's nice not to have to worry about choosing a future-proof length.
@ -0,0 +247,4 @@
/**
* List of `GreasePencilLayerMask`. Only used for storage in the .blend file.
*/
ListBase masks_storage;
Member

I wonder if you'd consider making this an array rather than a ListBase in DNA. Then the memory could be reused when the file is loaded, fewer small allocations would be needed, and the C++ type wouldn't have to have next and prev pointers even during runtime.

I wonder if you'd consider making this an array rather than a `ListBase` in DNA. Then the memory could be reused when the file is loaded, fewer small allocations would be needed, and the C++ type wouldn't have to have `next` and `prev` pointers even during runtime.
@ -0,0 +406,4 @@
struct AnimData *adt;
/**
* An array of pointers to drawings. The drawing can own it's data or reference it from another
Member

Grammar: it's data -> its data

Grammar: `it's data` -> `its data`
@ -0,0 +414,4 @@
int drawing_array_size;
char _pad[4];
#ifdef __cplusplus
void read_drawing_array(BlendDataReader *reader);
Member

Best to follow the style guidelines and put all methods at the bottom.
I think that's best since you can easily see what's actually stored in the class without digging through a bunch of methods.

Best to follow the style guidelines and put all methods at the bottom. I think that's best since you can easily see what's actually stored in the class without digging through a bunch of methods.
@ -0,0 +446,4 @@
*/
GreasePencilRuntimeHandle *runtime;
#ifdef __cplusplus
blender::Span<GreasePencilDrawingBase *> drawings() const;
Member

It probably shouldn't be possible to get a span of non-const pointers from a const GreasePencil:

Span<const GreasePencilDrawingBase *>

It probably shouldn't be possible to get a span of non-const pointers from a const `GreasePencil`: `Span<const GreasePencilDrawingBase *>`
@ -0,0 +449,4 @@
blender::Span<GreasePencilDrawingBase *> drawings() const;
blender::MutableSpan<GreasePencilDrawingBase *> drawings_for_write();
void add_empty_drawings(int n);
void remove_drawing(int index);
Member

This sort of API function (dealing with one index at a time) can be a bit dangerous since it can easily lead to quadratic behavior if it's called many times. I wonder if a remove_drawings(IndexMask drawings_to_remove) function would work a bit better

This sort of API function (dealing with one index at a time) can be a bit dangerous since it can easily lead to quadratic behavior if it's called many times. I wonder if a `remove_drawings(IndexMask drawings_to_remove)` function would work a bit better
Sergey Sharybin requested changes 2023-04-25 10:15:37 +02:00
Sergey Sharybin left a comment
Owner

A quick pass of review.

The most unclear part is the amount of TODOs, and some dead/unfinished code. Are those intended to be solved before the merge? If not, it would be nice to have some brief wrap up about what those are about.

Like, not /* TODO */ but /* TODO: Support reading intent of artists. */, or /* Reference code implementation */.

A quick pass of review. The most unclear part is the amount of TODOs, and some dead/unfinished code. Are those intended to be solved before the merge? If not, it would be nice to have some brief wrap up about what those are about. Like, not `/* TODO */` but `/* TODO: Support reading intent of artists. */`, or `/* Reference code implementation */`.
@ -0,0 +117,4 @@
*
* | | | | | | | | | | |1|1|1|1|1|1|1|
* Time: |0|1|2|3|4|5|6|7|8|9|0|1|2|3|4|5|6|...
* Frame: [#0 ][#1 ] [#2 ]

I am a bit confused here.

Following the text, it seems it should be Frame and Drawing (not Time and Frame).

Or, the description needs to be updated (and a good argument made why the common terms Time and Frame are redefined in the grease pencil).

I am a bit confused here. Following the text, it seems it should be `Frame` and `Drawing` (not `Time` and `Frame`). Or, the description needs to be updated (and a good argument made why the common terms `Time` and `Frame` are redefined in the grease pencil).
Author
Member

Ah yes, Frame is the term we use for a Drawing at a particular time. I agree that this is very confusing here. I am not sure what a better term would be.

Ah yes, `Frame` is the term we use for a `Drawing` at a particular time. I agree that this is very confusing here. I am not sure what a better term would be.

I see. The GreasePencilFrame is nice and unambiguous. And within a context of GP it is not bad to stick to general rule "frame means grease pencil frame".

But when you refer to frame as a moment in time, is the best to use "scene frame". So, it will be something rhe first drawing starts at scene frame 0 and ends at scene frame 5 (exclusive) (btw exclusive not excusive)`.

Also use the same "scene frame" in diagram below. Generally time in Blender is scene frame number divided by the scene fps, and is measured in seconds.

I see. The `GreasePencilFrame` is nice and unambiguous. And within a context of GP it is not bad to stick to general rule "frame means grease pencil frame". But when you refer to frame as a moment in time, is the best to use "scene frame". So, it will be something `rhe first drawing starts at scene frame 0 and ends at scene frame 5 (exclusive)` (btw exclusive not excusive)`. Also use the same "scene frame" in diagram below. Generally time in Blender is scene frame number divided by the scene fps, and is measured in seconds.
@ -0,0 +151,4 @@
}
}
radii.finish();

I thought we had some RAII helper which does finish() at the end of the scope.
Or maybe it was just an idea to have one? Maybe Jacques of Hans remember better :)

I thought we had some RAII helper which does ` finish()` at the end of the scope. Or maybe it was just an idea to have one? Maybe Jacques of Hans remember better :)

Right now regular we do this manually, but true, this could be better to use BLI_SCOPED_DEFER.

Right now regular we do this manually, but true, this could be better to use `BLI_SCOPED_DEFER`.
Member

We don't really want to do non-trivial work in a class's destructor. It's also helpful to make this explicit, since sometimes it matters when it happens.

We don't really want to do non-trivial work in a class's destructor. It's also helpful to make this explicit, since sometimes it matters when it happens.

I don't really understand it, and can't say I'm sold on the motivation.
But in any case, at this point the discussion does not affect the PR, so lets consider it resolved here, and any followup we can do in the chat instead.

I don't really understand it, and can't say I'm sold on the motivation. But in any case, at this point the discussion does not affect the PR, so lets consider it resolved here, and any followup we can do in the chat instead.
@ -0,0 +16,4 @@
/* --------------------------------------------------------------------------------------------- */
/* Grease Pencil ID Tests. */
struct GreasePencilIDTestContext {
Use text fixtures: http://google.github.io/googletest/primer.html#same-data-multiple-tests
Author
Member

I used fixtures before, but decided to use this approach instead, because all the tests are then under the same greasepencil namespace. I didn't find a way to do this with fixtures, unless I name the class literally greasepencil.

I used fixtures before, but decided to use this approach instead, because all the tests are then under the same `greasepencil` namespace. I didn't find a way to do this with fixtures, unless I name the class literally `greasepencil`.

You can't do that either. It is at a very least discouraged to use the same name for fixture and "regular" test.

Typically the "namespace" defines context you're testing, and it is perfectly fine to have multiple of such contexts per file (euclidean_resection_test.cc). But with the monolithic nature of BKE/BLI tests there is no good way to achieve the same behavior.

I would love to improve the monolithic state of tests at some point (as it is really in a way every time I work on test), but that is outside of the scope of this patch.

Would be nice to add a comment on top of the GreasePencilIDTestContext which briefly summarizes your choice of this approach. Basically, so if someone else stumbles on this code and winders "why not fixtures" they have an answer.

You can't do that either. It is at a very least discouraged to use the same name for fixture and "regular" test. Typically the "namespace" defines context you're testing, and it is perfectly fine to have multiple of such contexts per file (`euclidean_resection_test.cc`). But with the monolithic nature of BKE/BLI tests there is no good way to achieve the same behavior. I would love to improve the monolithic state of tests at some point (as it is really in a way every time I work on test), but that is outside of the scope of this patch. Would be nice to add a comment on top of the `GreasePencilIDTestContext` which briefly summarizes your choice of this approach. Basically, so if someone else stumbles on this code and winders "why not fixtures" they have an answer.
@ -0,0 +1,154 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2022 Blender Foundation. */

2023

2023
filedescriptor marked this conversation as resolved
@ -0,0 +77,4 @@
const float *sizeinv = DRW_viewport_invert_size_get();
const float4 metrics = {sizeinv[0], sizeinv[1], size[0], size[1]};
if (anti_aliasing_enabled_) {

I would totally split the code into smaller functions. I do not see what are we winning from such inlined code.

I would totally split the code into smaller functions. I do not see what are we winning from such inlined code.
@ -0,0 +136,4 @@
manager.submit(edge_detect_ps_);
}
if (anti_aliasing_enabled_) {