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

View File

@ -8,6 +8,9 @@
*/
#include "BLI_map.hh"
#include "BLI_stack.hh"
#include "BLI_string.h"
#include "DNA_gpencil_legacy_types.h"
#include "DNA_grease_pencil_types.h"
@ -18,16 +21,189 @@ class GreasePencilLayerRuntime {
Map<int, int> frames;
};
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`?
namespace gpencil::convert {
namespace gpencil {
class TreeNode : public ::GreasePencilLayerTreeNode {
private:
Vector<TreeNode, 0> children_;
public:
TreeNode()
{
this->name = nullptr;
}
TreeNode(StringRefNull name)
{
this->name = BLI_strdup(name.c_str());
}
TreeNode(const TreeNode &other) : TreeNode(other.name)

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

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
}
return *this;
}
TreeNode &operator=(TreeNode &&other)
{
if (this != &other) {
std::swap(this->name, other.name);
other.name = nullptr;
}
return *this;
}
~TreeNode()
{
if (this->name) {
MEM_freeN(this->name);
}
}
public:
class PreOrderRange {
class Iterator {
using iterator_category = std::forward_iterator_tag;

Try putting this class out of line.

Try putting this class out of line.
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));
}
}
}
Review

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.
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));
}
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;

exclusive

`exclusive`
}
return (next_node_.size() == other.next_node_.size()) &&
next_node_.peek() == other.next_node_.peek();

I am a bit confused here.

Following the text, it seems it should be Frame and Drawing (not Time and Frame).

Or, the description needs to be updated (and a good argument made why the common terms Time and Frame are redefined in the grease pencil).

I am a bit confused here. Following the text, it seems it should be `Frame` and `Drawing` (not `Time` and `Frame`). Or, the description needs to be updated (and a good argument made why the common terms `Time` and `Frame` are redefined in the grease pencil).

Ah yes, Frame is the term we use for a Drawing at a particular time. I agree that this is very confusing here. I am not sure what a better term would be.

Ah yes, `Frame` is the term we use for a `Drawing` at a particular time. I agree that this is very confusing here. I am not sure what a better term would be.

I see. The GreasePencilFrame is nice and unambiguous. And within a context of GP it is not bad to stick to general rule "frame means grease pencil frame".

But when you refer to frame as a moment in time, is the best to use "scene frame". So, it will be something rhe first drawing starts at scene frame 0 and ends at scene frame 5 (exclusive) (btw exclusive not excusive)`.

Also use the same "scene frame" in diagram below. Generally time in Blender is scene frame number divided by the scene fps, and is measured in seconds.

I see. The `GreasePencilFrame` is nice and unambiguous. And within a context of GP it is not bad to stick to general rule "frame means grease pencil frame". But when you refer to frame as a moment in time, is the best to use "scene frame". So, it will be something `rhe first drawing starts at scene frame 0 and ends at scene frame 5 (exclusive)` (btw exclusive not excusive)`. Also use the same "scene frame" in diagram below. Generally time in Blender is scene frame number divided by the scene fps, and is measured in seconds.
}
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);
}
};
public:
bool operator==(const TreeNode &other) const
{
return this == &other;
}
bool operator!=(const TreeNode &other) const
{
return this != &other;
}

Better to return a span here maybe?

Better to return a span here maybe?
void add_child(TreeNode &node)
{
children_.append(node);
}
void add_child(TreeNode &&node)

Put out of line.

Put out of line.
{
children_.append(std::move(node));
}

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.
int size()
{
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;
}
PreOrderRange children_in_pre_order()
{
return PreOrderRange(this);
}
};
class Layer : TreeNode, ::GreasePencilLayer {};
namespace convert {
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.
CurvesGeometry legacy_gpencil_frame_to_curves_geometry(bGPDframe &gpf);
void legacy_gpencil_to_grease_pencil(bGPdata &gpd, GreasePencil &grease_pencil);
} // namespace gpencil::convert
} // namespace convert
} // namespace gpencil

Put out of line.

Put out of line.
} // namespace blender::bke
struct Main;
struct BoundBox;
void *BKE_grease_pencil_add(Main *bmain, const char *name);
BoundBox *BKE_grease_pencil_boundbox_get(Object *ob);

Put out of line.

Put out of line.