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

Initial Grease Pencil 3.0 stage #106848

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

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

@ -39,7 +39,15 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
this->type = type;
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.
this->name = BLI_strdup(name.c_str());
}
TreeNode(const TreeNode &other) = delete;
TreeNode(const TreeNode &other)
{
this->type = other.type;
this->name = BLI_strdup(other.name);
children_.reserve(other.children_.size());
for (const std::unique_ptr<TreeNode> &elem : other.children_) {
children_.append(std::make_unique<TreeNode>(*elem));
}
}
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
TreeNode(TreeNode &&other) : children_(std::move(other.children_))
{
this->name = other.name;
@ -197,25 +205,13 @@ class Layer : public TreeNode, ::GreasePencilLayer {
Layer(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF, name), frames_()
{
}
Layer(const Layer &other)
: TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF, other.name), frames_(other.frames_)
Layer(const Layer &other) : TreeNode(other)
Falk David commented 2023-04-12 10:40:11 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
{
}
Bastien Montagne commented 2023-05-06 17:19:44 +02:00
Outdated
Review
Copy Link

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.
Layer(Layer &&other) : TreeNode(std::move(other))
{
frames_ = std::move(other.frames_);
}
Layer &operator=(const Layer &other)
{
return copy_assign_container(*this, other);
}
Layer &operator=(Layer &&other)
{
if (this != &other) {
frames_ = std::move(other.frames_);
}
return *this;
}
~Layer()
{
}
@ -238,7 +234,7 @@ class LayerGroup : public TreeNode {
LayerGroup(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, name)
{
}
LayerGroup(const LayerGroup &other) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, other.name)
LayerGroup(const LayerGroup &other) : TreeNode(other)
{
}
LayerGroup(LayerGroup &&other) : TreeNode(std::move(other))

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

@ -17,8 +17,13 @@ TEST(gpencil, build_layer_tree)
group.add_layer(Layer("Layer1"));
group.add_layer(Layer("Layer2"));
Sergey Sharybin commented 2023-04-25 09:39:28 +02:00
Outdated
Review
Copy Link

Use text fixtures: http://google.github.io/googletest/primer.html#same-data-multiple-tests

Use text fixtures: http://google.github.io/googletest/primer.html#same-data-multiple-tests
Falk David commented 2023-04-25 17:06:55 +02:00
Outdated
Review
Copy Link

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`.
Sergey Sharybin commented 2023-04-28 17:12:57 +02:00
Outdated
Review
Copy Link

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.
LayerGroup group2("Group2");
group2.add_layer(Layer("Layer3"));
group2.add_layer(Layer("Layer4"));
group.add_group(std::move(group2));
root.add_group(std::move(group));
root.add_layer(Layer("Layer3"));
root.add_layer(Layer("Layer5"));
root.foreach_children_pre_order([](TreeNode &child) { std::cout << child.name << std::endl; });
}