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
e8d45e8ace Rename gpencil files to legacy
This renames the `BKE_gpencil_*` as well as the `DNA_gpencil_types.h`
files to indicate that it's the legacy grease pencil.
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
buildbot/vexp-code-patch-coordinator Build done. Details
0b9a5381f9
Add GreasePencilFrame in the layer map
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
c8dc81e8e4 Big refactor
* 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
e4eddaa839 Fix old grease pencil not rendering
When the experimental option was turned off, old grease pencil
objects would no longer render.
buildbot/vexp-code-patch-coordinator Build done. Details
b2f8f066c0
Make draw keymap dependent of experimental setting
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
buildbot/vexp-code-patch-coordinator Build done. Details
228d817743
Fix keymap test failing
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_) {

Why this extra check is needed?

Why this extra check is needed?
@ -0,0 +1,325 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2022 Blender Foundation. */

2023

2023
filedescriptor marked this conversation as resolved
@ -0,0 +19,4 @@
#include "draw_manager.hh"
#include "draw_pass.hh"
#define GP_LIGHT

What is this about?

What is this about?
@ -0,0 +1,65 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2022 Blender Foundation. */

Check everywhere else :)

2023. Check everywhere else :)
@ -0,0 +8,4 @@
#pragma once
#include "BKE_grease_pencil.hh"
#include "BKE_image.h"

I am not sure all of those are needed here.
I.e. I do not see any BKE_image API used in this header. Might be missing something.

I am not sure all of those are needed here. I.e. I do not see any `BKE_image` API used in this header. Might be missing something.
filedescriptor marked this conversation as resolved
@ -0,0 +32,4 @@
void sync(const Object *object, const bke::greasepencil::Layer &layer, bool &do_layer_blending)
{
UNUSED_VARS(object, layer);

In C++ use void sync(const Object * /*object*/, const bke::greasepencil::Layer & /*layer*/, bool &do_layer_blending)

In C++ use `void sync(const Object * /*object*/, const bke::greasepencil::Layer & /*layer*/, bool &do_layer_blending)`
@ -0,0 +164,4 @@
ob.material_offset = material_offset;
if (do_layer_blending) {
// for (const LayerData &layer : frame.layers) {

Typically dead code is discouraged.
Not sure what is it part of, so can't really give strong suggestions.

Typically dead code is discouraged. Not sure what is it part of, so can't really give strong suggestions.
@ -0,0 +186,4 @@
}
// });
#if 0

Add a comment what is this about, or remove.

Add a comment what is this about, or remove.
@ -0,0 +248,4 @@
}
private:
// static float4 frame_tint_get(const bGPdata *gpd, const bGPDframe *gpf, int /* current_frame

Same as above.

Same as above.
@ -0,0 +41,4 @@
const char *name = static_shader_create_info_name_get(eShaderType(i));
if (name == nullptr) {
std::cerr << "GPencil: Missing case for eShaderType(" << i
<< ") in static_shader_create_info_name_get().";

<< std::endl to put new line and flush the output buffer.

Also, why not to assert the details in the static_shader_create_info_name_get() ? It is a bit weird to print an error about mistake in some other code.

P.S. I think we should implement Glog style of checkers: DCHECK_NE(name, nullptr) << "GPencil: Missing case for eShaderType(" << i << ") in static_shader_create_info_name_get()."

`<< std::endl` to put new line and flush the output buffer. Also, why not to assert the details in the `static_shader_create_info_name_get()` ? It is a bit weird to print an error about mistake in some other code. P.S. I think we should implement Glog style of checkers: `DCHECK_NE(name, nullptr) << "GPencil: Missing case for eShaderType(" << i << ") in static_shader_create_info_name_get()."`
@ -0,0 +105,4 @@
shaders_[shader_type] = GPU_shader_create_from_info_name(shader_name);
if (shaders_[shader_type] == nullptr) {
fprintf(stderr, "GPencil: error: Could not compile static shader \"%s\"\n", shader_name);

Pick a side between fprintf and std::cerr ;)

Pick a side between `fprintf` and `std::cerr` ;)
@ -256,3 +256,3 @@
/* GPencil */
/* GPencil (legacy) */
struct GPUBatch *DRW_cache_gpencil_get(struct Object *ob, int cfra);

