Initial Grease Pencil 3.0 stage #106848
|
@ -418,7 +418,7 @@ bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &frame)
|
|||
|
||||
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &&frame)
|
||||
{
|
||||
return this->frames_for_write().add_overwrite(frame_number, frame);
|
||||
return this->frames_for_write().add_overwrite(frame_number, std::move(frame));
|
||||
|
||||
}
|
||||
|
||||
Span<int> Layer::sorted_keys() const
|
||||
|
|
|
@ -16,6 +16,8 @@ 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
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
I used fixtures before, but decided to use this approach instead, because all the tests are then under the same 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
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 ( 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 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. */
|
||||
struct GreasePencilIDTestContext {
|
||||
Main *bmain = nullptr;
|
||||
|
||||
|
@ -159,6 +161,19 @@ TEST(greasepencil, remove_drawing)
|
|||
expected_frames_pairs_layer0[1][1]);
|
||||
}
|
||||
|
||||
TEST(greasepencil, overwrite_frame)
|
||||
{
|
||||
Layer layer1("Layer1");
|
||||
|
||||
layer1.insert_frame(0, GreasePencilFrame{0});
|
||||
layer1.tag_frames_map_keys_changed();
|
||||
|
||||
EXPECT_EQ(layer1.frames().lookup(0).drawing_index, 0);
|
||||
|
||||
layer1.overwrite_frame(0, GreasePencilFrame{42});
|
||||
EXPECT_EQ(layer1.frames().lookup(0).drawing_index, 42);
|
||||
}
|
||||
|
||||
/* --------------------------------------------------------------------------------------------- */
|
||||
/* Layer Tree Tests. */
|
||||
|
||||
|
|
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.Maybe @JacquesLucke can clarify here, but I believe sine
add_overwrite
has an implementation with an r-value reference that does thestd::move
I think this is fine?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
.