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

Initial Grease Pencil 3.0 stage #106848

Merged
Falk David merged 224 commits from filedescriptor/blender:grease-pencil-v3 into main 2023-05-30 11:14:22 +02:00
Conversation 8 Commits 224 Files Changed 94 +534 -854
6 changed files with 534 additions and 854 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e80fc2ad39 - Show all commits

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

@ -21,27 +21,55 @@ namespace blender::bke {
namespace greasepencil {
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:38:49 +02:00
Outdated
Review
Copy Link

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

Decide on the namespace here. Should it maybe be just `gp`?
/**
* A single point for a stroke that is currently being drawn.
*/
struct StrokePoint {
float3 position;
float radius;
float opacity;
float4 color;
};
/**
* Stroke cache for a stroke that is currently being drawn.
*/
struct StrokeCache {
Vector<StrokePoint> points;
Vector<uint3> triangles;
Hans Goudey commented 2023-05-03 16:08:56 +02:00
Outdated
Review
Copy Link

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?
Falk David commented 2023-05-04 11:23:10 +02:00
Outdated
Review
Copy Link

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.
int mat = 0;
void clear()
{
this->points.clear_and_shrink();
this->triangles.clear_and_shrink();
this->mat = 0;
}
};
class DrawingRuntime {
Hans Goudey commented 2023-05-02 23:30:37 +02:00
Outdated
Review
Copy Link

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

Is defining these as `constexpr` helpful? I sort of doubt any real computation is done on these nodes at compile time. But maybe?
Hans Goudey commented 2023-05-03 16:08:15 +02:00
Outdated
Review
Copy Link

The const in the const bool return type means nothing here

The `const` in the `const bool` return type means nothing here
public:
/**
* Triangle cache for all the strokes in the drawing.
*/
mutable SharedCache<Vector<uint3>> triangles_cache;
StrokeCache stroke_cache;
};
class LayerGroup;
class Layer;
/**
* A TreeNode represents one node in the layer tree.
* It can either be a layer or a group. The node has zero children if it is a layer or zero or more
* It can either be a layer or a group. The node has zero children if it is a layer or zero or
more
* children if it is a group.
*/
class TreeNode : public ::GreasePencilLayerTreeNode, NonMovable {
friend class LayerGroup;
class TreeNode : public ::GreasePencilLayerTreeNode {
public:
explicit TreeNode(GreasePencilLayerTreeNodeType type);
explicit TreeNode(GreasePencilLayerTreeNodeType type, StringRefNull name);
TreeNode(const TreeNode &other);
TreeNode &operator=(const TreeNode &other) = delete;
virtual ~TreeNode();
private:
LayerGroup *parent_ = nullptr;
Vector<std::unique_ptr<TreeNode>> children_;
Falk David commented 2023-04-12 10:39:29 +02:00
Outdated
Review
Copy Link

Try putting this class out of line.

Try putting this class out of line.
public:
/**
@ -96,12 +124,8 @@ class LayerMask : public ::GreasePencilLayerMask {
~LayerMask();
};
/**
* A layer maps drawings to scene frames. It can be thought of as one independent channel in the
* timeline.
*/
class Layer : public TreeNode, public ::GreasePencilLayer {
private:
class LayerRuntime {
public:
/**
* This Map maps a scene frame number (key) to a GreasePencilFrame. This struct holds an index
* (drawing_index) to the drawing in the GreasePencil->drawings array. The frame number indicates
@ -137,7 +161,13 @@ class Layer : public TreeNode, public ::GreasePencilLayer {
* A layer can have zero or more layer masks.
*/
Vector<LayerMask> masks_;
Falk David commented 2023-04-12 10:39:55 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
};
/**
Hans Goudey commented 2023-05-03 16:11:22 +02:00
Outdated
Review
Copy Link

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

The function is "is_locked" and the comment says "return if it is locked". This sort of comment doesn't add anything IMO, just wastes space. If there is a comment, maybe it should use a different word besides "locked" to describe the state. Or there doesn't need to be a comment at all.
* A layer maps drawings to scene frames. It can be thought of as one independent channel in the
* timeline.
*/
class Layer : public ::GreasePencilLayer {
public:
Layer();
explicit Layer(StringRefNull name);
@ -153,8 +183,8 @@ class Layer : public TreeNode, public ::GreasePencilLayer {
/**
* \returns the layer masks.
*/
Span<LayerMask> masks() const;
Vector<LayerMask> &masks_for_write();
// Span<LayerMask> masks() const;
// Vector<LayerMask> &masks_for_write();
bool is_visible() const;
bool is_locked() const;
@ -192,51 +222,56 @@ class Layer : public TreeNode, public ::GreasePencilLayer {
void tag_frames_map_keys_changed();
Hans Goudey commented 2023-04-25 05:02:57 +02:00
Outdated
Review
Copy Link

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
Sergey Sharybin commented 2023-04-25 09:37:10 +02:00
Outdated
Review
Copy Link

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.
Falk David commented 2023-04-25 17:01:57 +02:00
Outdated
Review
Copy Link

I think emplace_layer would be a better name indeed.

I think `emplace_layer` would be a better name indeed.
};
/**
* A LayerGroup is a grouping of zero or more Layers.
*/
class LayerGroup : public TreeNode {
class LayerGroupRuntime {
public:
LayerGroup() : TreeNode(GP_LAYER_TREE_GROUP) {}
explicit LayerGroup(const StringRefNull name) : TreeNode(GP_LAYER_TREE_GROUP, name) {}
LayerGroup(const LayerGroup &other);
private:
/**
* CacheMutex for `children_cache_` and `layer_cache_`;
* CacheMutex for `nodes_cache_` and `layer_cache_`;
*/
mutable CacheMutex children_cache_mutex_;
mutable CacheMutex nodes_cache_mutex_;
/**
* Caches all the children of this group in a single pre-ordered vector.
* Caches all the nodes of this group in a single pre-ordered vector.
*/
mutable Vector<TreeNode *> children_cache_;
mutable Vector<TreeNode *> nodes_cache_;
/**
* Caches all the layers in this group in a single pre-ordered vector.
*/
mutable Vector<Layer *> layer_cache_;
};
/**
* A LayerGroup is a grouping of zero or more Layers.
*/
class LayerGroup : public ::GreasePencilLayerTreeGroup {
public:
LayerGroup();
explicit LayerGroup(const StringRefNull name);
LayerGroup(const LayerGroup &other);
~LayerGroup();
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:41:05 +02:00
Outdated
Review
Copy Link

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.
public:
/**
* Adds a group at the end of this group.
*/
void add_group(LayerGroup &group);
void add_group(LayerGroup &&group);
LayerGroup &add_group(LayerGroup *group);
LayerGroup &add_group(StringRefNull name);
// void add_group(LayerGroup &&group);
/**
* Adds a layer at the end of this group and returns it.
*/
Layer &add_layer(Layer &layer);
Layer &add_layer(Layer &&layer);
Layer &add_layer(Layer *layer);
Layer &add_layer(StringRefNull name);
// Layer &add_layer(Layer &&layer);
/**
* Returns the number of direct children in this group.
* Returns the number of direct nodes in this group.
*/
int64_t num_direct_children() const;
int64_t num_direct_nodes() const;
/**
* Returns the total number of children in this group.
* Returns the total number of nodes in this group.
*/
int64_t num_children_total() const;
int64_t num_nodes_total() const;
/**
Bastien Montagne commented 2023-05-06 18:45:34 +02:00
Outdated
Review
Copy Link

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`.
* Removes a child from the group by index.
@ -246,8 +281,8 @@ class LayerGroup : public TreeNode {
/**
* Returns a `Span` of pointers to all the `TreeNode`s in this group.
*/
Span<const TreeNode *> children() const;
Span<TreeNode *> children_for_write();
Span<const TreeNode *> nodes() const;
Span<TreeNode *> nodes_for_write();
/**
* Returns a `Span` of pointers to all the `Layers`s in this group.
@ -256,13 +291,13 @@ class LayerGroup : public TreeNode {
Span<Layer *> layers_for_write();
/**
* Print the children. For debugging purposes.
* Print the nodes. For debugging purposes.
*/
void print_children(StringRefNull header) const;
void print_nodes(StringRefNull header) const;
Hans Goudey commented 2023-05-02 23:36:18 +02:00
Outdated
Review
Copy Link

= {} 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
Falk David commented 2023-04-12 10:41:37 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
private:
void ensure_children_cache() const;
void tag_children_cache_dirty() const;
void ensure_nodes_cache() const;
void tag_nodes_cache_dirty() const;
};
namespace convert {
Hans Goudey commented 2023-04-25 04:50:40 +02:00
Outdated
Review
Copy Link

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

Looks like this should be private maybe? It has a `_` suffix.
@ -273,72 +308,40 @@ void legacy_gpencil_to_grease_pencil(Main &main, GreasePencil &grease_pencil, bG
} // namespace convert
Falk David commented 2023-04-12 10:41:41 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
/**
* A single point for a stroke that is currently being drawn.
*/
struct StrokePoint {
float3 position;
float radius;
float opacity;
float4 color;
};
/**
* Stroke cache for a stroke that is currently being drawn.
*/
struct StrokeCache {
Vector<StrokePoint> points;
Vector<uint3> triangles;
int mat = 0;
void clear()
{
this->points.clear_and_shrink();
this->triangles.clear_and_shrink();
this->mat = 0;
}
};
} // namespace greasepencil
class GreasePencilDrawingRuntime {
public:
/**
* Triangle cache for all the strokes in the drawing.
*/
mutable SharedCache<Vector<uint3>> triangles_cache;
greasepencil::StrokeCache stroke_cache;
};
class GreasePencilRuntime {
public:
void *batch_cache = nullptr;
private:
greasepencil::LayerGroup root_group_;
int active_layer_index_ = -1;
public:
GreasePencilRuntime() {}
GreasePencilRuntime(const GreasePencilRuntime &other);
public:
const greasepencil::LayerGroup &root_group() const;
greasepencil::LayerGroup &root_group_for_write();
Span<const greasepencil::Layer *> layers() const;
Span<greasepencil::Layer *> layers_for_write();
bool has_active_layer() const;
const greasepencil::Layer &active_layer() const;
greasepencil::Layer &active_layer_for_write();
void set_active_layer_index(int index);
int active_layer_index() const;
// bool has_active_layer() const;
// const greasepencil::Layer &active_layer() const;
// greasepencil::Layer &active_layer_for_write();
Falk David commented 2023-04-12 10:41:45 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
// void set_active_layer_index(int index);
// int active_layer_index() const;
};
filedescriptor marked this conversation as resolved Outdated
Bastien Montagne commented 2023-05-24 18:40:29 +02:00
Outdated
Review
Copy Link

Same remark as above about commented out code.

Same remark as above about commented out code.
} // namespace blender::bke
Hans Goudey commented 2023-04-25 04:52:58 +02:00
Outdated
Review
Copy Link

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.
inline blender::bke::greasepencil::Layer &GreasePencilLayer::wrap()
{
return *reinterpret_cast<blender::bke::greasepencil::Layer *>(this);
}
inline const blender::bke::greasepencil::Layer &GreasePencilLayer::wrap() const
Hans Goudey commented 2023-04-25 04:53:30 +02:00
Outdated
Review
Copy Link

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
{
return *reinterpret_cast<const blender::bke::greasepencil::Layer *>(this);
}
inline blender::bke::greasepencil::LayerGroup &GreasePencilLayerTreeGroup::wrap()
{
return *reinterpret_cast<blender::bke::greasepencil::LayerGroup *>(this);
}
inline const blender::bke::greasepencil::LayerGroup &GreasePencilLayerTreeGroup::wrap() const
{
return *reinterpret_cast<const blender::bke::greasepencil::LayerGroup *>(this);
}
struct Main;
struct Depsgraph;
struct BoundBox;
Falk David commented 2023-04-12 10:41:54 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.

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

File diff suppressed because it is too large Load Diff

66
source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc
View File

@ -19,12 +19,13 @@
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:53:05 +02:00
Outdated
Review
Copy Link

legacy_gpencil_frame_to_grease_pencil_drawing

`legacy_gpencil_frame_to_grease_pencil_drawing`
namespace blender::bke::greasepencil::convert {
void legacy_gpencil_frame_to_grease_pencil_drawing(const bGPDframe &gpf, GreasePencilDrawing &r_drawing)
void legacy_gpencil_frame_to_grease_pencil_drawing(const bGPDframe &gpf,
Hans Goudey commented 2023-05-02 23:41:46 +02:00
Outdated
Review
Copy Link

Order return argument last, make the gpf argument const

Order return argument last, make the `gpf` argument const
GreasePencilDrawing &r_drawing)
{
/* Construct an empty CurvesGeometry in-place. */
new (&r_drawing.geometry) CurvesGeometry();
r_drawing.base.type = GP_DRAWING;
r_drawing.runtime = MEM_new<bke::GreasePencilDrawingRuntime>(__func__);
r_drawing.runtime = MEM_new<bke::greasepencil::DrawingRuntime>(__func__);
/* Get the number of points, number of strokes and the offsets for each stroke. */
Vector<int> offsets;
@ -187,52 +188,35 @@ void legacy_gpencil_to_grease_pencil(Main &bmain, GreasePencil &grease_pencil, b
MEM_cnew_array<GreasePencilDrawing *>(num_drawings, __func__));
int i = 0, layer_idx = 0;
int active_layer_index = 0;
LayerGroup &root_goup = grease_pencil.root_group.wrap();
LISTBASE_FOREACH_INDEX (bGPDlayer *, gpl, &gpd.layers, layer_idx) {
if ((gpl->flag & GP_LAYER_ACTIVE) != 0) {
active_layer_index = layer_idx;
}
/* Create a new layer. */
Layer &new_layer = grease_pencil.root_group_for_write().add_layer(
Layer(StringRefNull(gpl->info, BLI_strnlen(gpl->info, 128))));
Layer &new_layer = root_goup.add_layer(StringRefNull(gpl->info, BLI_strnlen(gpl->info, 128)));
/* Flags. */
SET_FLAG_FROM_TEST(new_layer.flag, (gpl->flag & GP_LAYER_HIDE), GP_LAYER_TREE_NODE_HIDE);
SET_FLAG_FROM_TEST(new_layer.flag, (gpl->flag & GP_LAYER_LOCKED), GP_LAYER_TREE_NODE_LOCKED);
SET_FLAG_FROM_TEST(new_layer.flag, (gpl->flag & GP_LAYER_SELECT), GP_LAYER_TREE_NODE_SELECT);
SET_FLAG_FROM_TEST(new_layer.flag, (gpl->flag & GP_LAYER_FRAMELOCK), GP_LAYER_TREE_NODE_MUTE);
SET_FLAG_FROM_TEST(new_layer.base.flag, (gpl->flag & GP_LAYER_HIDE), GP_LAYER_TREE_NODE_HIDE);
SET_FLAG_FROM_TEST(
new_layer.flag, (gpl->flag & GP_LAYER_USE_LIGHTS), GP_LAYER_TREE_NODE_USE_LIGHTS);
SET_FLAG_FROM_TEST(new_layer.flag,
new_layer.base.flag, (gpl->flag & GP_LAYER_LOCKED), GP_LAYER_TREE_NODE_LOCKED);
SET_FLAG_FROM_TEST(
new_layer.base.flag, (gpl->flag & GP_LAYER_SELECT), GP_LAYER_TREE_NODE_SELECT);
SET_FLAG_FROM_TEST(
new_layer.base.flag, (gpl->flag & GP_LAYER_FRAMELOCK), GP_LAYER_TREE_NODE_MUTE);
SET_FLAG_FROM_TEST(
new_layer.base.flag, (gpl->flag & GP_LAYER_USE_LIGHTS), GP_LAYER_TREE_NODE_USE_LIGHTS);
SET_FLAG_FROM_TEST(new_layer.base.flag,
(gpl->onion_flag & GP_LAYER_ONIONSKIN),
GP_LAYER_TREE_NODE_USE_ONION_SKINNING);
new_layer.parent_type = static_cast<int8_t>(gpl->partype);
new_layer.blend_mode = static_cast<int8_t>(gpl->blend_mode);
new_layer.thickness_adjustment = static_cast<int16_t>(gpl->line_change);
/* Copy the pointer to the parent. */
new_layer.parent = gpl->parent;
size_t parsubstr_len = BLI_strnlen(gpl->parsubstr, MAX_ID_NAME - 2);
if (parsubstr_len > 0) {
new_layer.parsubstr = BLI_strdupn(gpl->parsubstr, parsubstr_len);
}
size_t viewlayername_len = BLI_strnlen(gpl->viewlayername, MAX_ID_NAME - 2);
if (viewlayername_len > 0) {
new_layer.viewlayer_name = BLI_strdupn(gpl->viewlayername, viewlayername_len);
}
/* Convert the layer masks. */
Vector<LayerMask> masks = new_layer.masks_for_write();
LISTBASE_FOREACH (bGPDlayer_Mask *, mask, &gpl->mask_layers) {
LayerMask new_mask = LayerMask(mask->name);
new_mask.flag = mask->flag;
masks.append(std::move(new_mask));
}
new_layer.opacity = gpl->opacity;
copy_v4_v4(new_layer.tint_color, gpl->tintcolor);
copy_v3_v3(new_layer.location, gpl->location);
copy_v3_v3(new_layer.rotation, gpl->rotation);
copy_v3_v3(new_layer.scale, gpl->scale);
// Vector<LayerMask> masks = new_layer.masks_for_write();
// LISTBASE_FOREACH (bGPDlayer_Mask *, mask, &gpl->mask_layers) {
// LayerMask new_mask = LayerMask(mask->name);
// new_mask.flag = mask->flag;
// masks.append(std::move(new_mask));
// }
// new_layer.opacity = gpl->opacity;
LISTBASE_FOREACH (bGPDframe *, gpf, &gpl->frames) {
grease_pencil.drawing_array[i] = reinterpret_cast<GreasePencilDrawingBase *>(
@ -251,6 +235,10 @@ void legacy_gpencil_to_grease_pencil(Main &bmain, GreasePencil &grease_pencil, b
i++;
}
if ((gpl->flag & GP_LAYER_ACTIVE) != 0) {
grease_pencil.active_layer = static_cast<GreasePencilLayer *>(&new_layer);
}
/* TODO: Update drawing user counts. */
new_layer.tag_frames_map_keys_changed();
@ -270,7 +258,7 @@ void legacy_gpencil_to_grease_pencil(Main &bmain, GreasePencil &grease_pencil, b
copy_v3_v3(grease_pencil.onion_skinning_settings.color_before, gpd.gcolor_prev);
copy_v3_v3(grease_pencil.onion_skinning_settings.color_after, gpd.gcolor_next);
grease_pencil.runtime->set_active_layer_index(active_layer_index);
// grease_pencil.runtime->set_active_layer_index(active_layer_index);
BKE_id_materials_copy(&bmain, &gpd.id, &grease_pencil.id);
}

128
source/blender/blenkernel/intern/grease_pencil_test.cc
View File

@ -38,78 +38,40 @@ TEST(greasepencil, create_grease_pencil_id)
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(BKE_id_new(ctx.bmain, ID_GP, "GP"));
EXPECT_EQ(grease_pencil.drawings().size(), 0);
EXPECT_EQ(grease_pencil.root_group().num_children_total(), 0);
EXPECT_EQ(grease_pencil.root_group.wrap().num_nodes_total(), 0);
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:52:05 +02:00
Outdated
Review
Copy Link

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`.
}
TEST(greasepencil, save_layer_tree_to_storage)
{
GreasePencilIDTestContext ctx;
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(BKE_id_new(ctx.bmain, ID_GP, "GP"));
// TEST(greasepencil, set_active_layer_index)
// {
// GreasePencilIDTestContext ctx;
StringRefNull names[7] = {"Group1", "Layer1", "Layer2", "Group2", "Layer3", "Layer4", "Layer5"};
// GreasePencil &grease_pencil = *static_cast<GreasePencil *>(BKE_id_new(ctx.bmain, ID_GP,
// "GP")); grease_pencil.add_empty_drawings(3);
LayerGroup group(names[0]);
group.add_layer(Layer(names[1]));
group.add_layer(Layer(names[2]));
// const Layer &layer1 = grease_pencil.root_group.wrap().add_layer("Layer1");
// const Layer &layer2 = grease_pencil.root_group.wrap().add_layer("Layer2");
LayerGroup group2(names[3]);
group2.add_layer(Layer(names[4]));
group2.add_layer(Layer(names[5]));
// grease_pencil.runtime->set_active_layer_index(0);
// EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
// EXPECT_STREQ(layer1.name, grease_pencil.runtime->active_layer().name);
group.add_group(std::move(group2));
grease_pencil.root_group_for_write().add_group(std::move(group));
grease_pencil.root_group_for_write().add_layer(Layer(names[6]));
// grease_pencil.runtime->set_active_layer_index(1);
// EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
// EXPECT_STREQ(layer2.name, grease_pencil.runtime->active_layer().name);
/* Save to storage. */
grease_pencil.free_layer_tree_storage();
grease_pencil.save_layer_tree_to_storage();
MEM_delete(grease_pencil.runtime);
// /* Save to storage. */
// grease_pencil.free_layer_tree_storage();
// grease_pencil.save_layer_tree_to_storage();
// MEM_delete(grease_pencil.runtime);
/* Load from storage. */
grease_pencil.runtime = MEM_new<blender::bke::GreasePencilRuntime>(__func__);
grease_pencil.load_layer_tree_from_storage();
// /* Load from storage. */
// grease_pencil.runtime = MEM_new<blender::bke::GreasePencilRuntime>(__func__);
// grease_pencil.load_layer_tree_from_storage();
Span<const TreeNode *> children = grease_pencil.root_group().children();
for (const int i : children.index_range()) {
const TreeNode &child = *children[i];
EXPECT_STREQ(child.name, names[i].data());
}
}
TEST(greasepencil, set_active_layer_index)
{
GreasePencilIDTestContext ctx;
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(BKE_id_new(ctx.bmain, ID_GP, "GP"));
grease_pencil.add_empty_drawings(3);
Layer layer1("Layer1");
Layer layer2("Layer2");
const Layer &layer1_ref = grease_pencil.root_group_for_write().add_layer(std::move(layer1));
const Layer &layer2_ref = grease_pencil.root_group_for_write().add_layer(std::move(layer2));
grease_pencil.runtime->set_active_layer_index(0);
EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
EXPECT_STREQ(layer1_ref.name, grease_pencil.runtime->active_layer().name);
grease_pencil.runtime->set_active_layer_index(1);
EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
EXPECT_STREQ(layer2_ref.name, grease_pencil.runtime->active_layer().name);
/* Save to storage. */
grease_pencil.free_layer_tree_storage();
grease_pencil.save_layer_tree_to_storage();
MEM_delete(grease_pencil.runtime);
/* Load from storage. */
grease_pencil.runtime = MEM_new<blender::bke::GreasePencilRuntime>(__func__);
grease_pencil.load_layer_tree_from_storage();
/* Check if the active layer is still the second one. */
EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
EXPECT_STREQ(grease_pencil.runtime->active_layer().name, "Layer2");
}
// /* Check if the active layer is still the second one. */
// EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
// EXPECT_STREQ(grease_pencil.runtime->active_layer().name, "Layer2");
// }
/* --------------------------------------------------------------------------------------------- */
/* Drawing Array Tests. */
@ -132,8 +94,8 @@ TEST(greasepencil, remove_drawing)
grease_pencil.drawings_for_write()[1]);
drawing->geometry.wrap().resize(0, 10);
Layer layer1("Layer1");
Layer layer2("Layer2");
Layer &layer1 = grease_pencil.root_group.wrap().add_layer("Layer1");
Layer &layer2 = grease_pencil.root_group.wrap().add_layer("Layer2");
layer1.insert_frame(0, GreasePencilFrame{0});
layer1.insert_frame(10, GreasePencilFrame{1});
@ -143,9 +105,6 @@ TEST(greasepencil, remove_drawing)
layer2.insert_frame(0, GreasePencilFrame{1});
layer2.tag_frames_map_keys_changed();
grease_pencil.root_group_for_write().add_layer(std::move(layer1));
grease_pencil.root_group_for_write().add_layer(std::move(layer2));
grease_pencil.remove_drawing(1);
EXPECT_EQ(grease_pencil.drawings().size(), 2);
@ -179,17 +138,16 @@ TEST(greasepencil, overwrite_frame)
TEST(greasepencil, layer_tree_empty)
{
LayerGroup root{};
LayerGroup root;
}
TEST(greasepencil, layer_tree_build_simple)
{
LayerGroup root{};
LayerGroup root;
LayerGroup group("Group1");
group.add_layer(Layer("Layer1"));
group.add_layer(Layer("Layer2"));
root.add_group(std::move(group));
LayerGroup &group = root.add_group("Group1");
group.add_layer("Layer1");
group.add_layer("Layer2");
}
struct GreasePencilLayerTreeExample {
@ -199,17 +157,15 @@ struct GreasePencilLayerTreeExample {
GreasePencilLayerTreeExample()
{
LayerGroup group(names[0]);
group.add_layer(Layer(names[1]));
group.add_layer(Layer(names[2]));
LayerGroup &group = root.add_group(names[0]);
group.add_layer(names[1]);
group.add_layer(names[2]);
LayerGroup group2(names[3]);
group2.add_layer(Layer(names[4]));
group2.add_layer(Layer(names[5]));
LayerGroup &group2 = group.add_group(names[3]);
group2.add_layer(names[4]);
group2.add_layer(names[5]);
group.add_group(std::move(group2));
root.add_group(std::move(group));
root.add_layer(Layer(names[6]));
root.add_layer(names[6]);
}
};
@ -217,7 +173,7 @@ TEST(greasepencil, layer_tree_pre_order_iteration)
{
GreasePencilLayerTreeExample ex;
Span<const TreeNode *> children = ex.root.children();
Span<const TreeNode *> children = ex.root.nodes();
for (const int i : children.index_range()) {
const TreeNode &child = *children[i];
EXPECT_STREQ(child.name, ex.names[i].data());
@ -227,13 +183,13 @@ TEST(greasepencil, layer_tree_pre_order_iteration)
TEST(greasepencil, layer_tree_total_size)
{
GreasePencilLayerTreeExample ex;
EXPECT_EQ(ex.root.num_children_total(), 7);
EXPECT_EQ(ex.root.num_nodes_total(), 7);
}
TEST(greasepencil, layer_tree_node_types)
{
GreasePencilLayerTreeExample ex;
Span<const TreeNode *> children = ex.root.children();
Span<const TreeNode *> children = ex.root.nodes();
for (const int i : children.index_range()) {
const TreeNode &child = *children[i];
EXPECT_EQ(child.is_layer(), ex.is_layer[i]);

17
source/blender/editors/sculpt_paint/grease_pencil_paint.cc
View File

@ -52,11 +52,12 @@ struct PaintOperationExecutor {
* original object.
*/
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(ob_eval->data);
if (!grease_pencil.runtime->has_active_layer()) {
if (grease_pencil.active_layer != NULL) {
/* TODO: create a new layer. */
grease_pencil.runtime->set_active_layer_index(0);
BLI_assert_unreachable();
// grease_pencil.runtime->set_active_layer_index(0);
}
const bke::greasepencil::Layer &active_layer = grease_pencil.runtime->active_layer();
const bke::greasepencil::Layer &active_layer = grease_pencil.active_layer->wrap();
int index = active_layer.drawing_index_at(scene->r.cfra);
BLI_assert(index != -1);
@ -67,7 +68,8 @@ struct PaintOperationExecutor {
float3 proj_pos;
ED_view3d_win_to_3d_on_plane(region, plane, stroke_extension.mouse_position, false, proj_pos);
bke::greasepencil::StrokePoint new_point{proj_pos, stroke_extension.pressure * 100.0f, 1.0f, float4(1.0f)};
bke::greasepencil::StrokePoint new_point{
proj_pos, stroke_extension.pressure * 100.0f, 1.0f, float4(1.0f)};
drawing.runtime->stroke_cache.points.append(std::move(new_point));
@ -91,10 +93,9 @@ void PaintOperation::on_stroke_done(const bContext &C)
GreasePencil &grease_pencil_orig = *static_cast<GreasePencil *>(obact->data);
GreasePencil &grease_pencil_eval = *static_cast<GreasePencil *>(ob_eval->data);
BLI_assert(grease_pencil_orig.runtime->has_active_layer() &&
grease_pencil_eval.runtime->has_active_layer());
const bke::greasepencil::Layer &active_layer_orig = grease_pencil_orig.runtime->active_layer();
const bke::greasepencil::Layer &active_layer_eval = grease_pencil_eval.runtime->active_layer();
BLI_assert(grease_pencil_orig.active_layer != NULL && grease_pencil_eval.active_layer != NULL);
const bke::greasepencil::Layer &active_layer_orig = grease_pencil_orig.active_layer->wrap();
const bke::greasepencil::Layer &active_layer_eval = grease_pencil_eval.active_layer->wrap();
int index_orig = active_layer_orig.drawing_index_at(scene->r.cfra);
int index_eval = active_layer_eval.drawing_index_at(scene->r.cfra);
BLI_assert(index_orig != -1 && index_eval != -1);

173
source/blender/makesdna/DNA_grease_pencil_types.h
View File

@ -9,6 +9,7 @@
#include "DNA_ID.h"
#include "DNA_curves_types.h"
#include "DNA_listBase.h"
#ifdef __cplusplus
# include "BLI_function_ref.hh"
@ -19,16 +20,23 @@ namespace blender::bke {
class GreasePencilRuntime;
class GreasePencilDrawingRuntime;
namespace greasepencil {
class DrawingRuntime;
class Layer;
class LayerRuntime;
class LayerGroup;
class LayerGroupRuntime;
struct StrokePoint;
} // namespace greasepencil
} // namespace blender::bke
using GreasePencilRuntimeHandle = blender::bke::GreasePencilRuntime;
using GreasePencilDrawingRuntimeHandle = blender::bke::GreasePencilDrawingRuntime;
using GreasePencilDrawingRuntimeHandle = blender::bke::greasepencil::DrawingRuntime;
using GreasePencilLayerRuntimeHandle = blender::bke::greasepencil::LayerRuntime;
using GreasePencilLayerGroupRuntimeHandle = blender::bke::greasepencil::LayerGroupRuntime;
#else
typedef struct GreasePencilRuntimeHandle GreasePencilRuntimeHandle;
typedef struct GreasePencilDrawingRuntimeHandle GreasePencilDrawingRuntimeHandle;
typedef struct GreasePencilLayerRuntimeHandle GreasePencilLayerRuntimeHandle;
typedef struct GreasePencilLayerGroupRuntimeHandle GreasePencilLayerGroupRuntimeHandle;
#endif
#ifdef __cplusplus
@ -172,6 +180,7 @@ typedef enum GreasePencilLayerMaskFlag {
* A grease pencil layer mask stores the name of a layer that is the mask.
*/
typedef struct GreasePencilLayerMask {
struct GreasePencilLayerMask *next, *prev;
Hans Goudey commented 2023-04-25 04:32:41 +02:00
Outdated
Review
Copy Link

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
Sergey Sharybin commented 2023-04-25 10:10:22 +02:00
Outdated
Review
Copy Link

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.
Hans Goudey commented 2023-04-25 14:52:41 +02:00
Outdated
Review
Copy Link

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.
/**
* The name of the layer that is the mask.
*/
@ -183,23 +192,6 @@ typedef struct GreasePencilLayerMask {
char _pad[6];
} GreasePencilLayerMask;
/**
* Storage for the layer masks.
*/
typedef struct GreasePencilLayerMaskStorage {
GreasePencilLayerMask *masks;
int masks_size;
char _pad[4];
} GreasePencilLayerMaskStorage;
/**
* Type of parent of a layer. #GreasePencilLayer.parent_type
*/
typedef enum GreasePencilLayerParentType {
GP_LAYER_PARENT_OBJECT = 0,
GP_LAYER_PARENT_BONE = 1,
} GreasePencilLayerParentType;
/**
* Layer blending modes. #GreasePencilLayer.blend_mode
*/
@ -212,57 +204,6 @@ typedef enum GreasePencilLayerBlendMode {
GP_LAYER_BLEND_DIVIDE = 5,
} GreasePencilLayerBlendMode;
/**
* A grease pencil layer is a collection of drawings mapped to a specific time on the timeline.
*/
typedef struct GreasePencilLayer {
/* Only used for storage in the .blend file. */
GreasePencilLayerFramesMapStorage frames_storage;
/**
* Layer parent type. See `GreasePencilLayerParentType`.
*/
int8_t parent_type;
/**
* Layer blend mode. See `GreasePencilLayerBlendMode`.
*/
int8_t blend_mode;
/**
* Thickness adjustment of all strokes in this layer.
*/
int16_t thickness_adjustment;
char _pad[4];
/**
* Parent object. Can be nullptr.
*/
struct Object *parent;
/**
* Parent sub-object info. E.g. the name of a bone if the parent is an armature.
*/
char *parsubstr;
/**
* Name of a view layer. If used, the layer is only rendered on the specified view layer. Not
* used for viewport rendering.
*/
char *viewlayer_name;
/**
* Only used for storage in the .blend file.
*/
GreasePencilLayerMaskStorage masks_storage;
/**
* Opacity of the layer.
*/
float opacity;
/**
* Color used to tint the layer. Alpha value is used as a factor.
*/
float tint_color[4];
/**
* Layer transform.
* \note rotation is in euler format.
*/
float location[3], rotation[3], scale[3];
} GreasePencilLayer;
/**
* Type of layer node.
* If `GP_LAYER_TREE_LEAF` the node is a `GreasePencilLayerTreeLeaf`,
@ -285,7 +226,16 @@ typedef enum GreasePencilLayerTreeNodeFlag {
GP_LAYER_TREE_NODE_USE_ONION_SKINNING = (1 << 5),
} GreasePencilLayerTreeNodeFlag;
struct GreasePencilLayerTreeGroup;
typedef struct GreasePencilLayerTreeNode {
/* ListBase pointers. */
struct GreasePencilLayerTreeNode *next, *prev;
/* Parent pointer. Can be null. */
struct GreasePencilLayerTreeGroup *parent;
/**
* Name of the layer/group. Dynamic length.
*/
char *name;
/**
* One of `GreasePencilLayerTreeNodeType`.
* Indicates the type of struct this element is.
@ -300,32 +250,54 @@ typedef struct GreasePencilLayerTreeNode {
* See `GreasePencilLayerTreeNodeFlag`.
Hans Goudey commented 2023-04-25 05:06:34 +02:00
Outdated
Review
Copy Link

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.
*/
uint32_t flag;
/**
* Name of the layer/group. Dynamic length.
*/
char *name;
} GreasePencilLayerTreeNode;
/**
Falk David commented 2023-04-18 16:10:24 +02:00
Outdated
Review
Copy Link

typo transform

typo `transform`
* A grease pencil layer is a collection of drawings mapped to a specific time on the timeline.
*/
typedef struct GreasePencilLayer {
GreasePencilLayerTreeNode base;
/* Only used for storage in the .blend file. */
GreasePencilLayerFramesMapStorage frames_storage;
/**
* Layer blend mode. See `GreasePencilLayerBlendMode`.
*/
int8_t blend_mode;
char _pad[3];
/**
* Opacity of the layer.
*/
float opacity;
/**
* List of `GreasePencilLayerMask`.
*/
ListBase masks;
/**
* Runtime struct pointer.
*/
GreasePencilLayerRuntimeHandle *runtime;
#ifdef __cplusplus
blender::bke::greasepencil::Layer &wrap();
const blender::bke::greasepencil::Layer &wrap() const;
#endif
} GreasePencilLayer;
typedef struct GreasePencilLayerTreeGroup {
GreasePencilLayerTreeNode base;
int children_num;
char _pad[4];
/**
* List of `GreasePencilLayerTreeNode`.
*/
ListBase children;
/**
* Runtime struct pointer.
*/
GreasePencilLayerGroupRuntimeHandle *runtime;
#ifdef __cplusplus
blender::bke::greasepencil::LayerGroup &wrap();
const blender::bke::greasepencil::LayerGroup &wrap() const;
#endif
} GreasePencilLayerTreeGroup;
typedef struct GreasePencilLayerTreeLeaf {
GreasePencilLayerTreeNode base;
GreasePencilLayer layer;
} GreasePencilLayerTreeLeaf;
/* Only used for storage in the .blend file. */
typedef struct GreasePencilLayerTreeStorage {
/* Array of tree nodes. Pre-order serialization of the layer tree. */
GreasePencilLayerTreeNode **nodes;
int nodes_num;
/* Index pointing to the node in the array that is the active layer. */
int active_layer_index;
} GreasePencilLayerTreeStorage;
/**
* Flag for the grease pencil data-block. #GreasePencil.flag
*/
@ -414,8 +386,14 @@ typedef struct GreasePencil {
int drawing_array_size;
char _pad[4];
/* Only used for storage in the .blend file. */
GreasePencilLayerTreeStorage layer_tree_storage;
/* Root group of the layer tree. */
GreasePencilLayerTreeGroup root_group;
/**
* Pointer to the active layer. Can be NULL.
* This pointer does not own the data.
*/
GreasePencilLayer *active_layer;
/**
* An array of materials.
@ -441,21 +419,14 @@ typedef struct GreasePencil {
void write_drawing_array(BlendWriter *writer);
void free_drawing_array();
/* GreasePencilLayerTreeStorage functions. */
void save_layer_tree_to_storage();
void load_layer_tree_from_storage();
void read_layer_tree_storage(BlendDataReader *reader);
void write_layer_tree_storage(BlendWriter *writer);
void free_layer_tree_storage();
/* Layer tree read/write functions. */
void read_layer_tree(BlendDataReader *reader);
void write_layer_tree(BlendWriter *writer);
/* Drawings read/write access. */
blender::Span<GreasePencilDrawingBase *> drawings() const;
blender::MutableSpan<GreasePencilDrawingBase *> drawings_for_write();
/* Root group read/write access. */
const blender::bke::greasepencil::LayerGroup &root_group() const;
blender::bke::greasepencil::LayerGroup &root_group_for_write();
/* Layers read/write access. */
blender::Span<const blender::bke::greasepencil::Layer *> layers() const;
blender::Span<blender::bke::greasepencil::Layer *> layers_for_write();