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

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

@ -31,267 +31,58 @@ class Layer;
* A TreeNode represents one node in the layer tree.
* It can either be a layer or a group. The node has zero children if it is a layer or zero or more
* children if it is a group.
* This class is mainly used for iteration over the layer tree.
*/
class TreeNode : public ::GreasePencilLayerTreeNode, NonMovable {
using TreeNodeIterFn = FunctionRef<void(TreeNode &)>;
using LayerIterFn = FunctionRef<void(Layer &)>;
using LayerIndexIterFn = FunctionRef<void(uint64_t, Layer &)>;
protected:
Vector<std::unique_ptr<TreeNode>> children_;
public:
TreeNode(GreasePencilLayerTreeNodeType type)
{
this->type = type;
this->name = nullptr;
}
TreeNode(GreasePencilLayerTreeNodeType type, StringRefNull name)
{
this->type = type;
this->name = BLI_strdup(name.c_str());
}
TreeNode(const TreeNode &other) : TreeNode(GreasePencilLayerTreeNodeType(other.type))
{
if (other.name) {
this->name = BLI_strdup(other.name);
}
}
explicit TreeNode(GreasePencilLayerTreeNodeType type);
explicit TreeNode(GreasePencilLayerTreeNodeType type, StringRefNull name);
TreeNode(const TreeNode &other);
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.
TreeNode &operator=(const TreeNode &other) = delete;
virtual ~TreeNode()
{
if (this->name) {
MEM_freeN(this->name);
}
}
virtual ~TreeNode();
public:
class PreOrderRange {
class Iterator {
using iterator_category = std::forward_iterator_tag;
private:
Stack<TreeNode *> next_node_;
public:
explicit Iterator(TreeNode *root)
{
if (root != nullptr) {
for (auto it = root->children_.rbegin(); it != root->children_.rend(); it++) {
next_node_.push((*it).get());
}
}
}
TreeNode &operator*()
{
return *next_node_.peek();
}
const TreeNode &operator*() const
{
return *next_node_.peek();
}
Iterator &operator++()
{
BLI_assert(!next_node_.is_empty());
TreeNode &next_node = *next_node_.pop();
for (auto it = next_node.children_.rbegin(); it != next_node.children_.rend(); it++) {
next_node_.push((*it).get());
}
return *this;
}
Iterator operator++(int)
{
Iterator old = *this;
operator++();
return old;
}
bool operator==(Iterator &other) const
{
if (next_node_.size() == 0) {
return other.next_node_.size() == 0;
}
return (next_node_.size() == other.next_node_.size()) &&
next_node_.peek() == other.next_node_.peek();
}
bool operator!=(Iterator &other) const
{
return !(*this == other);
}
};
private:
TreeNode *_root;
public:
explicit PreOrderRange(TreeNode *root) : _root(root) {}
Iterator begin()
{
return Iterator(_root);
}
Iterator end()
{
return Iterator(nullptr);
}
};
class PreOrderIndexRange {
public:
struct Item {
const int64_t index;
TreeNode &node;
};
private:
class Iterator {
using iterator_category = std::forward_iterator_tag;
private:
int64_t current_index_;
Stack<TreeNode *> next_node_;
public:
explicit Iterator(TreeNode *root)
{
current_index_ = 0;
if (root != nullptr) {
for (auto it = root->children_.rbegin(); it != root->children_.rend(); it++) {
next_node_.push((*it).get());
}
}
}
Item operator*()
{
return {current_index_, *next_node_.peek()};
}
Iterator &operator++()
{
BLI_assert(!next_node_.is_empty());
TreeNode &next_node = *next_node_.pop();
current_index_++;
for (auto it = next_node.children_.rbegin(); it != next_node.children_.rend(); it++) {
next_node_.push((*it).get());
}
return *this;
}
Iterator operator++(int)
{
Iterator old = *this;
operator++();
return old;
}
bool operator==(Iterator &other) const
{
if (next_node_.size() == 0) {
return other.next_node_.size() == 0;
}
return (next_node_.size() == other.next_node_.size()) &&
next_node_.peek() == other.next_node_.peek() &&
current_index_ == other.current_index_;
}
bool operator!=(Iterator &other) const
{
return !(*this == other);
}
};
private:
TreeNode *_root;
public:
explicit PreOrderIndexRange(TreeNode *root) : _root(root) {}
Iterator begin()
{
return Iterator(_root);
}
Iterator end()
{
return Iterator(nullptr);
}
};
Vector<std::unique_ptr<TreeNode>> children;
public:
/**
* \returns true if this node is a LayerGroup.
*/
constexpr bool is_group() const
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
{
return this->type == GREASE_PENCIL_LAYER_TREE_GROUP;
}
/**
* \returns true if this node is a Layer.
*/
constexpr bool is_layer() const
{
return this->type == GREASE_PENCIL_LAYER_TREE_LEAF;
}
LayerGroup &as_group();
Layer &as_layer();
/**
* \returns this tree node as a LayerGroup.
* \note This results in undefined behavior if the node is not a LayerGroup.
*/
const LayerGroup &as_group() const;
/**
* \returns this tree node as a Layer.
* \note This results in undefined behavior if the node is not a Layer.
*/
const Layer &as_layer() const;
Falk David commented 2023-04-12 10:39:29 +02:00
Outdated
Review
Copy Link

Try putting this class out of line.

Try putting this class out of line.
int total_num_children() const
{
int total = 0;
Stack<TreeNode *> stack;
for (auto it = this->children_.rbegin(); it != this->children_.rend(); it++) {
stack.push((*it).get());
}
while (!stack.is_empty()) {
TreeNode &next_node = *stack.pop();
total++;
for (auto it = next_node.children_.rbegin(); it != next_node.children_.rend(); it++) {
stack.push((*it).get());
}
}
return total;
}
/**
* \returns this tree node as a mutable LayerGroup.
* \note This results in undefined behavior if the node is not a LayerGroup.
*/
LayerGroup &as_group_for_write();
PreOrderRange children_in_pre_order();
PreOrderIndexRange children_with_index_in_pre_order();
void foreach_children_pre_order(TreeNodeIterFn function)
{
for (auto &child : children_) {
child->foreach_children_pre_order_recursive_(function);
}
}
void foreach_layer_pre_order(LayerIterFn function)
{
for (auto &child : children_) {
child->foreach_layer_pre_order_recursive_(function);
}
}
private:
void foreach_children_pre_order_recursive_(TreeNodeIterFn function)
{
function(*this);
for (auto &child : children_) {
child->foreach_children_pre_order_recursive_(function);
}
}
void foreach_layer_pre_order_recursive_(LayerIterFn function)
{
if (this->is_layer()) {
function(this->as_layer());
}
for (auto &child : children_) {
child->foreach_layer_pre_order_recursive_(function);
}
}
/**
* \returns this tree node as a mutable Layer.
* \note This results in undefined behavior if the node is not a Layer.
*/
Layer &as_layer_for_write();
Hans Goudey commented 2023-05-02 23:32:14 +02:00
Review
Copy Link

The _for_write() suffix doesn't seem super helpful here TBH, but I guess it doesn't hurt.

The `_for_write()` suffix doesn't seem super helpful here TBH, but I guess it doesn't hurt.
};
/**
@ -330,150 +121,110 @@ class Layer : public TreeNode, ::GreasePencilLayer {
public:
Layer() : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF), frames_() {}
Layer(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF, name), frames_() {}
Layer(const Layer &other) : TreeNode(other)
{
frames_ = other.frames_;
}
const Map<int, GreasePencilFrame> &frames() const
{
return frames_;
}
Map<int, GreasePencilFrame> &frames_for_write()
{
sorted_keys_cache_.tag_dirty();
return frames_;
}
bool insert_frame(int frame_number, GreasePencilFrame &frame)
{
sorted_keys_cache_.tag_dirty();
return frames_for_write().add(frame_number, frame);
}
bool insert_frame(int frame_number, GreasePencilFrame &&frame)
{
sorted_keys_cache_.tag_dirty();
return frames_for_write().add(frame_number, frame);
}
bool overwrite_frame(int frame_number, GreasePencilFrame &frame)
{
sorted_keys_cache_.tag_dirty();
return frames_for_write().add_overwrite(frame_number, frame);
}
bool overwrite_frame(int frame_number, GreasePencilFrame &&frame)
{
sorted_keys_cache_.tag_dirty();
return frames_for_write().add_overwrite(frame_number, frame);
}
Span<int> sorted_keys() const
{
sorted_keys_cache_.ensure([&](Vector<int> &r_data) {
r_data.clear_and_shrink();
r_data.reserve(frames().size());
for (int64_t key : frames().keys()) {
r_data.append(key);
}
std::sort(r_data.begin(), r_data.end());
});
return sorted_keys_cache_.data().as_span();
}
explicit Layer(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF, name), frames_() {}
Layer(const Layer &other);
/**
* Return the index of the drawing at frame \a frame or -1 if there is no drawing.
* \returns the frames mapping.
*/
int drawing_at(int frame) const
{
Span<int> sorted_keys = this->sorted_keys();
/* Before the first drawing, return no drawing. */
if (frame < sorted_keys.first()) {
return -1;
}
/* After or at the last drawing, return the last drawing. */
if (frame >= sorted_keys.last()) {
return this->frames().lookup(sorted_keys.last()).drawing_index;
}
/* 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 this->frames().lookup(*std::prev(it)).drawing_index;
}
const Map<int, GreasePencilFrame> &frames() const;
Map<int, GreasePencilFrame> &frames_for_write();
/**
* Inserts the frame into the layer. Fails if there exists a frame at \a frame_number already.
* \returns true on success.
*/
bool insert_frame(int frame_number, GreasePencilFrame &frame);
bool insert_frame(int frame_number, GreasePencilFrame &&frame);
/**
* Inserts the frame into the layer. If there exists a frame at \a frame_number already, it is
* overwritten.
* \returns true on success.
*/
bool overwrite_frame(int frame_number, GreasePencilFrame &frame);
bool overwrite_frame(int frame_number, GreasePencilFrame &&frame);
/**
* Returns the sorted (start) frame numbers of the frames of this layer.
* \note This will cache the keys lazily.
*/
Span<int> sorted_keys() const;
/**
* \returns the index of the drawing at frame \a frame or -1 if there is no drawing.
*/
Hans Goudey commented 2023-05-03 16:10:32 +02:00
Outdated
Review
Copy Link

Better to return a span here maybe?

Better to return a span here maybe?
int drawing_index_at(int frame) const;
};
/**
* A LayerGroup is a grouping of zero or more Layers.
*/
class LayerGroup : public TreeNode {
Falk David commented 2023-04-12 10:39:55 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
using TreeNodeIterFn = FunctionRef<void(TreeNode &)>;
using TreeNodeIndexIterFn = FunctionRef<void(int64_t, TreeNode &)>;
using LayerIterFn = FunctionRef<void(Layer &)>;
Hans Goudey commented 2023-05-03 16:11:22 +02:00
Outdated
Review
Copy Link

The function is "is_locked" and the comment says "return if it is locked". This sort of comment doesn't add anything IMO, just wastes space. If there is a comment, maybe it should use a different word besides "locked" to describe the state. Or there doesn't need to be a comment at all.

The function is "is_locked" and the comment says "return if it is locked". This sort of comment doesn't add anything IMO, just wastes space. If there is a comment, maybe it should use a different word besides "locked" to describe the state. Or there doesn't need to be a comment at all.
using LayerIndexIterFn = FunctionRef<void(int64_t, Layer &)>;
public:
LayerGroup() : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP) {}
explicit LayerGroup(const StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, name) {}
LayerGroup(const LayerGroup &other) : TreeNode(other)
{
children_.reserve(other.children_.size());
for (const std::unique_ptr<TreeNode> &elem : other.children_) {
if (elem.get()->is_group()) {
children_.append(std::make_unique<LayerGroup>(elem.get()->as_group()));
}
else if (elem.get()->is_layer()) {
children_.append(std::make_unique<Layer>(elem.get()->as_layer()));
}
}
}
LayerGroup(const LayerGroup &other);
public:
void add_group(LayerGroup &group)
{
children_.append(std::make_unique<LayerGroup>(group));
}
/**
* Adds a group at the end of this group.
*/
void add_group(LayerGroup &group);
void add_group(LayerGroup &&group);
void add_group(LayerGroup &&group)
{
children_.append(std::make_unique<LayerGroup>(group));
}
/**
* Adds a layer at the end of this group and returns it.
*/
Layer &add_layer(Layer &layer);
Layer &add_layer(Layer &&layer);
Layer &add_layer(Layer &layer)
{
int64_t index = children_.append_and_get_index(std::make_unique<Layer>(layer));
return children_[index].get()->as_layer();
}
/**
* Returns the number of direct children in this group.
*/
int64_t num_direct_children() const;
Layer &add_layer(Layer &&layer)
{
int64_t index = children_.append_and_get_index(std::make_unique<Layer>(layer));
return children_[index].get()->as_layer();
}
/**
* Returns the total number of children in this group.
filedescriptor marked this conversation as resolved Outdated
Bastien Montagne commented 2023-05-24 18:36:55 +02:00
Outdated
Review
Copy Link

Would be good to know why this is commented out, or it should be cleaned up before merge in main.

Would be good to know why this is commented out, or it should be cleaned up before merge in main.
*/
int64_t num_children_total() const;
int num_children() const
{
return children_.size();
}
/**
* Removes a child from the group by index.
*/
void remove_child(int64_t index);
Falk David commented 2023-04-12 10:40:07 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
void remove_child(int64_t index)
{
BLI_assert(index >= 0 && index < children_.size());
children_.remove(index);
}
/**
* Calls \a function on every `TreeNode` in this group.
*/
void foreach_children_pre_order(TreeNodeIterFn function);
void foreach_children_with_index_pre_order(TreeNodeIndexIterFn function);
/**
Falk David commented 2023-04-12 10:40:11 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
* Returns a `Vector` of pointers to all the `TreeNode`s in this group.
*/
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.
Vector<TreeNode *> children_in_pre_order() const;
/**
* Returns a `Vector` of pointers to all the `Layers`s in this group.
*/
Vector<Layer *> layers_in_pre_order() const;
};
namespace convert {
void legacy_gpencil_frame_to_grease_pencil_drawing(GreasePencilDrawing &drawing, bGPDframe &gpf);
void legacy_gpencil_to_grease_pencil(GreasePencil &grease_pencil, bGPdata &gpd);
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.
} // namespace convert
} // namespace greasepencil
using namespace blender::bke::greasepencil;
struct StrokePoint {
float3 position;
float radius;
@ -494,71 +245,34 @@ class GreasePencilDrawingRuntime {
};
class GreasePencilRuntime {
private:
LayerGroup root_group_;
public:
mutable SharedCache<Vector<greasepencil::Layer *>> layer_cache_;
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:41:05 +02:00
Outdated
Review
Copy Link

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.
private:
greasepencil::LayerGroup root_group_;
int active_layer_index_ = -1;
Layer *active_layer_ = nullptr;
public:
GreasePencilRuntime() {}
GreasePencilRuntime(const GreasePencilRuntime &other)
: root_group_(other.root_group_), active_layer_index_(other.active_layer_index_)
{
active_layer_ = get_active_layer_from_index(other.active_layer_index_);
}
GreasePencilRuntime(const GreasePencilRuntime &other);
/* TODO: There should be a const version of this for reads and a mutable version of this for
* writes. */
LayerGroup &root_group()
{
return root_group_;
}
public:
const greasepencil::LayerGroup &root_group() const;
greasepencil::LayerGroup &root_group_for_write();
bool has_active_layer() const
{
return active_layer_ != nullptr;
}
bool has_active_layer() const;
const greasepencil::Layer &active_layer() const;
greasepencil::Layer &active_layer_for_write() const;
void set_active_layer_index(int index);
int active_layer_index() const;
const Layer &active_layer() const
{
BLI_assert(active_layer_ != nullptr);
return *active_layer_;
}
Layer &active_layer_for_write() const
{
BLI_assert(active_layer_ != nullptr);
return *active_layer_;
}
void set_active_layer(int index)
{
active_layer_index_ = index;
active_layer_ = get_active_layer_from_index(index);
}
int active_layer_index() const
{
return active_layer_index_;
}
void ensure_layer_cache() const;
public:
void *batch_cache = nullptr;
private:
Layer *get_active_layer_from_index(int index)
{
if (index < 0) {
return nullptr;
}
for (auto item : this->root_group().children_with_index_in_pre_order()) {
if (item.node.is_layer() && item.index == index) {
return &item.node.as_layer();
}
}
return nullptr;
}
greasepencil::Layer *get_active_layer_from_index(int index) const;
};
Bastien Montagne commented 2023-05-06 18:45:34 +02:00
Outdated
Review
Copy Link

Not sure why the greasepencil namespace ends up here? Also means that e.g. the fairly generically-named StrokePoint struct is in the fairly generic blender::bke namespace, which does not sounds great to me? not to mention 'C-style' namespace in names like GreasePencilDrawingRuntime.

Not sure why the `greasepencil` namespace ends up here? Also means that e.g. the fairly generically-named `StrokePoint` struct is in the fairly generic `blender::bke` namespace, which does not sounds great to me? not to mention 'C-style' namespace in names like `GreasePencilDrawingRuntime`.
} // namespace blender::bke

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