Can those be renamed to DRW_cache_gpencil_legacy_* ?

Can those be renamed to `DRW_cache_gpencil_legacy_*` ?
@ -0,0 +303,4 @@
s_vert.stroke_id = verts_range.first();
s_vert.mat = materials[curve_i] % GPENCIL_MATERIAL_BUFFER_LEN;
/* TODO */

Add details about TODO. Something that helps others to pick up a task, or to understand that specific case is not expected to be yet working.

Add details about TODO. Something that helps others to pick up a task, or to understand that specific case is not expected to be yet working.
@ -0,0 +47,4 @@
operation = greasepencil::new_paint_operation().release();
break;
default:
BLI_assert_unreachable();

I am not really sure what the goal is here. If the goal is to somehow ensure all cases are handled it is much better to not use default and let compiler tell you where cases are missing at the compile time.

I am not really sure what the goal is here. If the goal is to somehow ensure all cases are handled it is much better to not use `default` and let compiler tell you where cases are missing at the compile time.
@ -0,0 +122,4 @@
return OPERATOR_CANCELLED;
}
op->customdata = static_cast<void *>(paint_stroke_new(C,

Is the explicit cast to void* needed?

Is the explicit cast to `void*` needed?
filedescriptor marked this conversation as resolved
@ -0,0 +131,4 @@
stroke_done,
event->type));
int return_value = op->type->modal(C, op, event);

const int

`const int`
filedescriptor marked this conversation as resolved
@ -0,0 +191,4 @@
ob->mode = OB_MODE_PAINT_GPENCIL;
/* Setup cursor color. BKE_paint_init() could be used, but creates an additional brush. */

What's this commented out code is about?

What's this commented out code is about?
Author
Member

Thank you Hans and Sergey for the first review.

I should maybe clarify that the intention is not to move to master with something that is fully working. I'm sure that the APIs around the different grease pencil parts will change as we implement more operators, etc. I was trying to have something very minimal. My goal is to start working with contributors together as soon as possible.

What I am mostly concerned about at the moment are two things:

  1. The DNA. I would like to avoid changing anything major in it after moving to main to avoid having to do versioning of experimental code, just because our test files start crashing when being opened.
  2. The separation between old and new grease pencil. There are some questionable things that I am doing right now to make sure everything is working as before when the experimental option is unchecked (like if/else in the keymap code, overwriting the mode switching operator, etc.). I am still struggling to find good solutions here.
Thank you Hans and Sergey for the first review. I should maybe clarify that the intention is not to move to master with something that is fully working. I'm sure that the APIs around the different grease pencil parts will change as we implement more operators, etc. I was trying to have something very minimal. My goal is to start working with contributors together as soon as possible. What I am mostly concerned about at the moment are two things: 1) The DNA. I would like to avoid changing anything major in it after moving to main to avoid having to do versioning of experimental code, just because our test files start crashing when being opened. 2) The separation between old and new grease pencil. There are some questionable things that I am doing right now to make sure everything is working as before when the experimental option is unchecked (like if/else in the keymap code, overwriting the mode switching operator, etc.). I am still struggling to find good solutions here.
Falk David added 2 commits 2023-04-25 17:32:10 +02:00
Falk David added 6 commits 2023-04-26 17:37:25 +02:00
Falk David added 2 commits 2023-04-26 18:10:33 +02:00
Falk David added 1 commit 2023-04-26 18:15:58 +02:00
Falk David added 4 commits 2023-04-27 21:15:04 +02:00
Brecht Van Lommel refused to review 2023-04-28 16:12:38 +02:00

This seems fine overall from a quick look. Since others have already looked at this in more detail will leave review to them.

