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 +35 -3
3 changed files with 35 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit a7596f02a8 - Show all commits

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

@ -222,6 +222,8 @@ class Layer : public ::GreasePencilLayer {
*/
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.
int drawing_index_at(const int frame) const;
void tag_frames_map_changed();
/**
* Should be called whenever the keys in the frames map have changed. E.g. when new keys were
* added, removed or updated.

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

@ -334,6 +334,7 @@ Layer::Layer()
this->frames_storage.size = 0;
this->frames_storage.keys = nullptr;
this->frames_storage.values = nullptr;
this->frames_storage.flag = 0;
BLI_listbase_clear(&this->masks);
@ -398,21 +399,25 @@ bool Layer::is_locked() const
bool Layer::insert_frame(int frame_number, const GreasePencilFrame &frame)
{
this->tag_frames_map_changed();
return this->frames_for_write().add(frame_number, frame);
}
bool Layer::insert_frame(int frame_number, GreasePencilFrame &&frame)
{
this->tag_frames_map_changed();
return this->frames_for_write().add(frame_number, frame);
}
bool Layer::overwrite_frame(int frame_number, const GreasePencilFrame &frame)
{
this->tag_frames_map_changed();
Hans Goudey commented 2023-05-03 16:13:17 +02:00
Outdated
Review
Copy Link

Since this is making a copy of the frame, the argument might as well be a const reference rather than a non-const one.

Since this is making a copy of the frame, the argument might as well be a const reference rather than a non-const one.
return this->frames_for_write().add_overwrite(frame_number, frame);
}
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &&frame)
{
this->tag_frames_map_changed();
return this->frames_for_write().add_overwrite(frame_number, std::move(frame));
Hans Goudey commented 2023-05-02 23:39:38 +02:00
Outdated
Review
Copy Link

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.

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.
Falk David commented 2023-05-03 11:02:06 +02:00
Outdated
Review
Copy Link

Maybe @JacquesLucke can clarify here, but I believe sine add_overwrite has an implementation with an r-value reference that does the std::move I think this is fine?

Maybe @JacquesLucke can clarify here, but I believe sine `add_overwrite` has an implementation with an r-value reference that does the `std::move` I think this is fine?
Jacques Lucke commented 2023-05-03 11:05:50 +02:00
Outdated
Review
Copy Link

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.

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`.
👍 1
}
@ -452,8 +457,14 @@ int Layer::drawing_index_at(const int frame) const
return this->frames().lookup(*std::prev(it)).drawing_index;
}
void Layer::tag_frames_map_changed()
{
this->frames_storage.flag |= GP_LAYER_FRAMES_STORAGE_DIRTY;
}
void Layer::tag_frames_map_keys_changed()
{
this->tag_frames_map_changed();
this->runtime->sorted_keys_cache_.tag_dirty();
}
@ -1193,10 +1204,24 @@ static void write_layer(BlendWriter *writer, GreasePencilLayer *node)
BLO_write_struct(writer, GreasePencilLayer, node);
BLO_write_string(writer, node->base.name);
MEM_SAFE_FREE(node->frames_storage.keys);
MEM_SAFE_FREE(node->frames_storage.values);
/* Re-create the frames storage only if it was tagged dirty. */
if ((node->frames_storage.flag & GP_LAYER_FRAMES_STORAGE_DIRTY) != 0) {
MEM_SAFE_FREE(node->frames_storage.keys);
MEM_SAFE_FREE(node->frames_storage.values);
const Layer &layer = node->wrap();
node->frames_storage.size = layer.frames().size();
node->frames_storage.keys = MEM_cnew_array<int>(node->frames_storage.size, __func__);
node->frames_storage.values = MEM_cnew_array<GreasePencilFrame>(node->frames_storage.size,
__func__);
const Span<int> sorted_keys = layer.sorted_keys();
for (const int i : sorted_keys.index_range()) {
node->frames_storage.keys[i] = sorted_keys[i];
node->frames_storage.values[i] = layer.frames().lookup(sorted_keys[i]);
}
/* Reset the flag. */
node->frames_storage.flag &= ~GP_LAYER_FRAMES_STORAGE_DIRTY;
}
BLO_write_int32_array(writer, node->frames_storage.size, node->frames_storage.keys);

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

@ -155,6 +155,10 @@ typedef struct GreasePencilFrame {
char _pad[3];
} GreasePencilFrame;
typedef enum GreasePencilLayerFramesMapStorageFlag {
GP_LAYER_FRAMES_STORAGE_DIRTY = (1 << 0),
} GreasePencilLayerFramesMapStorageFlag;
/**
* Storage for the Map in `blender::bke::greasepencil::Layer`.
* See the description there for more detail.
@ -166,7 +170,8 @@ typedef struct GreasePencilLayerFramesMapStorage {
GreasePencilFrame *values;
/* Size of the map (number of key-value pairs). */
int size;
char _pad[4];
/* Flag for the status of the storage. */
int flag;
} GreasePencilLayerFramesMapStorage;
/**