@ -250,24 +250,36 @@ IDTypeInfo IDType_ID_GP = {
namespace blender::bke::greasepencil {
TreeNode::PreOrderRange TreeNode::children_in_pre_order()
TreeNode::TreeNode(GreasePencilLayerTreeNodeType type)
{
return TreeNode::PreOrderRange(this);
this->type = type;
this->name = nullptr;
}
TreeNode::PreOrderIndexRange TreeNode::children_with_index_in_pre_order()
TreeNode::TreeNode(GreasePencilLayerTreeNodeType type, StringRefNull name)
{
return TreeNode::PreOrderIndexRange(this);
this->type = type;
this->name = BLI_strdup(name.c_str());
}
LayerGroup &TreeNode::as_group()
TreeNode::TreeNode(const TreeNode &other)
: TreeNode::TreeNode(GreasePencilLayerTreeNodeType(other.type))
{
return *static_cast<LayerGroup *>(this);
if (other.name) {
this->name = BLI_strdup(other.name);
}
}
Layer &TreeNode::as_layer()
TreeNode::~TreeNode()
{
return *static_cast<Layer *>(this);
if (this->name) {
MEM_freeN(this->name);
}
}
const LayerGroup &TreeNode::as_group() const
{
return *static_cast<const LayerGroup *>(this);
}
const Layer &TreeNode::as_layer() const
@ -275,8 +287,282 @@ const Layer &TreeNode::as_layer() const
return *static_cast<const Layer *>(this);
}
LayerGroup &TreeNode::as_group_for_write()
{
return *static_cast<LayerGroup *>(this);
}
Layer &TreeNode::as_layer_for_write()
{
Falk David commented 2023-04-12 11:03:35 +02:00
Outdated
Review
Copy Link

This should only consider the current frame.

This should only consider the current frame.
return *static_cast<Layer *>(this);
}
Layer::Layer(const Layer &other) : TreeNode::TreeNode(other)
{
this->frames_ = other.frames_;
}
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:48:52 +02:00
Outdated
Review
Copy Link

Make sure to not overwrite min and max but account any value already present. E.g. use math::min_max(...).

Make sure to not overwrite `min` and `max` but account any value already present. E.g. use `math::min_max(...)`.
const Map<int, GreasePencilFrame> &Layer::frames() const
{
return this->frames_;
}
Map<int, GreasePencilFrame> &Layer::frames_for_write()
{
this->sorted_keys_cache_.tag_dirty();
return this->frames_;
}
bool Layer::insert_frame(int frame_number, GreasePencilFrame &frame)
{
this->sorted_keys_cache_.tag_dirty();
return this->frames_for_write().add(frame_number, frame);
}
bool Layer::insert_frame(int frame_number, GreasePencilFrame &&frame)
{
this->sorted_keys_cache_.tag_dirty();
return this->frames_for_write().add(frame_number, frame);
}
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &frame)
{
this->sorted_keys_cache_.tag_dirty();
return this->frames_for_write().add_overwrite(frame_number, frame);
}
bool Layer::overwrite_frame(int frame_number, GreasePencilFrame &&frame)
{
this->sorted_keys_cache_.tag_dirty();
return this->frames_for_write().add_overwrite(frame_number, frame);
}
Span<int> Layer::sorted_keys() const
{
this->sorted_keys_cache_.ensure([&](Vector<int> &r_data) {
r_data.clear_and_shrink();
r_data.reserve(this->frames().size());
for (int64_t key : this->frames().keys()) {
r_data.append(key);
}
std::sort(r_data.begin(), r_data.end());
});
return this->sorted_keys_cache_.data().as_span();
}
int Layer::drawing_index_at(int frame) const
{
Span<int> sorted_keys = this->sorted_keys();
/* Before the first drawing, return no drawing. */
if (frame < sorted_keys.first()) {
return -1;
}
/* After or at the last drawing, return the last drawing. */
if (frame >= sorted_keys.last()) {
return this->frames().lookup(sorted_keys.last()).drawing_index;
}
/* 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 this->frames().lookup(*std::prev(it)).drawing_index;
}
LayerGroup::LayerGroup(const LayerGroup &other) : TreeNode(other)
{
this->children.reserve(other.children.size());
for (const std::unique_ptr<TreeNode> &elem : other.children) {
if (elem.get()->is_group()) {
this->children.append(std::make_unique<LayerGroup>(elem.get()->as_group()));
}
else if (elem.get()->is_layer()) {
this->children.append(std::make_unique<Layer>(elem.get()->as_layer()));
}
}
}
void LayerGroup::add_group(LayerGroup &group)
{
this->children.append(std::make_unique<LayerGroup>(group));
}
void LayerGroup::add_group(LayerGroup &&group)
{
this->children.append(std::make_unique<LayerGroup>(group));
}
Layer &LayerGroup::add_layer(Layer &layer)
{
int64_t index = children.append_and_get_index(std::make_unique<Layer>(layer));
return children[index].get()->as_layer_for_write();
}
Layer &LayerGroup::add_layer(Layer &&layer)
{
int64_t index = children.append_and_get_index(std::make_unique<Layer>(layer));
return children[index].get()->as_layer_for_write();
}
int64_t LayerGroup::num_direct_children() const
{
return children.size();
}
int64_t LayerGroup::num_children_total() const
{
int64_t total = 0;
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.
Stack<TreeNode *> stack;
for (auto it = this->children.rbegin(); it != this->children.rend(); it++) {
stack.push((*it).get());
}
while (!stack.is_empty()) {
TreeNode &next_node = *stack.pop();
total++;
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
for (auto it = next_node.children.rbegin(); it != next_node.children.rend(); it++) {
stack.push((*it).get());
}
}
return total;
}
Hans Goudey commented 2023-04-25 04:56:25 +02:00
Outdated
Review
Copy Link

Any reason to call clear_and_shrink here? Might as well call reinitialize if not

Any reason to call `clear_and_shrink` here? Might as well call `reinitialize` if not
void LayerGroup::remove_child(int64_t index)
{
BLI_assert(index >= 0 && index < this->children.size());
this->children.remove(index);
}
Hans Goudey commented 2023-04-25 04:57:11 +02:00
Outdated
Review
Copy Link

.as_span() shouldn't be necessary here.

`.as_span()` shouldn't be necessary here.
void LayerGroup::foreach_children_pre_order(TreeNodeIterFn function)
{
for (auto &child : this->children) {
Hans Goudey commented 2023-05-03 16:21:17 +02:00
Outdated
Review
Copy Link

int frame -> const int frame

`int frame` -> `const int frame`
function(*child);
if (child->is_group()) {
child->as_group_for_write().foreach_children_pre_order(function);
}
}
}
void LayerGroup::foreach_children_with_index_pre_order(TreeNodeIndexIterFn function)
{
Vector<TreeNode *> children = this->children_in_pre_order();
for (const int64_t i : IndexRange(children.size())) {
function(i, *children[i]);
}
}
Vector<TreeNode *> LayerGroup::children_in_pre_order() const
{
Vector<TreeNode *> children;
Stack<TreeNode *> stack;
for (auto it = this->children.rbegin(); it != this->children.rend(); it++) {
stack.push((*it).get());
}
while (!stack.is_empty()) {
TreeNode &next_node = *stack.pop();
children.append(&next_node);
for (auto it = next_node.children.rbegin(); it != next_node.children.rend(); it++) {
stack.push((*it).get());
}
}
return children;
}
Vector<Layer *> LayerGroup::layers_in_pre_order() const
{
Vector<Layer *> layers;
Stack<TreeNode *> stack;
for (auto it = this->children.rbegin(); it != this->children.rend(); it++) {
stack.push((*it).get());
}
while (!stack.is_empty()) {
TreeNode &next_node = *stack.pop();
if (next_node.is_layer()) {
layers.append(&next_node.as_layer_for_write());
}
else {
for (auto it = next_node.children.rbegin(); it != next_node.children.rend(); it++) {
Hans Goudey commented 2023-04-25 05:00:31 +02:00
Outdated
Review
Copy Link

I believe it's standard to std::move from a T &&

Is there a need for overloads for T & and T &&? What's the benefit of that?

I believe it's standard to `std::move` from a `T &&` Is there a need for overloads for `T &` and `T &&`? What's the benefit of that?
stack.push((*it).get());
}
}
}
return layers;
}
} // namespace blender::bke::greasepencil
/* ------------------------------------------------------------------- */
/** \name GreasePencilRuntime functions
* \{ */
namespace blender::bke {
GreasePencilRuntime::GreasePencilRuntime(const GreasePencilRuntime &other)
: root_group_(other.root_group_), active_layer_index_(other.active_layer_index_)
{
layer_cache_ = other.layer_cache_;
}
const greasepencil::LayerGroup &GreasePencilRuntime::root_group() const
{
return this->root_group_;
}
greasepencil::LayerGroup &GreasePencilRuntime::root_group_for_write()
{
this->layer_cache_.tag_dirty();
return this->root_group_;
}
bool GreasePencilRuntime::has_active_layer() const
{
return this->active_layer_index_ >= 0;
}
const greasepencil::Layer &GreasePencilRuntime::active_layer() const
{
BLI_assert(this->active_layer_index_ >= 0);
return *get_active_layer_from_index(this->active_layer_index_);
}
greasepencil::Layer &GreasePencilRuntime::active_layer_for_write() const
{
BLI_assert(this->active_layer_index_ >= 0);
return *get_active_layer_from_index(this->active_layer_index_);
}
void GreasePencilRuntime::set_active_layer_index(int index)
{
this->active_layer_index_ = index;
}
int GreasePencilRuntime::active_layer_index() const
{
return this->active_layer_index_;
}
void GreasePencilRuntime::ensure_layer_cache() const
{
this->layer_cache_.ensure([this](Vector<greasepencil::Layer *> &data) {
data = this->root_group_.layers_in_pre_order();
});
}
greasepencil::Layer *GreasePencilRuntime::get_active_layer_from_index(int index) const
{
this->ensure_layer_cache();
return this->layer_cache_.data()[index];
}
} // namespace blender::bke
/** \} */
/* ------------------------------------------------------------------- */
/** \name Grease Pencil kernel functions
* \{ */
void *BKE_grease_pencil_add(Main *bmain, const char *name)
{
GreasePencil *grease_pencil = static_cast<GreasePencil *>(BKE_id_new(bmain, ID_GP, name));
@ -356,7 +642,11 @@ void BKE_grease_pencil_data_update(struct Depsgraph * /*depsgraph*/,
BKE_object_eval_assign_data(object, &grease_pencil_eval->id, true);
}
/* Draw Cache */
/** \} */
/* ------------------------------------------------------------------- */
/** \name Draw Cache
* \{ */
void (*BKE_grease_pencil_batch_cache_dirty_tag_cb)(GreasePencil *grease_pencil,
int mode) = nullptr;
@ -376,7 +666,11 @@ void BKE_grease_pencil_batch_cache_free(GreasePencil *grease_pencil)
}
}
/* GreasePencilDrawing API */
/** \} */
/* ------------------------------------------------------------------- */
/** \name Grease Pencil Drawing API
* \{ */
blender::Span<blender::uint3> GreasePencilDrawing::triangles() const
{
@ -454,7 +748,11 @@ blender::Span<blender::bke::StrokePoint> GreasePencilDrawing::stroke_buffer()
return this->runtime->stroke_cache.as_span();
}
/* GreasePencil API */
/** \} */
/* ------------------------------------------------------------------- */
/** \name Grease Pencil data-block API
* \{ */
static void grease_pencil_grow_drawing_array_by(GreasePencil &self, const int add_capacity)
{
@ -533,18 +831,17 @@ void GreasePencil::remove_drawing(int index_to_remove)
/* Move the drawing that should be removed to the last index. */
const int last_drawing_index = this->drawing_array_size - 1;
if (index_to_remove != last_drawing_index) {
this->root_group().foreach_layer_pre_order(
[last_drawing_index, index_to_remove](Layer &layer) {
blender::Map<int, GreasePencilFrame> &frames = layer.frames_for_write();
for (auto [key, value] : frames.items()) {
if (value.drawing_index == last_drawing_index) {
value.drawing_index = index_to_remove;
}
else if (value.drawing_index == index_to_remove) {
value.drawing_index = last_drawing_index;
}
}
});
for (Layer *layer : this->layers_for_write()) {
blender::Map<int, GreasePencilFrame> &frames = layer->frames_for_write();
for (auto [key, value] : frames.items()) {
if (value.drawing_index == last_drawing_index) {
value.drawing_index = index_to_remove;
}
else if (value.drawing_index == index_to_remove) {
value.drawing_index = last_drawing_index;
}
}
}
std::swap(this->drawings_for_write()[index_to_remove],
this->drawings_for_write()[last_drawing_index]);
}
@ -571,12 +868,12 @@ void GreasePencil::remove_drawing(int index_to_remove)
}
/* Remove any frame that points to the last drawing. */
this->root_group().foreach_layer_pre_order([last_drawing_index](Layer &layer) {
blender::Map<int, GreasePencilFrame> &frames = layer.frames_for_write();
for (Layer *layer : this->layers_for_write()) {
blender::Map<int, GreasePencilFrame> &frames = layer->frames_for_write();
frames.remove_if([last_drawing_index](auto item) {
return item.value.drawing_index == last_drawing_index;
});
});
}
/* Shrink drawing array. */
grease_pencil_shrink_drawing_array_by(*this, 1);
@ -588,12 +885,8 @@ void GreasePencil::foreach_visible_drawing(
using namespace blender::bke::greasepencil;
blender::Span<GreasePencilDrawingOrReference *> drawings = this->drawings();
for (TreeNode &node : this->root_group().children_in_pre_order()) {
if (!node.is_layer()) {
continue;
}
Layer &layer = node.as_layer();
int index = layer.drawing_at(frame);
for (const Layer *layer : this->layers()) {
int index = layer->drawing_index_at(frame);
if (index == -1) {
continue;
}
@ -608,12 +901,41 @@ void GreasePencil::foreach_visible_drawing(
}
}
blender::bke::greasepencil::LayerGroup &GreasePencil::root_group()
const blender::bke::greasepencil::LayerGroup &GreasePencil::root_group() const
{
BLI_assert(this->runtime != nullptr);
return this->runtime->root_group();
}
blender::bke::greasepencil::LayerGroup &GreasePencil::root_group_for_write()
{
BLI_assert(this->runtime != nullptr);
return this->runtime->root_group_for_write();
}
blender::Span<const blender::bke::greasepencil::Layer *> GreasePencil::layers() const
{
this->runtime->ensure_layer_cache();
return this->runtime->layer_cache_.data();
}
blender::Span<blender::bke::greasepencil::Layer *> GreasePencil::layers_for_write()
{
this->runtime->ensure_layer_cache();
return this->runtime->layer_cache_.data();
}
void GreasePencil::tag_layer_tree_changed()
{
this->runtime->layer_cache_.tag_dirty();
}
/** \} */
/* ------------------------------------------------------------------- */
/** \name Drawing array storage functions
* \{ */
void GreasePencil::read_drawing_array(BlendDataReader *reader)
{
BLO_read_pointer_array(reader, (void **)&this->drawing_array);
@ -692,7 +1014,13 @@ void GreasePencil::free_drawing_array()
this->drawing_array_size = 0;
}
static void save_tree_node_to_storage(blender::bke::greasepencil::TreeNode &node,
/** \} */
/* ------------------------------------------------------------------- */
/** \name Layer Tree storage functions
* \{ */
static void save_tree_node_to_storage(const blender::bke::greasepencil::TreeNode &node,
GreasePencilLayerTreeNode *dst)
{
dst->type = node.type;
@ -703,7 +1031,7 @@ static void save_tree_node_to_storage(blender::bke::greasepencil::TreeNode &node
}
}
static void save_layer_to_storage(blender::bke::greasepencil::Layer &node,
static void save_layer_to_storage(const blender::bke::greasepencil::Layer &node,
GreasePencilLayerTreeNode **dst)
{
using namespace blender;
Hans Goudey commented 2023-04-25 04:24:39 +02:00
Outdated
Review
Copy Link

What about replacing these with slightly more generic functions like this?

template<typename T> static void grow_array(T **array,  int *size, const int add_size) {}
template<typename T> static void shring_array(T **array,  int *size, const int shrink_size) {}

That would allow for simpler variable naming inside, and would tell the reader "there's nothing special going on here, just resizing some arrays" which makes it easier to skim

What about replacing these with slightly more generic functions like this? ``` template<typename T> static void grow_array(T **array, int *size, const int add_size) {} template<typename T> static void shring_array(T **array, int *size, const int shrink_size) {} ``` That would allow for simpler variable naming inside, and would tell the reader "there's nothing special going on here, just resizing some arrays" which makes it easier to skim
@ -730,7 +1058,7 @@ static void save_layer_to_storage(blender::bke::greasepencil::Layer &node,
*dst = reinterpret_cast<GreasePencilLayerTreeNode *>(new_leaf);
}
static void save_layer_group_to_storage(blender::bke::greasepencil::LayerGroup &node,
static void save_layer_group_to_storage(const blender::bke::greasepencil::LayerGroup &node,
GreasePencilLayerTreeNode **dst)
{
GreasePencilLayerTreeGroup *new_group = MEM_cnew<GreasePencilLayerTreeGroup>(__func__);
@ -738,7 +1066,7 @@ static void save_layer_group_to_storage(blender::bke::greasepencil::LayerGroup &
save_tree_node_to_storage(node, &new_group->base);
/* Save number of children. */
new_group->children_num = node.num_children();
new_group->children_num = node.num_direct_children();
/* Store pointer. */
*dst = reinterpret_cast<GreasePencilLayerTreeNode *>(new_group);
@ -748,25 +1076,24 @@ void GreasePencil::save_layer_tree_to_storage()
{
Hans Goudey commented 2023-04-25 04:26:00 +02:00
Outdated
Review
Copy Link

int n -> const int add_size

`int n` -> `const int add_size`
using namespace blender::bke::greasepencil;
/* We always store the root group, so we have to add one here. */
int num_tree_nodes = this->root_group().total_num_children() + 1;
int num_tree_nodes = this->root_group_for_write().num_children_total() + 1;
this->layer_tree_storage.nodes_num = num_tree_nodes;
this->layer_tree_storage.nodes = MEM_cnew_array<GreasePencilLayerTreeNode *>(num_tree_nodes,
__func__);
int i = 0;
save_layer_group_to_storage(this->root_group(), &this->layer_tree_storage.nodes[i++]);
for (TreeNode &node : this->root_group().children_in_pre_order()) {
GreasePencilLayerTreeNode **dst = &this->layer_tree_storage.nodes[i];
if (node.is_group()) {
LayerGroup &group = node.as_group();
save_layer_group_to_storage(group, dst);
}
else if (node.is_layer()) {
Layer &layer = node.as_layer();
save_layer_to_storage(layer, dst);
}
i++;
}
save_layer_group_to_storage(this->root_group_for_write(), &this->layer_tree_storage.nodes[0]);
Hans Goudey commented 2023-04-25 04:25:46 +02:00
Outdated
Review
Copy Link

IndexRange(new_drawings.size()) -> new_drawings.index_range()

Maybe regex this one? It's probably in this patch more than once

`IndexRange(new_drawings.size())` -> `new_drawings.index_range()` Maybe regex this one? It's probably in this patch more than once
this->root_group_for_write().foreach_children_with_index_pre_order(
[this](uint64_t i, TreeNode &node) {
GreasePencilLayerTreeNode **dst = &this->layer_tree_storage.nodes[i + 1];
if (node.is_group()) {
const LayerGroup &group = node.as_group();
save_layer_group_to_storage(group, dst);
}
else if (node.is_layer()) {
const Layer &layer = node.as_layer();
save_layer_to_storage(layer, dst);
Hans Goudey commented 2023-04-25 04:26:22 +02:00
Outdated
Review
Copy Link

Declare all arguments const in definitions

Declare all arguments const in definitions
}
});
this->layer_tree_storage.active_layer_index = this->runtime->active_layer_index();
}
Hans Goudey commented 2023-04-25 04:27:33 +02:00
Outdated
Review
Copy Link

Is the - meant to be a 1)? I think 1. is a bit more common in code comments than 1)

Is the `-` meant to be a `1)`? I think `1.` is a bit more common in code comments than `1)`
@ -811,10 +1138,11 @@ void GreasePencil::load_layer_tree_from_storage()
this->layer_tree_storage.nodes[0]);
BLI_assert(root->type == GREASE_PENCIL_LAYER_TREE_GROUP);
for (int i = 0; i < reinterpret_cast<GreasePencilLayerTreeGroup *>(root)->children_num; i++) {
read_layer_node_recursive(this->runtime->root_group(), this->layer_tree_storage.nodes, i + 1);
read_layer_node_recursive(
this->runtime->root_group_for_write(), this->layer_tree_storage.nodes, i + 1);
}
Hans Goudey commented 2023-05-02 23:51:09 +02:00
Outdated
Review
Copy Link

GreasePencilLayerMask *mask = &masks_storage -> GreasePencilLayerMask &mask = masks_storage

`GreasePencilLayerMask *mask = &masks_storage` -> `GreasePencilLayerMask &mask = masks_storage`
this->runtime->set_active_layer(this->layer_tree_storage.active_layer_index);
this->runtime->set_active_layer_index(this->layer_tree_storage.active_layer_index);
}
void GreasePencil::read_layer_tree_storage(BlendDataReader *reader)
@ -903,3 +1231,5 @@ void GreasePencil::free_layer_tree_storage()
this->layer_tree_storage.nodes = nullptr;
this->layer_tree_storage.nodes_num = 0;
}
Hans Goudey commented 2023-04-25 04:28:57 +02:00
Outdated
Review
Copy Link

These two things are handled by bke::CurvesGeometry::blend_read() now.

These two things are handled by `bke::CurvesGeometry::blend_read()` now.
/** \} */

4
source/blender/blenkernel/intern/grease_pencil_convert_legacy.cc
View File

@ -111,7 +111,7 @@ void legacy_gpencil_to_grease_pencil(GreasePencil &grease_pencil, bGPdata &gpd)
int i = 0, layer_idx = 0;
int active_layer_index = 0;
LISTBASE_FOREACH_INDEX (bGPDlayer *, gpl, &gpd.layers, layer_idx) {
Layer &new_layer = grease_pencil.root_group().add_layer(Layer(gpl->info));
Layer &new_layer = grease_pencil.root_group_for_write().add_layer(Layer(gpl->info));
if ((gpl->flag & GP_LAYER_ACTIVE) != 0) {
active_layer_index = layer_idx;
}
@ -135,7 +135,7 @@ void legacy_gpencil_to_grease_pencil(GreasePencil &grease_pencil, bGPdata &gpd)
}
}
grease_pencil.runtime->set_active_layer(active_layer_index);
grease_pencil.runtime->set_active_layer_index(active_layer_index);
}
} // namespace blender::bke::greasepencil::convert

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