This seems fine overall from a quick look. Since others have already looked at this in more detail will leave review to them.
Falk David added 1 commit 2023-05-02 12:51:21 +02:00
Falk David added 2 commits 2023-05-02 14:14:43 +02:00
Falk David added 1 commit 2023-05-02 15:02:02 +02:00
Falk David requested review from Hans Goudey 2023-05-02 15:02:28 +02:00
Falk David requested review from Sergey Sharybin 2023-05-02 15:02:47 +02:00
Clément Foucault approved these changes 2023-05-02 17:02:01 +02:00
Clément Foucault left a comment
Member

The draw manager / draw engine side looks good (no wonder, it's mostly my code 🤡 ). But would be nice to compile all the TODOs in #102506 (if they are not already there) to keep track of the progress.

The draw manager / draw engine side looks good (no wonder, it's mostly my code 🤡 ). But would be nice to compile all the TODOs in #102506 (if they are not already there) to keep track of the progress.
Hans Goudey requested changes 2023-05-02 23:57:44 +02:00
Hans Goudey left a comment
Member

Looks closer. Trying not to be too picky since the plan is to continue work in main. But I did comment on a few simple things too, just to get them out of the way before they become a cleanup commit in main. The diff could use a make format (in some Python files too).

Looks closer. Trying not to be too picky since the plan is to continue work in main. But I did comment on a few simple things too, just to get them out of the way before they become a cleanup commit in main. The diff could use a `make format` (in some Python files too).
@ -0,0 +47,4 @@
/**
* \returns true if this node is a LayerGroup.
*/
constexpr bool is_group() const
Member

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?
@ -0,0 +82,4 @@
* \returns this tree node as a mutable Layer.
* \note This results in undefined behavior if the node is not a Layer.
*/
Layer &as_layer_for_write();
Member

The _for_write() suffix doesn't seem super helpful here TBH, but I guess it doesn't hurt.

The `_for_write()` suffix doesn't seem super helpful here TBH, but I guess it doesn't hurt.
@ -0,0 +293,4 @@
* Stroke cache for a stroke that is currently being drawn.
*/
struct StrokeCache {
Vector<StrokePoint> points = {};
Member

= {} 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
@ -0,0 +54,4 @@
{
using namespace blender;
GreasePencil *grease_pencil_dst = (GreasePencil *)id_dst;
Member

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

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.
@ -0,0 +418,4 @@
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &&frame)
{
return this->frames_for_write().add_overwrite(frame_number, frame);
Member

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

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

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`.
@ -0,0 +687,4 @@
return grease_pencil;
}
BoundBox *BKE_grease_pencil_boundbox_get(Object *ob)
Member

Other geometry data-block types have "min max" functions that take the data-block as an argument rather than the object. I hope to remove the object level function in the future, so for consistency, having the data-block function here would be nice.

Other geometry data-block types have "min max" functions that take the data-block as an argument rather than the object. I hope to remove the object level function in the future, so for consistency, having the data-block function here would be nice.
HooglyBoogly marked this conversation as resolved
@ -0,0 +787,4 @@
const bke::CurvesGeometry &curves = this->geometry.wrap();
const Span<float3> positions = curves.positions();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
Member

offset_indices::OffsetIndices -> OffsetIndices

Please search the patch for this, I mentioned it last time too

`offset_indices::OffsetIndices` -> `OffsetIndices` Please search the patch for this, I mentioned it last time too
@ -0,0 +1141,4 @@
Vector<LayerMask> masks = new_layer.masks_for_write();
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];
Member

GreasePencilLayerMask *mask = &masks_storage -> GreasePencilLayerMask &mask = masks_storage

`GreasePencilLayerMask *mask = &masks_storage` -> `GreasePencilLayerMask &mask = masks_storage`
@ -0,0 +19,4 @@
namespace blender::bke::greasepencil::convert {
void legacy_gpencil_frame_to_grease_pencil_drawing(GreasePencilDrawing &drawing, bGPDframe &gpf)
Member

Order return argument last, make the gpf argument const

Order return argument last, make the `gpf` argument const
Falk David added 1 commit 2023-05-03 11:29:56 +02:00
Falk David force-pushed grease-pencil-v3 from 96c1c209f9 to 9a64690cac 2023-05-03 11:30:17 +02:00 Compare
Falk David added 1 commit 2023-05-03 12:13:43 +02:00
Falk David requested review from Hans Goudey 2023-05-03 12:13:58 +02:00
Falk David added 1 commit 2023-05-03 12:59:56 +02:00
Falk David added 1 commit 2023-05-03 15:15:18 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
754ed9a6d0
comment
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2023-05-03 16:26:43 +02:00
@ -0,0 +36,4 @@
explicit TreeNode(GreasePencilLayerTreeNodeType type);
explicit TreeNode(GreasePencilLayerTreeNodeType type, StringRefNull name);
TreeNode(const TreeNode &other);
TreeNode &operator=(const TreeNode &other) = delete;
Member

Is there a particular reason you remove copy assignment but keep copy construction defined?

Is there a particular reason you remove copy assignment but keep copy construction defined?
Author
Member

Just because copying should be explicit. And removing the copy assignment constructor avoids errors.

Just because copying should be explicit. And removing the copy assignment constructor avoids errors.
@ -0,0 +47,4 @@
/**
* \returns true if this node is a LayerGroup.
*/
const bool is_group() const
Member

The const in the const bool return type means nothing here

The `const` in the `const bool` return type means nothing here
@ -0,0 +153,4 @@
/**
* \returns the layer masks.
*/
const Vector<LayerMask> &masks() const;
Member

Better to return a span here maybe?

Better to return a span here maybe?
@ -0,0 +163,4 @@
/**
* \return true if the layer is locked.
*/
bool is_locked() const;
Member

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.
@ -0,0 +53,4 @@
const int /*flag*/)
{
using namespace blender;
Member

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

C++ cast here (and above, I'll stop writing it now)
@ -0,0 +169,4 @@
static void grease_pencil_blend_read_data(BlendDataReader *reader, ID *id)
{
using namespace blender::bke::greasepencil;
GreasePencil *grease_pencil = (GreasePencil *)id;
Member

C++ cast here

C++ cast here
@ -0,0 +190,4 @@
static void grease_pencil_blend_read_lib(BlendLibReader *reader, ID *id)
{
GreasePencil *grease_pencil = (GreasePencil *)id;
Member

C++ cast here

C++ cast here
@ -0,0 +411,4 @@
return this->frames_for_write().add(frame_number, frame);
}
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &frame)
Member

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.
@ -0,0 +434,4 @@
return this->sorted_keys_cache_.data();
}
int Layer::drawing_index_at(int frame) const
Member

int frame -> const int frame

`int frame` -> `const int frame`
@ -0,0 +1196,4 @@
}
static void save_tree_node_to_storage(const blender::bke::greasepencil::TreeNode &node,
GreasePencilLayerTreeNode *dst)
Member

GreasePencilLayerTreeNode *dst -> GreasePencilLayerTreeNode &dst

`GreasePencilLayerTreeNode *dst` -> `GreasePencilLayerTreeNode &dst`
@ -0,0 +1334,4 @@
void GreasePencil::read_layer_tree_storage(BlendDataReader *reader)
{
BLO_read_pointer_array(reader, (void **)&this->layer_tree_storage.nodes);
Member

Use reinterpret_cast over C style cast

Use `reinterpret_cast` over C style cast
@ -0,0 +1415,4 @@
}
for (int i = 0; i < this->layer_tree_storage.nodes_num; i++) {
GreasePencilLayerTreeNode *node = this->layer_tree_storage.nodes[i];
if (node->name) {
Member

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
Falk David added 1 commit 2023-05-03 16:36:55 +02:00
Falk David requested review from Hans Goudey 2023-05-04 11:44:45 +02:00
Falk David added 2 commits 2023-05-04 11:44:46 +02:00
Sergey Sharybin approved these changes 2023-05-04 11:48:28 +02:00
Falk David added 1 commit 2023-05-04 12:04:59 +02:00
Falk David added 1 commit 2023-05-04 13:58:26 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
79498b8740
Merge branch 'main' into grease-pencil-v3
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2023-05-04 14:49:29 +02:00
@ -0,0 +19,4 @@
#include "BLI_smaa_textures.h"
namespace blender::greasepencil {
Member

It looks like this should be in the blender::draw:: namespace first, given the file path. Since greasepencil isn't a top-level module, a blender::greasepencil namespace probably doesn't make sense in general.

Same with elsewhere in this diff.

It looks like this should be in the `blender::draw::` namespace first, given the file path. Since `greasepencil` isn't a top-level module, a `blender::greasepencil` namespace probably doesn't make sense in general. Same with elsewhere in this diff.
@ -0,0 +200,4 @@
using namespace blender;
struct GPENCIL_NEXT_Data {
Member

This struct name is interesting... I guess it's temporary though :P

This struct name is interesting... I guess it's temporary though :P
Author
Member

Yes it is :)

Yes it is :)
@ -0,0 +296,4 @@
struct RenderLayer *layer,
const struct rcti * /*rect*/)
{
UNUSED_VARS(vedata, engine, layer);
Member

Commented argument names instead of this macro here

Commented argument names instead of this macro here
Falk David added 2 commits 2023-05-04 15:08:22 +02:00
Falk David requested review from Hans Goudey 2023-05-04 15:08:41 +02:00
Hans Goudey approved these changes 2023-05-04 15:37:11 +02:00
Antonio Vazquez refused to review 2023-05-04 16:43:11 +02:00
Bastien Montagne requested changes 2023-05-06 20:26:27 +02:00
Bastien Montagne left a comment
Owner

Globally looks pretty good, but I have a few concerns with current patch.

First, not sure what is the role of the WITH_GREASE_PENCIL_V3 define? It does not seem to control any logic, but only a few UI strings? Is it a left-over from previous stages of development for this patch?

Unless I missed it, there is currently no check at all for dependency loops between IDs? When it comes to GreasePencilDrawingReference, pretty sure it should be forbidden to have these, just like with Collections. And most likely same check would be needed for the Object parent pointer of GreasePencilLayer.

Am also missing at least a skeletal of RNA implementation... What even happens currently from Py API when accessing ob.data on one of these new GP objects? We just get a generic ID data? In general having at least a read-only RNA access to new data should be a high priority task when introducing a new ID type imho, since it makes introspection and 'high level' debugging so much easier...

Finally, my biggest concern is with how this new ID and its data is handled when written on disk/memfile. Besides the dire consequences of the current behavior on partial undo (see comment below for details), in general this goes against the 'philosophy' of IDs. The file version of an ID is essentially a memory dump of its DNA data. That's why it's so fast to write and read, why it can be used as undo data, etc. If the 'file' version of the data is not optimal for some editing task, some runtime data is generated from it when needed, and 'baked' back into file-type data asap.

Unless I'm completely misunderstanding things, this new GPencil ID is doing the opposite for its layers: runtime data is the 'main' data, and the persistent data stored on file is generated on demand from it? Not sure if/when/who signed off on this design, but if this is the way we want to go it would be good to have the whole team aware and agreeing on it, this is a major change for Blender as a whole. Probably better to talk IRL about this though next week.

Globally looks pretty good, but I have a few concerns with current patch. First, not sure what is the role of the `WITH_GREASE_PENCIL_V3` define? It does not seem to control any logic, but only a few UI strings? Is it a left-over from previous stages of development for this patch? Unless I missed it, there is currently no check at all for dependency loops between IDs? When it comes to `GreasePencilDrawingReference`, pretty sure it should be forbidden to have these, just like with Collections. And most likely same check would be needed for the Object `parent` pointer of `GreasePencilLayer`. Am also missing at least a skeletal of RNA implementation... What even happens currently from Py API when accessing `ob.data` on one of these new GP objects? We just get a generic ID data? In general having at least a read-only RNA access to new data should be a high priority task when introducing a new ID type imho, since it makes introspection and 'high level' debugging so much easier... Finally, my biggest concern is with how this new ID and its data is handled when written on disk/memfile. Besides the dire consequences of the current behavior on partial undo (see comment below for details), in general this goes against the 'philosophy' of IDs. The file version of an ID is essentially a memory dump of its DNA data. That's why it's so fast to write and read, why it can be used as undo data, etc. If the 'file' version of the data is not optimal for some editing task, some runtime data is generated from it when needed, and 'baked' back into file-type data asap. Unless I'm completely misunderstanding things, this new GPencil ID is doing the opposite for its layers: runtime data is the 'main' data, and the persistent data stored on file is generated on demand from it? Not sure if/when/who signed off on this design, but if this is the way we want to go it would be good to have the whole team aware and agreeing on it, this is a major change for Blender as a whole. Probably better to talk IRL about this though next week.
@ -1983,3 +1990,3 @@
),
)

Picky detail, but there should not be extra spaces on this empty line ;)

Picky detail, but there should not be extra spaces on this empty line ;)
@ -0,0 +114,4 @@
* {0: 0, 5: 1, 10: -1, 12: 2, 16: -1}
*
* In this example there are three drawings (drawing #0, drawing #1 and drawing #2). The first
* drawing starts at frame 0 and ends at frame 5 (excusive). The second drawing starts at

exclusive

`exclusive`
@ -0,0 +207,4 @@
*/
mutable CacheMutex children_cache_mutex_;
/**
* Caches all the children of this group in a single pre-order vector.

Not sure what is a 'pre-order vector'? or is a typo? Like pre-ordered vector? Same below.

Not sure what is a 'pre-order vector'? or is a typo? Like `pre-ordered vector`? Same below.
@ -0,0 +273,4 @@
} // namespace convert
} // namespace greasepencil

Not sure why the greasepencil namespace ends up here? Also means that e.g. the fairly generically-named StrokePoint struct is in the fairly generic blender::bke namespace, which does not sounds great to me? not to mention 'C-style' namespace in names like GreasePencilDrawingRuntime.

Not sure why the `greasepencil` namespace ends up here? Also means that e.g. the fairly generically-named `StrokePoint` struct is in the fairly generic `blender::bke` namespace, which does not sounds great to me? not to mention 'C-style' namespace in names like `GreasePencilDrawingRuntime`.
@ -0,0 +144,4 @@
/* Free storage if there is one. */
grease_pencil->free_layer_tree_storage();
grease_pencil->save_layer_tree_to_storage();

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.
@ -0,0 +684,4 @@
{
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(
BKE_id_new_nomain(ID_GP, nullptr));
grease_pencil_init_data(&grease_pencil->id);

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.
@ -480,6 +480,8 @@ uint64_t BKE_library_id_can_use_filter_id(const ID *id_owner)
case ID_IP:
/* Deprecated... */
return 0;
case ID_GP:

Would rather add the new case immediately after the legacy GP one... At the very least before the generic 'do nothing' block of cases, and the deprecated ones.

Would rather add the new case immediately after the legacy GP one... At the very least before the generic 'do nothing' block of cases, and the deprecated ones.
@ -654,6 +654,8 @@ ListBase *which_libbase(Main *bmain, short type)
return &(bmain->volumes);
case ID_SIM:
return &(bmain->simulations);
case ID_GP:

Same as above.

Same as above.
@ -719,6 +721,7 @@ int set_listbasepointers(Main *bmain, ListBase *lb[/*INDEX_ID_MAX*/])
lb[INDEX_ID_WM] = &(bmain->wm);
lb[INDEX_ID_MSK] = &(bmain->masks);
lb[INDEX_ID_SIM] = &(bmain->simulations);
lb[INDEX_ID_GP] = &(bmain->grease_pencils);

Same as above.

Same as above.
@ -1110,3 +1110,3 @@
return N_("Browse Particle Settings to be linked");
case ID_GD_LEGACY:
return N_("Browse Grease Pencil Data to be linked");
return N_("Browse Grease Pencil (legacy) Data to be linked");

Shouldn't this change also be conditioned by the #ifdef WITH_GREASE_PENCIL_V3?

Shouldn't this change also be conditioned by the `#ifdef WITH_GREASE_PENCIL_V3`?
@ -41,3 +41,3 @@
{ID_CV, "CURVES", ICON_CURVES_DATA, "Curves", ""},
{ID_VF, "FONT", ICON_FONT_DATA, "Font", ""},
{ID_GD_LEGACY, "GREASEPENCIL", ICON_GREASEPENCIL, "Grease Pencil", ""},
{ID_GD_LEGACY, "GREASEPENCIL", ICON_GREASEPENCIL, "Grease Pencil (legacy)", ""},

This change should also be conditioned to #ifdef WITH_GREASE_PENCIL_V3 ?

This change should also be conditioned to `#ifdef WITH_GREASE_PENCIL_V3` ?
@ -255,3 +255,3 @@
{OB_POINTCLOUD, "POINTCLOUD", ICON_OUTLINER_OB_POINTCLOUD, "Point Cloud", ""},
{OB_VOLUME, "VOLUME", ICON_OUTLINER_OB_VOLUME, "Volume", ""},
{OB_GPENCIL_LEGACY, "GPENCIL", ICON_OUTLINER_OB_GREASEPENCIL, "Grease Pencil", ""},
{OB_GPENCIL_LEGACY, "GPENCIL", ICON_OUTLINER_OB_GREASEPENCIL, "Grease Pencil (legacy)", ""},

This change should also be conditioned to #ifdef WITH_GREASE_PENCIL_V3 ?

This change should also be conditioned to `#ifdef WITH_GREASE_PENCIL_V3` ?
Falk David added 2 commits 2023-05-08 12:25:24 +02:00
Falk David added 1 commit 2023-05-08 12:34:31 +02:00
Falk David added 2 commits 2023-05-08 14:17:01 +02:00
Falk David added 1 commit 2023-05-15 15:03:29 +02:00
Falk David added 1 commit 2023-05-22 10:54:15 +02:00
Falk David added 2 commits 2023-05-23 14:36:21 +02:00
Falk David added 2 commits 2023-05-23 15:13:43 +02:00
Author
Member

With the latest commit, the layer tree is now fully stored in DNA. The API and the tests have been updated. Still missing the test for the active layer.

With the latest commit, the layer tree is now fully stored in DNA. The API and the tests have been updated. Still missing the test for the active layer.
Falk David requested review from Bastien Montagne 2023-05-23 15:19:28 +02:00
Falk David requested review from Hans Goudey 2023-05-23 15:19:41 +02:00
Falk David added 2 commits 2023-05-23 15:48:40 +02:00
Falk David added 2 commits 2023-05-23 16:22:44 +02:00
Falk David added 1 commit 2023-05-25 11:31:30 +02:00
Bastien Montagne requested changes 2023-05-25 14:54:07 +02:00
Bastien Montagne left a comment
Owner

Besides issues with read/write code, things look pretty OK to me now. (as before, mainly reviewed ID management, read/write code, RNA areas, did not check drawing code at all, and not much of the sculpt/paint related one either).

Not sure about the point of keeping WITH_GREASE_PENCIL_V3 though? it's making the code a bit more confusing, and not sure what's it's adding at this point?

Besides issues with read/write code, things look pretty OK to me now. (as before, mainly reviewed ID management, read/write code, RNA areas, did not check drawing code at all, and not much of the sculpt/paint related one either). Not sure about the point of keeping `WITH_GREASE_PENCIL_V3` though? it's making the code a bit more confusing, and not sure what's it's adding at this point?
@ -0,0 +190,4 @@
/**
* \returns the layer masks.
*/
// Span<LayerMask> masks() const;

Would be good to know why this is commented out, or it should be cleaned up before merge in main.

Would be good to know why this is commented out, or it should be cleaned up before merge in main.
filedescriptor marked this conversation as resolved
@ -0,0 +320,4 @@
void *batch_cache = nullptr;
public:
// bool has_active_layer() const;

Same remark as above about commented out code.

Same remark as above about commented out code.
filedescriptor marked this conversation as resolved
@ -0,0 +122,4 @@
BKE_grease_pencil_batch_cache_free(grease_pencil);
MEM_delete(grease_pencil->runtime);
grease_pencil->runtime = nullptr;

These two lines could use MEM_SAFE_FREE macro instead.

These two lines could use `MEM_SAFE_FREE` macro instead.
Member

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

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.
@ -0,0 +147,4 @@
/* Write animation data. */
if (grease_pencil->adt) {
BKE_animdata_blend_write(writer, grease_pencil->adt);

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!
filedescriptor marked this conversation as resolved
@ -0,0 +172,4 @@
/* Read animation data. */
BLO_read_data_address(reader, &grease_pencil->adt);
BKE_animdata_blend_read_data(reader, grease_pencil->adt);

Code is missing re-reading active_layer address...

Code is missing re-reading `active_layer` address...
filedescriptor marked this conversation as resolved
@ -0,0 +1188,4 @@
BLO_write_struct(writer, GreasePencilLayer, node);
BLO_write_string(writer, node->base.name);
MEM_SAFE_FREE(node->frames_storage.keys);

Once again this is re-generating data on write, which will systematically destroy the partial undo process, since it means that the data will always be modified on each and every undo step.

Once again this is re-generating data on write, which will systematically destroy the partial undo process, since it means that the data will always be modified on each and every undo step.
filedescriptor marked this conversation as resolved
Falk David added 4 commits 2023-05-25 15:04:54 +02:00
Falk David added 3 commits 2023-05-25 18:29:36 +02:00
Falk David added 3 commits 2023-05-26 11:02:36 +02:00
Falk David added 1 commit 2023-05-26 11:03:16 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
b5912421c0
Add API to find a layer by name
Author
Member

@blender-bot build

@blender-bot build
Bastien Montagne approved these changes 2023-05-26 13:02:00 +02:00
Bastien Montagne left a comment
Owner

Some few minor things to fix still, besides that LGTM

Some few minor things to fix still, besides that LGTM
@ -0,0 +111,4 @@
/* Duplicate runtime data. */
if (grease_pencil_src->runtime) {
grease_pencil_dst->runtime = MEM_new<bke::GreasePencilRuntime>(__func__,

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.
filedescriptor marked this conversation as resolved
@ -0,0 +128,4 @@
BKE_grease_pencil_batch_cache_free(grease_pencil);
MEM_delete(grease_pencil->runtime);

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.
@ -4523,6 +4523,9 @@ static RNAProcessItem PROCESS_ITEMS[] = {
{"rna_dynamicpaint.c", NULL, RNA_def_dynamic_paint},
{"rna_fcurve.c", "rna_fcurve_api.c", RNA_def_fcurve},
{"rna_gpencil_legacy.c", NULL, RNA_def_gpencil},
#ifdef WITH_GREASE_PENCIL_V3

This should not be ifdef'd, RNA should be available, even if the new type is hidden/disabled from user PoV.

This should not be ifdef'd, RNA should be available, even if the new type is hidden/disabled from user PoV.
@ -40,7 +40,11 @@ const EnumPropertyItem rna_enum_id_type_items[] = {
{ID_CU_LEGACY, "CURVE", ICON_CURVE_DATA, "Curve", ""},
{ID_CV, "CURVES", ICON_CURVES_DATA, "Curves", ""},
{ID_VF, "FONT", ICON_FONT_DATA, "Font", ""},
#ifdef WITH_GREASE_PENCIL_V3

Should not be ifdef'd, and missing entry for the new GP RNA type

Should not be ifdef'd, and missing entry for the new GP RNA type
@ -485,2 +489,4 @@
case ID_GD_LEGACY:
return &RNA_GreasePencil;
case ID_GP:
# ifdef WITH_GREASE_PENCIL_V3

Should not be ifdef'd

Should not be ifdef'd
Falk David added 2 commits 2023-05-26 14:56:05 +02:00
Falk David added 1 commit 2023-05-26 14:56:37 +02:00
Falk David added 2 commits 2023-05-30 10:42:14 +02:00
Falk David merged commit 3aaacd6e30 into main 2023-05-30 11:14:22 +02:00
Falk David deleted branch grease-pencil-v3 2023-05-30 11:14:23 +02:00
Falk David modified the project from (deleted) to Grease Pencil 2023-06-02 18:01:46 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
8 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#106848
No description provided.