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
5 changed files with 182 additions and 32 deletions
Showing only changes of commit 8088c04536 - Show all commits

View File

@ -34,6 +34,7 @@ class Layer;
class TreeNode : public ::GreasePencilLayerTreeNode {
using TreeNodeIterFn = FunctionRef<void(TreeNode &)>;
using LayerIterFn = FunctionRef<void(Layer &)>;
using LayerIndexIterFn = FunctionRef<void(uint64_t, Layer &)>;
protected:

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?

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.
Vector<std::unique_ptr<TreeNode>> children_;
@ -146,6 +147,88 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
}
};
class PreOrderIndexRange {
public:
struct Item {
const int64_t index;
const TreeNode &node;
};

Better to return a span here maybe?

Better to return a span here maybe?
private:
class Iterator {
using iterator_category = std::forward_iterator_tag;
private:
int64_t current_index_;
Stack<TreeNode *> next_node_;

Put out of line.

Put out of line.
public:
explicit Iterator(TreeNode *root)

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.
{
current_index_ = 0;
if (root != nullptr) {
for (auto it = root->children_.rbegin(); it != root->children_.rend(); it++) {
next_node_.push((*it).get());
}
}
}
Item operator*()
{
return {current_index_, *next_node_.peek()};
}
Iterator &operator++()
{
BLI_assert(!next_node_.is_empty());
TreeNode &next_node = *next_node_.pop();
current_index_++;
for (auto it = next_node.children_.rbegin(); it != next_node.children_.rend(); it++) {
next_node_.push((*it).get());
}
return *this;
}
Iterator operator++(int)
{
filedescriptor marked this conversation as resolved Outdated

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.
Iterator old = *this;
operator++();
return old;
}
bool operator==(Iterator &other) const
{

Put out of line.

Put out of line.
if (next_node_.size() == 0) {
return other.next_node_.size() == 0;
}
return (next_node_.size() == other.next_node_.size()) &&
next_node_.peek() == other.next_node_.peek() &&
current_index_ == other.current_index_;
}

Put out of line.

Put out of line.
bool operator!=(Iterator &other) const
{

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.
return !(*this == other);
}
};
private:
TreeNode *_root;
public:
explicit PreOrderIndexRange(TreeNode *root) : _root(root) {}
Iterator begin()
{

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.

I think emplace_layer would be a better name indeed.

I think `emplace_layer` would be a better name indeed.
return Iterator(_root);
}
Iterator end()
{
return Iterator(nullptr);
}
};
public:
constexpr bool is_group() const
{
@ -159,6 +242,7 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
LayerGroup &as_group();
Layer &as_layer();
const Layer &as_layer() const;
int total_num_children() const
{
@ -177,10 +261,8 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
return total;
}
PreOrderRange children_in_pre_order()
{
return PreOrderRange(this);
}
PreOrderRange children_in_pre_order();
PreOrderIndexRange children_with_index_in_pre_order();
void foreach_children_pre_order(TreeNodeIterFn function)
{
@ -272,7 +354,7 @@ class Layer : public TreeNode, ::GreasePencilLayer {
return this != &other;
}
const Map<int, GreasePencilFrame> &frames()
const Map<int, GreasePencilFrame> &frames() const
{
return frames_;

This needs to be implemented.

This needs to be implemented.
}
@ -307,7 +389,7 @@ class Layer : public TreeNode, ::GreasePencilLayer {
return frames_for_write().add_overwrite(frame_number, frame);
}
Span<int> sorted_keys()
Span<int> sorted_keys() const
{
sorted_keys_cache_.ensure([&](Vector<int> &r_data) {
r_data.clear_and_shrink();
@ -330,7 +412,7 @@ class Layer : public TreeNode, ::GreasePencilLayer {
/**
* Return the index of the drawing at frame \a frame or -1 if there is no drawing.
*/
int drawing_at(int frame)
int drawing_at(int frame) const
{
Span<int> sorted_keys = this->sorted_keys();
/* Before the first drawing, return no drawing. */
@ -463,6 +545,7 @@ class GreasePencilDrawingRuntime {
class GreasePencilRuntime {
private:
LayerGroup root_group_;
const Layer *active_layer_ = nullptr;
public:
GreasePencilRuntime() {}
@ -473,6 +556,16 @@ class GreasePencilRuntime {
return root_group_;
}
const Layer *active_layer() const
{
return active_layer_;
}
void set_active_layer(const Layer *new_active_layer)
{
active_layer_ = new_active_layer;
}
public:
void *batch_cache = nullptr;
};

View File

@ -252,6 +252,16 @@ IDTypeInfo IDType_ID_GP = {
namespace blender::bke::gpencil {
TreeNode::PreOrderRange TreeNode::children_in_pre_order()
{
return TreeNode::PreOrderRange(this);
}
TreeNode::PreOrderIndexRange TreeNode::children_with_index_in_pre_order()
{
return TreeNode::PreOrderIndexRange(this);
}
LayerGroup &TreeNode::as_group()
{
return *static_cast<LayerGroup *>(this);
@ -262,6 +272,11 @@ Layer &TreeNode::as_layer()
return *static_cast<Layer *>(this);
}
const Layer &TreeNode::as_layer() const
{
return *static_cast<const Layer *>(this);
}
} // namespace blender::bke::gpencil
void *BKE_grease_pencil_add(Main *bmain, const char *name)
@ -593,17 +608,9 @@ void GreasePencil::foreach_visible_drawing(
}
}
blender::bke::gpencil::Layer *GreasePencil::get_active_layer()
const blender::bke::gpencil::Layer *GreasePencil::active_layer() const
{
using namespace blender::bke::gpencil;
/* TOOD. For now get the first layer. */
for (TreeNode &node : this->root_group().children_in_pre_order()) {
if (!node.is_layer()) {
continue;
}
return &node.as_layer();
}
return nullptr;
return this->runtime->active_layer();
}
blender::bke::gpencil::LayerGroup &GreasePencil::root_group()
@ -710,18 +717,20 @@ static void save_layer_to_storage(blender::bke::gpencil::Layer &node,
save_tree_node_to_storage(node, &new_leaf->base);
/* Save frames map. */
int frames_size = node.frames().size();
new_leaf->layer.frames_storage.size = frames_size;
new_leaf->layer.frames_storage.keys = MEM_cnew_array<int>(frames_size, __func__);
new_leaf->layer.frames_storage.values = MEM_cnew_array<GreasePencilFrame>(frames_size, __func__);
const int frames_size = node.frames().size();
if (frames_size > 0) {
new_leaf->layer.frames_storage.size = frames_size;
new_leaf->layer.frames_storage.keys = MEM_cnew_array<int>(frames_size, __func__);
new_leaf->layer.frames_storage.values = MEM_cnew_array<GreasePencilFrame>(frames_size,
__func__);
MutableSpan<int> keys{new_leaf->layer.frames_storage.keys, frames_size};
MutableSpan<GreasePencilFrame> values{new_leaf->layer.frames_storage.values, frames_size};
keys.copy_from(node.sorted_keys());
for (int i : keys.index_range()) {
values[i] = node.frames().lookup(keys[i]);
MutableSpan<int> keys{new_leaf->layer.frames_storage.keys, frames_size};
MutableSpan<GreasePencilFrame> values{new_leaf->layer.frames_storage.values, frames_size};
keys.copy_from(node.sorted_keys());
for (int i : keys.index_range()) {
values[i] = node.frames().lookup(keys[i]);
}
}
/* Store pointer. */
*dst = reinterpret_cast<GreasePencilLayerTreeNode *>(new_leaf);
}
@ -763,6 +772,13 @@ void GreasePencil::save_layer_tree_to_storage()
}
i++;
}
for (auto item : this->root_group().children_with_index_in_pre_order()) {
if (&item.node == this->runtime->active_layer()) {
this->layer_tree_storage.active_layer_index = item.index;
break;
}
}
}
static void read_layer_node_recursive(blender::bke::gpencil::LayerGroup &current_group,
@ -807,6 +823,13 @@ void GreasePencil::load_layer_tree_from_storage()
for (int i = 0; i < reinterpret_cast<GreasePencilLayerTreeGroup *>(root)->children_num; i++) {
read_layer_node_recursive(this->runtime->root_group(), this->layer_tree_storage.nodes, i + 1);
}
for (auto item : this->root_group().children_with_index_in_pre_order()) {
if (item.index == this->layer_tree_storage.active_layer_index) {
this->runtime->set_active_layer(&item.node.as_layer());
break;
}
}
}
void GreasePencil::read_layer_tree_storage(BlendDataReader *reader)

View File

@ -39,6 +39,39 @@ TEST(gpencil, create_grease_pencil_id)
EXPECT_EQ(grease_pencil.root_group().total_num_children(), 0);
}
filedescriptor marked this conversation as resolved Outdated

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(gpencil, set_active_layer)
{
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");
Layer *layer1_ref = &grease_pencil.root_group().add_layer(std::move(layer1));
Layer *layer2_ref = &grease_pencil.root_group().add_layer(std::move(layer2));
grease_pencil.runtime->set_active_layer(layer1_ref);
EXPECT_EQ(layer1_ref, grease_pencil.active_layer());
grease_pencil.runtime->set_active_layer(layer2_ref);
EXPECT_EQ(layer2_ref, grease_pencil.active_layer());
/* 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_NE(grease_pencil.active_layer(), nullptr);
EXPECT_STREQ(grease_pencil.active_layer()->name, "Layer2");
}
/* --------------------------------------------------------------------------------------------- */
/* Drawing Array Tests. */

View File

@ -45,7 +45,7 @@ struct PaintOperationExecutor {
Object *ob_eval = DEG_get_evaluated_object(depsgraph, obact);
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(ob_eval->data);
Review

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

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

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.
bke::gpencil::Layer *active_layer = grease_pencil.get_active_layer();
const bke::gpencil::Layer *active_layer = grease_pencil.active_layer();
BLI_assert(active_layer != nullptr);
int index = active_layer->drawing_at(scene->r.cfra);
BLI_assert(index != -1);
@ -81,8 +81,8 @@ 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);
bke::gpencil::Layer *active_layer_orig = grease_pencil_orig.get_active_layer();
bke::gpencil::Layer *active_layer_eval = grease_pencil_eval.get_active_layer();
const bke::gpencil::Layer *active_layer_orig = grease_pencil_orig.active_layer();
const bke::gpencil::Layer *active_layer_eval = grease_pencil_eval.active_layer();
BLI_assert(active_layer_orig != nullptr && active_layer_eval != nullptr);
int index_orig = active_layer_orig->drawing_at(scene->r.cfra);
int index_eval = active_layer_eval->drawing_at(scene->r.cfra);

View File

@ -179,7 +179,8 @@ typedef struct GreasePencilLayerTreeStorage {
/* Array of tree nodes. Pre-order serialization of the layer tree. */
GreasePencilLayerTreeNode **nodes;
int nodes_num;
char _pad[4];
/* Index pointing to the node in the array that is the active layer. */
int active_layer_index;

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.

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.
} GreasePencilLayerTreeStorage;
/**
@ -236,7 +237,7 @@ typedef struct GreasePencil {
*/
GreasePencilRuntimeHandle *runtime;
#ifdef __cplusplus
blender::bke::gpencil::Layer *get_active_layer();
const blender::bke::gpencil::Layer *active_layer() const;
blender::bke::gpencil::LayerGroup &root_group();
#endif
} GreasePencil;