@ -36,10 +36,10 @@ TEST(greasepencil, create_grease_pencil_id)
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(BKE_id_new(ctx.bmain, ID_GP, "GP"));
EXPECT_EQ(grease_pencil.drawings().size(), 0);
EXPECT_EQ(grease_pencil.root_group().total_num_children(), 0);
EXPECT_EQ(grease_pencil.root_group().num_children_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)
TEST(greasepencil, set_active_layer_index)
{
GreasePencilIDTestContext ctx;
@ -49,14 +49,14 @@ TEST(greasepencil, set_active_layer)
Layer layer1("Layer1");
Layer layer2("Layer2");
const Layer &layer1_ref = grease_pencil.root_group().add_layer(std::move(layer1));
const Layer &layer2_ref = grease_pencil.root_group().add_layer(std::move(layer2));
const Layer &layer1_ref = grease_pencil.root_group_for_write().add_layer(std::move(layer1));
const Layer &layer2_ref = grease_pencil.root_group_for_write().add_layer(std::move(layer2));
grease_pencil.runtime->set_active_layer(0);
grease_pencil.runtime->set_active_layer_index(0);
EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
EXPECT_STREQ(layer1_ref.name, grease_pencil.runtime->active_layer().name);
grease_pencil.runtime->set_active_layer(1);
grease_pencil.runtime->set_active_layer_index(1);
EXPECT_TRUE(grease_pencil.runtime->has_active_layer());
EXPECT_STREQ(layer2_ref.name, grease_pencil.runtime->active_layer().name);
@ -104,16 +104,16 @@ TEST(greasepencil, remove_drawing)
layer2.insert_frame(0, GreasePencilFrame{1});
grease_pencil.root_group().add_layer(std::move(layer1));
grease_pencil.root_group().add_layer(std::move(layer2));
grease_pencil.root_group_for_write().add_layer(std::move(layer1));
grease_pencil.root_group_for_write().add_layer(std::move(layer2));
grease_pencil.remove_drawing(1);
EXPECT_EQ(grease_pencil.drawings().size(), 2);
static int expected_frames_size[] = {2, 0};
static int expected_frames_pairs_layer0[][2] = {{0, 0}, {20, 1}};
int layer_i = 0;
grease_pencil.root_group().foreach_layer_pre_order([&](Layer &layer) {
for (const int layer_i : IndexRange(grease_pencil.layers().size())) {
const Layer &layer = *grease_pencil.layers()[layer_i];
EXPECT_EQ(layer.frames().size(), expected_frames_size[layer_i]);
if (layer_i == 0) {
for (const int i : IndexRange(2)) {
@ -121,8 +121,7 @@ TEST(greasepencil, remove_drawing)
expected_frames_pairs_layer0[i][1]);
}
}
layer_i++;
});
}
}
/* --------------------------------------------------------------------------------------------- */
@ -167,35 +166,23 @@ struct GreasePencilLayerTreeExample {
TEST(greasepencil, layer_tree_pre_order_iteration_callback)
{
GreasePencilLayerTreeExample ex;
int i = 0;
ex.root.foreach_children_pre_order(
[&](TreeNode &child) { EXPECT_EQ(child.name, ex.names[i++]); });
}
TEST(greasepencil, layer_tree_pre_order_iteration_loop)
{
GreasePencilLayerTreeExample ex;
int i = 0;
for (TreeNode &child : ex.root.children_in_pre_order()) {
EXPECT_EQ(child.name, ex.names[i++]);
}
ex.root.foreach_children_with_index_pre_order(
[&](uint64_t i, TreeNode &child) { EXPECT_EQ(child.name, ex.names[i]); });
}
TEST(greasepencil, layer_tree_total_size)
{
GreasePencilLayerTreeExample ex;
EXPECT_EQ(ex.root.total_num_children(), 7);
EXPECT_EQ(ex.root.num_children_total(), 7);
}
TEST(greasepencil, layer_tree_node_types)
{
GreasePencilLayerTreeExample ex;
int i = 0;
for (TreeNode &child : ex.root.children_in_pre_order()) {
ex.root.foreach_children_with_index_pre_order([&](uint64_t i, TreeNode &child) {
EXPECT_EQ(child.is_layer(), ex.is_layer[i]);
EXPECT_EQ(child.is_group(), !ex.is_layer[i]);
i++;
}
});
}
} // namespace blender::bke::greasepencil::tests

