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
2 changed files with 35 additions and 12 deletions
Showing only changes of commit 1b087afa06 - Show all commits

View File

@ -10,6 +10,7 @@
#include "BLI_function_ref.hh"
#include "BLI_map.hh"
#include "BLI_math_vector.h"
#include "BLI_shared_cache.hh"
#include "BLI_stack.hh"
#include "BLI_string.h"
@ -247,7 +248,7 @@ class Layer : public TreeNode, ::GreasePencilLayer {
* drawings, then the last referenced drawing is held for the rest of the duration.
*/
filedescriptor marked this conversation as resolved Outdated

Since the map will have to change to store more than just an index as the value, make sure to update the comment too.

Since the map will have to change to store more than just an index as the value, make sure to update the comment too.
Map<int, int> frames_;
Vector<int> sorted_keys_cache_;
mutable SharedCache<Vector<int>> sorted_keys_cache_;
public:
Layer() : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF), frames_()
@ -284,26 +285,26 @@ class Layer : public TreeNode, ::GreasePencilLayer {
bool insert_frame(int frame_number, int index)
{
sorted_keys_cache_.clear_and_shrink();
sorted_keys_cache_.tag_dirty();
return frames_.add(frame_number, index);
}
bool overwrite_frame(int frame_number, int index)
{
sorted_keys_cache_.clear_and_shrink();
sorted_keys_cache_.tag_dirty();
return frames_.add_overwrite(frame_number, index);
}

= {} doesn't change anything here since Vector has a default constructor. It can be removed I think. Same with below

`= {}` doesn't change anything here since `Vector` has a default constructor. It can be removed I think. Same with below

Put out of line.

Put out of line.
Span<int> sorted_keys()
{
if (sorted_keys_cache_.is_empty() && frames_.size() > 0) {
sorted_keys_cache_.reserve(frames_.size());
for (int16_t key : frames_.keys()) {
sorted_keys_cache_.append(key);
sorted_keys_cache_.ensure([&](Vector<int> &r_data) {
r_data.reserve(frames_.size());
for (int64_t key : frames_.keys()) {
r_data.append(key);

Looks like this should be private maybe? It has a _ suffix.

Looks like this should be private maybe? It has a `_` suffix.
}
std::sort(sorted_keys_cache_.begin(), sorted_keys_cache_.end());
}
return sorted_keys_cache_.as_span();
std::sort(r_data.begin(), r_data.end());
});
return sorted_keys_cache_.data().as_span();
}
void save_to_storage(GreasePencilLayerTreeNode **dst)

Put out of line.

Put out of line.
@ -335,6 +336,28 @@ class Layer : public TreeNode, ::GreasePencilLayer {
this->frames_.add(node.frames_storage.keys[i], node.frames_storage.values[i]);
}
}
/**
* Return the index of the drawing at frame \a frame or -1 if there is no drawing.
*/
int drawing_at(int frame)
{
Span<int> sorted_keys = this->sorted_keys();
/* Before the first drawing, return no drawing. */
if (frame < sorted_keys.first()) {

Put out of line.

Put out of line.
return -1;
}
/* After or at the last drawing, return the last drawing. */
if (frame >= sorted_keys.last()) {
return frames().lookup(sorted_keys.last());
}
/* Search for the drawing. upper_bound will get the drawing just after. */
auto it = std::upper_bound(sorted_keys.begin(), sorted_keys.end(), frame);
if (it == sorted_keys.end() || it == sorted_keys.begin()) {
return -1;
}
return frames().lookup(*std::prev(it));

This needs to be implemented.

This needs to be implemented.
}
};
class LayerGroup : public TreeNode {

View File

@ -412,10 +412,10 @@ void GreasePencil::foreach_visible_drawing(
continue;
}
Layer &layer = node.as_layer();

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.
if (!layer.frames().contains(frame)) {
int index = layer.drawing_at(frame);
if (index == -1) {
continue;
}
int index = layer.frames().lookup(frame);
GreasePencilDrawingOrReference *drawing_or_reference = drawings[index];
if (drawing_or_reference->type == GREASE_PENCIL_DRAWING) {
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_or_reference);

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.

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?

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