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
3 changed files with 214 additions and 87 deletions
Showing only changes of commit d60b604189 - Show all commits

View File

@ -7,6 +7,7 @@
* \brief Low-level operations for grease pencil.
*/
#include "BLI_function_ref.hh"
#include "BLI_map.hh"
#include "BLI_stack.hh"
#include "BLI_string.h"
@ -16,37 +17,43 @@
namespace blender::bke {
class GreasePencilLayerRuntime {
public:
Map<int, int> frames;
};
namespace gpencil {
class LayerGroup;
class Layer;
filedescriptor marked this conversation as resolved Outdated

Decide on the namespace here. Should it maybe be just gp?

Decide on the namespace here. Should it maybe be just `gp`?
class TreeNode : public ::GreasePencilLayerTreeNode {
private:
using ItemIterFn = FunctionRef<void(TreeNode &)>;
protected:
Vector<TreeNode, 0> children_;
public:
TreeNode()
TreeNode(GreasePencilLayerTreeNodeType type)
{
this->type = type;
this->name = nullptr;
}
TreeNode(StringRefNull name)
TreeNode(GreasePencilLayerTreeNodeType type, StringRefNull name)
{
this->type = type;

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?

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.
this->name = BLI_strdup(name.c_str());
}
TreeNode(const TreeNode &other) : TreeNode(other.name)
TreeNode(const TreeNode &other)
{
this->name = BLI_strdup(other.name);
}
TreeNode(TreeNode &&other)
{
std::swap(this->name, other.name);
this->name = other.name;
other.name = nullptr;
}

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?

The const in the const bool return type means nothing here

The `const` in the `const bool` return type means nothing here
TreeNode &operator=(const TreeNode &other)
{
if (this != &other) {
if (this->name) {
MEM_freeN(this->name);
}
this->name = BLI_strdup(other.name);
}
return *this;
@ -54,7 +61,7 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
TreeNode &operator=(TreeNode &&other)
{
if (this != &other) {
std::swap(this->name, other.name);
this->name = other.name;
other.name = nullptr;
}
return *this;
@ -146,48 +153,174 @@ class TreeNode : public ::GreasePencilLayerTreeNode {
};
public:
bool operator==(const TreeNode &other) const
constexpr bool is_group() const

Better to return a span here maybe?

Better to return a span here maybe?
{
return this == &other;
}
bool operator!=(const TreeNode &other) const
{
return this != &other;
return type == GREASE_PENCIL_LAYER_TREE_GROUP;
}
void add_child(TreeNode &node)
constexpr bool is_layer() const
{
children_.append(node);
return type == GREASE_PENCIL_LAYER_TREE_LEAF;

Put out of line.

Put out of line.
}
void add_child(TreeNode &&node)
LayerGroup &as_group()

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.
{
children_.append(std::move(node));
return *static_cast<LayerGroup *>(this);
}
int size()
Layer &as_layer()
{
return children_.size();
}
bool remove_child(TreeNode &node)
{
BLI_assert(children_.size() != 0);
int64_t index = children_.first_index_of_try(node);
if (index < 0) {
return false;
}
children_.remove(index);
return true;
return *static_cast<Layer *>(this);
}
PreOrderRange children_in_pre_order()
{
return PreOrderRange(this);
}
void foreach_children_pre_order(ItemIterFn function)
{
for (TreeNode &child : children_) {
child.foreach_children_pre_order_recursive_(function);
}
}
void foreach_leaf_pre_order(ItemIterFn function)
{
for (TreeNode &child : children_) {
child.foreach_leaf_pre_order_recursive_(function);
}
}
filedescriptor marked this conversation as resolved Outdated

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.
private:
void foreach_children_pre_order_recursive_(ItemIterFn function)
{
function(*this);
for (TreeNode &child : children_) {
child.foreach_children_pre_order_recursive_(function);
}

Put out of line.

Put out of line.
}
void foreach_leaf_pre_order_recursive_(ItemIterFn function)
{
if (children_.size() == 0) {
function(*this);
}
for (TreeNode &child : children_) {

Put out of line.

Put out of line.
child.foreach_children_pre_order_recursive_(function);
}

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.
}
};
class Layer : TreeNode, ::GreasePencilLayer {};
class Layer : public TreeNode, ::GreasePencilLayer {
private:
Map<int, int> frames_;
public:
Layer() : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF), frames_()
{
}
Layer(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF, name), frames_()

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

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.

I think emplace_layer would be a better name indeed.

I think `emplace_layer` would be a better name indeed.
{
}
Layer(const Layer &other)
: TreeNode(GREASE_PENCIL_LAYER_TREE_LEAF, other.name), frames_(other.frames_)
{
}
Layer(Layer &&other) : TreeNode(std::move(other))
{
frames_ = std::move(other.frames_);
}
Layer &operator=(const Layer &other)
{
return copy_assign_container(*this, other);
}
Layer &operator=(Layer &&other)
{
if (this != &other) {
frames_ = std::move(other.frames_);
}
return *this;
}
bool operator==(const Layer &other) const
{
return this == &other;
}
bool operator!=(const Layer &other) const
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.
{
return this != &other;
}
Layer &as_layer()
{
return *this;
}
};
class LayerGroup : public TreeNode {
public:
LayerGroup() : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP)
{
}
LayerGroup(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, name)
{
}
LayerGroup(const LayerGroup &other) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, other.name)
{
}
LayerGroup(LayerGroup &&other) : TreeNode(std::move(other))
{
}
~LayerGroup()
{
}

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`.
public:
bool operator==(const LayerGroup &other) const
{
return this == &other;
}
bool operator!=(const LayerGroup &other) const
{
return this != &other;
}
void add_group(LayerGroup &group)
{
children_.append(group);
}
void add_group(LayerGroup &&group)
{
children_.append(std::move(group));
}

= {} 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.
void add_layer(Layer &layer)
{
children_.append(layer);
}
void add_layer(Layer &&layer)

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

Looks like this should be private maybe? It has a `_` suffix.
{
children_.append(std::move(layer));
}
int num_children()
{
return children_.size();

Put out of line.

Put out of line.
}
void remove_child(int64_t index)
{
BLI_assert(index >= 0 && index < children_.size());
children_.remove(index);
}
};
class LayerTree {

Put out of line.

Put out of line.
private:
LayerGroup root_;
};
filedescriptor marked this conversation as resolved Outdated

Same remark as above about commented out code.

Same remark as above about commented out code.
namespace convert {

These two functions should never have to be called outside of the ID callbacks in grease_pencil.cc; IMO they make more sense as static functions there. Probably better to keep that storage for DNA thing as localized as possible.

These two functions should never have to be called outside of the `ID` callbacks in `grease_pencil.cc`; IMO they make more sense as static functions there. Probably better to keep that storage for DNA thing as localized as possible.
@ -199,6 +332,11 @@ void legacy_gpencil_to_grease_pencil(bGPdata &gpd, GreasePencil &grease_pencil);
} // namespace gpencil
class GreasePencilRuntime {
private:
gpencil::LayerTree layer_tree_;
};
} // namespace blender::bke
struct Main;
@ -206,8 +344,3 @@ struct BoundBox;
void *BKE_grease_pencil_add(Main *bmain, const char *name);
BoundBox *BKE_grease_pencil_boundbox_get(Object *ob);
inline const blender::Map<int, int> &GreasePencilLayer::frames() const
{
return this->runtime->frames;
}

View File

@ -11,18 +11,22 @@ namespace blender::bke::gpencil::tests {
TEST(gpencil, build_layer_tree)
{
TreeNode root{};
LayerGroup root{};
TreeNode node("Node1");
node.add_child(TreeNode("Child1"));
node.add_child(TreeNode("Child2"));
LayerGroup group("Group1");
group.add_layer(Layer("Child1"));
group.add_layer(Layer("Child2"));
Use text fixtures: http://google.github.io/googletest/primer.html#same-data-multiple-tests

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

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.
root.add_child(std::move(node));
root.add_child(TreeNode("Node2"));
root.add_group(std::move(group));
root.add_layer(Layer("Group2"));
for (TreeNode &node : root.children_in_pre_order()) {
std::cout << node.name << std::endl;
}
root.foreach_children_pre_order([](LayerGroup &child) { std::cout << child.name << std::endl; });
// root.remove_child(0);
// for (LayerGroup &child : root.children_in_pre_order()) {
// std::cout << child.name << std::endl;
// }
}
} // namespace blender::bke::gpencil::tests

View File

@ -13,10 +13,8 @@
# include "BLI_map.hh"
# include "BLI_span.hh"
namespace blender::bke {
class GreasePencilLayerRuntime;
class GreasePencilRuntime;
} // namespace blender::bke
using GreasePencilLayerRuntimeHandle = blender::bke::GreasePencilLayerRuntime;
using GreasePencilRuntimeHandle = blender::bke::GreasePencilRuntime;
#else
typedef struct GreasePencilLayerRuntimeHandle GreasePencilLayerRuntimeHandle;
@ -69,22 +67,6 @@ typedef struct GreasePencilDrawingReference {
struct GreasePencil *id_reference;
} GreasePencilDrawingReference;
filedescriptor marked this conversation as resolved Outdated

Move below runtime data pointer.

Move below runtime data pointer.
/* Only used for storage in the .blend file. */
typedef struct GreasePencilLayerFramesMapStorage {
/* Array of `frames` keys (sorted in ascending order). */
int *keys;
/* Array of `frames` values (order matches the keys array). */
int *values;
/* Size of the map (number of key-value pairs). */
int size;
char _pad[4];
} GreasePencilLayerFramesMapStorage;
/**
* A grease pencil layer is a collection of drawings mapped to a specific time on the timeline.
*/
typedef struct GreasePencilLayer {
#ifdef __cplusplus
/**
* This Map maps a scene frame number (key) to an index into GreasePencil->drawings (value). The
* frame number indicates the first frame the drawing is shown. The end time is implicitly
@ -109,14 +91,22 @@ typedef struct GreasePencilLayer {
* referenced drawings are discarded. If the frame is longer than the number of referenced
* drawings, then the last referenced drawing is held for the rest of the duration.
*/
const blender::Map<int, int> &frames() const;
#endif
GreasePencilLayerFramesMapStorage frames_storage;
typedef struct GreasePencilLayerFramesMapStorage {
/* Array of `frames` keys (sorted in ascending order). */
int *keys;
/* Array of `frames` values (order matches the keys array). */
int *values;
/* Size of the map (number of key-value pairs). */
int size;
char _pad[4];
} GreasePencilLayerFramesMapStorage;
/**
* Runtime struct pointer.
* A grease pencil layer is a collection of drawings mapped to a specific time on the timeline.
*/
GreasePencilLayerRuntimeHandle *runtime;
typedef struct GreasePencilLayer {
/* Only used for storage in the .blend file. */
GreasePencilLayerFramesMapStorage frames_storage;
} GreasePencilLayer;
typedef enum GreasePencilLayerTreeNodeType {

What about a single function that returned std::optional rather than a separate "has_" function?

What about a single function that returned `std::optional` rather than a separate "has_" function?