5
source/blender/draw/engines/gpencil/gpencil_object.hh
View File

@ -124,8 +124,9 @@ class ObjectModule {
}
uint layer_offset = layers_.object_offset_get();
grease_pencil.root_group().foreach_layer_pre_order(
[&](Layer &layer) { layers_.sync(object, layer, do_layer_blending); });
for (const Layer *layer : grease_pencil.layers()) {
layers_.sync(object, *layer, do_layer_blending);
}
/* Order rendering using camera Z distance. */
float3 position = float3(object->object_to_world[3]);

8
source/blender/editors/sculpt_paint/grease_pencil_paint.cc
View File

@ -49,10 +49,10 @@ struct PaintOperationExecutor {
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(ob_eval->data);
if (!grease_pencil.runtime->has_active_layer()) {
/* TODO: create a new layer. */
grease_pencil.runtime->set_active_layer(0);
grease_pencil.runtime->set_active_layer_index(0);
}
const bke::greasepencil::Layer &active_layer = grease_pencil.runtime->active_layer();
int index = active_layer.drawing_at(scene->r.cfra);
int index = active_layer.drawing_index_at(scene->r.cfra);
BLI_assert(index != -1);
GreasePencilDrawing &drawing = *reinterpret_cast<GreasePencilDrawing *>(
@ -90,8 +90,8 @@ void PaintOperation::on_stroke_done(const bContext &C)
grease_pencil_eval.runtime->has_active_layer());
const bke::greasepencil::Layer &active_layer_orig = grease_pencil_orig.runtime->active_layer();
const bke::greasepencil::Layer &active_layer_eval = grease_pencil_eval.runtime->active_layer();
int index_orig = active_layer_orig.drawing_at(scene->r.cfra);
int index_eval = active_layer_eval.drawing_at(scene->r.cfra);
int index_orig = active_layer_orig.drawing_index_at(scene->r.cfra);
int index_eval = active_layer_eval.drawing_index_at(scene->r.cfra);
BLI_assert(index_orig != -1 && index_eval != -1);
GreasePencilDrawing &drawing_orig = *reinterpret_cast<GreasePencilDrawing *>(

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

@ -203,7 +203,6 @@ typedef struct GreasePencil {
GreasePencilDrawingOrReference **drawing_array;
int drawing_array_size;
char _pad[4];
#ifdef __cplusplus
void read_drawing_array(BlendDataReader *reader);
void write_drawing_array(BlendWriter *writer);
@ -241,7 +240,11 @@ typedef struct GreasePencil {
void remove_drawing(int index);
void foreach_visible_drawing(int frame,
blender::FunctionRef<void(GreasePencilDrawing &)> function);
blender::bke::greasepencil::LayerGroup &root_group();
const blender::bke::greasepencil::LayerGroup &root_group() const;
blender::bke::greasepencil::LayerGroup &root_group_for_write();
blender::Span<const blender::bke::greasepencil::Layer *> layers() const;
blender::Span<blender::bke::greasepencil::Layer *> layers_for_write();
void tag_layer_tree_changed();
#endif
} GreasePencil;
Hans Goudey commented 2023-04-25 05:06:34 +02:00
Outdated
Review
Copy Link

I wonder if you'd consider making this an array rather than a ListBase in DNA. Then the memory could be reused when the file is loaded, fewer small allocations would be needed, and the C++ type wouldn't have to have next and prev pointers even during runtime.

I wonder if you'd consider making this an array rather than a `ListBase` in DNA. Then the memory could be reused when the file is loaded, fewer small allocations would be needed, and the C++ type wouldn't have to have `next` and `prev` pointers even during runtime.