blender
/
blender
133
398
Fork 579
Code Issues 6.3k Pull Requests 745 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 +4 -42
2 changed files with 4 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0504c6a5a3 - Show all commits

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

@ -251,7 +251,7 @@ class LayerGroupRuntime {
class LayerGroup : public ::GreasePencilLayerTreeGroup {
public:
LayerGroup();
explicit LayerGroup(const StringRefNull name);
explicit LayerGroup(StringRefNull name);
LayerGroup(const LayerGroup &other);
~LayerGroup();
@ -279,7 +279,8 @@ class LayerGroup : public ::GreasePencilLayerTreeGroup {
int64_t num_nodes_total() const;
/**
* Removes a child from the group by index.
* Removes a child from the group by index. Does not free the memory.
* \note: Assumes the removed child is not the active layer.
*/
void remove_child(int64_t index);
@ -318,13 +319,6 @@ void legacy_gpencil_to_grease_pencil(Main &main, GreasePencil &grease_pencil, bG
class GreasePencilRuntime {
public:
Falk David commented 2023-04-12 10:41:45 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
void *batch_cache = nullptr;
public:
// 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;
};
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

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

@ -17,7 +17,7 @@ namespace blender::bke::greasepencil::tests {
/* Grease Pencil ID Tests. */
/* Note: Using a struct with constructor and destructor instead of a fixture here, to have all the
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.
* tests in the same group. */
* tests in the same group (`greasepencil`). */
struct GreasePencilIDTestContext {
Main *bmain = nullptr;
@ -41,38 +41,6 @@ TEST(greasepencil, create_grease_pencil_id)
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, 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);
// const Layer &layer1 = grease_pencil.root_group.wrap().add_layer("Layer1");
// const Layer &layer2 = grease_pencil.root_group.wrap().add_layer("Layer2");
// 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);
// 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);
// /* 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");
// }
/* --------------------------------------------------------------------------------------------- */
/* Drawing Array Tests. */