Initial Grease Pencil 3.0 stage
#106848
Open
Falk David
wants to merge 222 commits from filedescriptor/blender:grease-pencil-v3
into main
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Adds the initial stage for the grease pencil 3.0 project.
This patch includes:
Exposed to the user:
Object
>Convert
menu.Draw Mode
.Some comments for myself reading through the entire thing.
@ -0,0 +1,28 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
Needs copyright.
@ -0,0 +1,480 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
Needs copyright.
@ -0,0 +20,4 @@
namespace blender::bke {
namespace gpencil {
Decide on the namespace here. Should it maybe be just
gp
?@ -0,0 +70,4 @@
}
public:
class PreOrderRange {
Try putting this class out of line.
@ -0,0 +160,4 @@
LayerGroup &as_group();
Layer &as_layer();
int total_num_children() const
Put out of line.
@ -0,0 +197,4 @@
}
private:
void foreach_children_pre_order_recursive_(TreeNodeIterFn function)
Put out of line.
@ -0,0 +205,4 @@
}
}
void foreach_layer_pre_order_recursive_(LayerIterFn function)
Put out of line.
@ -0,0 +246,4 @@
* 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.
*/
Map<int, int> frames_;
Since the map will have to change to store more than just an index as the value, make sure to update the comment too.
@ -0,0 +294,4 @@
return frames_for_write().add_overwrite(frame_number, index);
}
Span<int> sorted_keys()
Put out of line.
@ -0,0 +307,4 @@
return sorted_keys_cache_.data().as_span();
}
void load_from_storage(GreasePencilLayer &node)
Put out of line.
@ -0,0 +317,4 @@
/**
* Return the index of the drawing at frame \a frame or -1 if there is no drawing.
*/
int drawing_at(int frame)
Put out of line.
@ -0,0 +344,4 @@
public:
LayerGroup() : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP) {}
LayerGroup(StringRefNull name) : TreeNode(GREASE_PENCIL_LAYER_TREE_GROUP, name) {}
LayerGroup(const LayerGroup &other) : TreeNode(other)
Put out of line.
@ -0,0 +356,4 @@
}
}
}
LayerGroup(LayerGroup &&other) : TreeNode(std::move(other))
This needs to be implemented.
@ -0,0 +461,4 @@
}
public:
void *batch_cache = nullptr;
Could this be private?
@ -200,6 +200,7 @@ typedef struct Main {
ListBase pointclouds;
ListBase volumes;
ListBase simulations;
ListBase greasepencils;
Maybe
grease_pencils
?@ -1580,6 +1580,7 @@ GVArray CurvesGeometry::adapt_domain(const GVArray &varray,
/** \name File reading/writing.
* \{ */
Remove blank line.
@ -0,0 +41,4 @@
{
using namespace blender::bke;
// printf("grease_pencil_init_data\n");
Remove printfs.
@ -0,0 +135,4 @@
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BKE_LIB_FOREACHID_PROCESS_IDSUPER(data, drawing_reference->id_reference, IDWALK_CB_USER);
}
}
Needs to walk the parents of layers too (once added).
@ -0,0 +201,4 @@
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_read_id_address(reader, grease_pencil->id.lib, &drawing_reference->id_reference);
}
}
Needs to walk the parents of layers too (once added).
@ -0,0 +217,4 @@
reinterpret_cast<GreasePencilDrawingReference *>(drawing_or_ref);
BLO_expand(expander, drawing_reference->id_reference);
}
}
Needs to walk the parents of layers too (once added).
@ -0,0 +293,4 @@
float3 min(FLT_MAX);
float3 max(-FLT_MAX);
for (int i = 0; i < grease_pencil->drawing_array_size; i++) {
This should only consider the current frame.
@ -0,0 +301,4 @@
const blender::bke::CurvesGeometry &curves = drawing->geometry.wrap();
if (!curves.bounds_min_max(min, max)) {
min = float3(-1);
Make sure to not overwrite
min
andmax
but account any value already present. E.g. usemath::min_max(...)
.@ -0,0 +16,4 @@
namespace blender::bke::gpencil::convert {
void legacy_gpencil_frame_to_curves_geometry(GreasePencilDrawing &drawing, bGPDframe &gpf)
legacy_gpencil_frame_to_grease_pencil_drawing
@ -0,0 +38,4 @@
EXPECT_EQ(grease_pencil.drawings().size(), 0);
EXPECT_EQ(grease_pencil.root_group().total_num_children(), 0);
}
Add tests for
load_layer_tree_from_storage
andsave_layer_tree_to_storage
.@ -3936,7 +3950,6 @@ void BKE_object_minmax(Object *ob, float r_min[3], float r_max[3], const bool us
changed = true;
break;
}
Fix line
@ -315,2 +320,4 @@
BKE_volume_batch_cache_dirty_tag((struct Volume *)ob->data, BKE_VOLUME_BATCH_DIRTY_ALL);
break;
case OB_GREASE_PENCIL:
/* TODO: tag batches. */
Remove todo.
@ -2514,2 +2516,4 @@
break;
}
case ID_GP:
break;
This needs to build the relations for the the parent of layers.
@ -0,0 +31,4 @@
/** Instancing Data */
GPUVertBuf *vbo;
GPUVertBuf *vbo_col;
/** Indices in material order, then stroke order with fill first.
Update this comment. Out of date.
@ -0,0 +38,4 @@
/** Batches */
GPUBatch *geom_batch;
/** Stroke lines only */
GPUBatch *lines_batch;
Remove while unused?
@ -0,0 +66,4 @@
* The stroke data for this drawing.
*/
CurvesGeometry geometry;
#ifdef __cplusplus
Move below runtime data pointer.
@ -6525,1 +6525,4 @@
prop = RNA_def_property(srna, "use_grease_pencil_version3", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, NULL, "use_grease_pencil_version3", 1);
RNA_def_property_ui_text(prop, "Grease Pencil 3.0", "Enable the new grease pencil 3.0 codebase");
Enable the new Grease Pencil 3.0 object
?@blender-bot build
@ -0,0 +252,4 @@
*/
float tint_color[4];
/**
* Layer transfrom.
typo
transform
WIP: Initial Grease Pencil 3.0 stageto Initial Grease Pencil 3.0 stage@blender-bot build
@ -0,0 +219,4 @@
* Adds a layer at the end of this group and returns it.
*/
Layer &add_layer(Layer &layer);
Layer &add_layer(Layer &&layer);
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.
I think
emplace_layer
would be a better name indeed.@ -0,0 +300,4 @@
class GreasePencilRuntime {
public:
mutable SharedCache<Vector<greasepencil::Layer *>> layer_cache_;
Looks like this should be private maybe? It has a
_
suffix.@ -0,0 +323,4 @@
void ensure_layer_cache() const;
void load_layer_tree_from_storage(GreasePencilLayerTreeStorage &storage);
void save_layer_tree_to_storage(GreasePencilLayerTreeStorage &storage);
These two functions should never have to be called outside of the
ID
callbacks ingrease_pencil.cc
; IMO they make more sense as static functions there. Probably better to keep that storage for DNA thing as localized as possible.@ -0,0 +328,4 @@
void tag_layer_tree_topology_changed();
public:
void *batch_cache = nullptr;
Member variables come before functions https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Class_Layout
@ -0,0 +155,4 @@
BLO_write_id_struct(writer, GreasePencil, id_address, &grease_pencil->id);
BKE_id_blend_write(writer, &grease_pencil->id);
/* Write drawings. */
Here the comment says "Write drawings" and the code says "write drawing array". That's basically the same thing-- the comment isn't really necessary. Same with the others here IMO.
Maybe this comment was missed? Or do you disagree?
@ -0,0 +424,4 @@
Span<int> Layer::sorted_keys() const
{
this->sorted_keys_cache_.ensure([&](Vector<int> &r_data) {
r_data.clear_and_shrink();
Any reason to call
clear_and_shrink
here? Might as well callreinitialize
if not@ -0,0 +431,4 @@
}
std::sort(r_data.begin(), r_data.end());
});
return this->sorted_keys_cache_.data().as_span();
.as_span()
shouldn't be necessary here.@ -0,0 +482,4 @@
void LayerGroup::add_group(LayerGroup &&group)
{
this->children.append(std::make_unique<LayerGroup>(group));
I believe it's standard to
std::move
from aT &&
Is there a need for overloads for
T &
andT &&
? What's the benefit of that?@ -0,0 +594,4 @@
GreasePencilRuntime::GreasePencilRuntime(const GreasePencilRuntime &other)
: root_group_(other.root_group_), active_layer_index_(other.active_layer_index_)
{
/* We cannot copy this cache, because the pointers in that cache become invalid. */
It probably shouldn't be a
SharedCache
then. Either that or it should cache indices rather than pointers?Ah yes, I don't see a way it can be a shared cache then.
@ -0,0 +956,4 @@
{
using namespace blender;
const bke::GreasePencilDrawingRuntime &runtime = *this->runtime;
runtime.triangles_cache.ensure([&](Vector<uint3> &cache) {
cache
is namedr_data
in other shared cache "ensure" functions, might as well be consistent with that@ -0,0 +964,4 @@
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
int total_triangles = 0;
for (int curve_i : curves.curves_range()) {
int curve_i
->const int curve_i
@ -0,0 +965,4 @@
int total_triangles = 0;
for (int curve_i : curves.curves_range()) {
IndexRange points = points_by_curve[curve_i];
IndexRange points
->const IndexRange points
@ -0,0 +984,4 @@
const int num_trinagles = points.size() - 2;
uint(*tris)[3] = static_cast<uint(*)[3]>(
BLI_memarena_alloc(pf_arena, sizeof(*tris) * size_t(num_trinagles)));
Pass a slice of the result vector to
BLI_polyfill_calc_arena
directly rather than allocating a temporary array here@ -0,0 +1034,4 @@
/** \name Grease Pencil data-block API
* \{ */
static void grease_pencil_grow_drawing_array_by(GreasePencil &self, const int add_capacity)
What about replacing these with slightly more generic functions like this?
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
@ -0,0 +1073,4 @@
this->drawing_array_size};
}
void GreasePencil::add_empty_drawings(int n)
int n
->const int add_size
@ -0,0 +1081,4 @@
grease_pencil_grow_drawing_array_by(*this, n);
MutableSpan<GreasePencilDrawingBase *> new_drawings = this->drawings_for_write().drop_front(
prev_size);
for (const int i : IndexRange(new_drawings.size())) {
IndexRange(new_drawings.size())
->new_drawings.index_range()
Maybe regex this one? It's probably in this patch more than once
@ -0,0 +1091,4 @@
}
}
void GreasePencil::remove_drawing(int index_to_remove)
Declare all arguments const in definitions
@ -0,0 +1096,4 @@
using namespace blender::bke::greasepencil;
/* In order to not change the indices of the drawings, we do the following to the drawing to be
* removed:
* - If the drawing (A) is not the last one:
Is the
-
meant to be a1)
? I think1.
is a bit more common in code comments than1)
@ -0,0 +1230,4 @@
GreasePencilDrawing *drawing = reinterpret_cast<GreasePencilDrawing *>(drawing_base);
drawing->geometry.wrap().blend_read(*reader);
/* Initialize runtime data. */
drawing->geometry.runtime = MEM_new<blender::bke::CurvesGeometryRuntime>(__func__);
These two things are handled by
bke::CurvesGeometry::blend_read()
now.@ -0,0 +1233,4 @@
drawing->geometry.runtime = MEM_new<blender::bke::CurvesGeometryRuntime>(__func__);
drawing->geometry.wrap().update_curve_types();
drawing->runtime = MEM_new<blender::bke::GreasePencilDrawingRuntime>(__func__);
drawing->runtime->triangles_cache.tag_dirty();
Dirty is the initial state of a
SharedCache
, no need to tag it that way explicitly here.@ -0,0 +1281,4 @@
drawing->geometry.wrap().~CurvesGeometry();
MEM_delete(drawing->runtime);
drawing->runtime = nullptr;
MEM_freeN(drawing);
Looks like drawings are allocated with
MEM_new
above, which means they should be deallocated withMEM_delete
.@ -0,0 +195,4 @@
int total_points_num = 0;
int total_triangles_num = 0;
int v = 0;
Vector<Vector<int>> verts_start_offsets_per_visible_drawing;
Looks like this can be
Vector<Array<int>>
, which is better since they don't need to be resized anyway@ -0,0 +197,4 @@
int v = 0;
Vector<Vector<int>> verts_start_offsets_per_visible_drawing;
Vector<Vector<int>> tris_start_offsets_per_visible_drawing;
grease_pencil.foreach_visible_drawing(cfra, [&](GreasePencilDrawing &drawing) {
What about building a vector of the drawings once, to avoid putting most of the code inside the lambda here and the need to call "foreach_visible_drawing" twice:
@ -0,0 +205,4 @@
Vector<int> tris_start_offsets(curves.curves_num());
/* Calculate the vertex and triangle offsets for all the curves. */
int t = 0;
More specific/helpful variable names than
t
andv
would be nice here@ -0,0 +221,4 @@
}
verts_start_offsets[curve_i] = v;
v += 1 + points.size() + (is_cyclic ? 1 : 0) + 1;
Not clear what's going on here--
1 + ... + 1
?Looks like my comment about this got lost somehow. There is one extra vertex before and after each stroke. It's written in this way to make it more clear where that padding is added.
@ -0,0 +271,4 @@
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
const Span<float3> positions = curves.positions();
const VArray<bool> cyclic = curves.cyclic();
const VArray<float> radii = attributes.lookup_or_default<float>(
Use the
*
overload rather than adding.varray
at the end@ -0,0 +283,4 @@
"material_index", ATTR_DOMAIN_CURVE, -1).varray;
const Span<uint3> triangles = drawing.triangles();
const Span<int> verts_start_offsets =
verts_start_offsets_per_visible_drawing[drawing_i].as_span();
These
.as_span()
calls shouldn't be necessary@ -688,6 +688,9 @@ static bool ED_object_editmode_load_free_ex(Main *bmain,
else if (obedit->type == OB_CURVES) {
/* Curves don't have specific edit mode data, so pass. */
}
else if (obedit->type == OB_GREASE_PENCIL) {
This could be added to the existing
OB_CURVES
check withELEM
@ -0,0 +44,4 @@
Scene *scene = CTX_data_scene(&C);
ARegion *region = CTX_wm_region(&C);
Object *obact = CTX_data_active_object(&C);
Object *ob_eval = DEG_get_evaluated_object(depsgraph, obact);
It seems a bit weird to modifier the evaluated object here. Is that purposeful? Or am I missing something?
Yes, this is done on purpose so that the extra copy from orig to eval for rendering is not needed every update. Once drawing is done, the stroke is copied from the eval buffer to orig.
Okay, makes sense. This definitely deserves a comment, since typically changing the evaluated data isn't a good idea.
@ -0,0 +109,4 @@
curves.offsets_for_write()[num_old_curves] = num_old_points;
curves.offsets_for_write()[num_old_curves + 1] = num_old_points + stroke_points.size();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
offset_indices::OffsetIndices
->OffsetIndices
@ -0,0 +82,4 @@
/**
* Number of users of this drawing in the layer tree.
*/
int32_t user_count;
I wonder if this user count could be runtime data? Or if not, maybe it's worth mentioning why it isn't in a comment.
I thought about this, but I don't think it can be runtime data. Since there will be keyframe instances, we need to make the user count is saved.
The connection between keyframe instances and saving the user count isn't super clear to me. Maybe a comment about what the user count is used for here would help?
Maybe it's still better to have this as runtime data. If there is ever a bug where this doesn't get counted correctly, at least a file read could fix it. It should be possible to read this as zero, and increment it for every user?
This is easy to change afterwards though.
I see, re-creating the user count when reading could work indeed.
@ -0,0 +109,4 @@
/**
* A buffer for a single stroke while drawing.
*/
blender::Span<blender::bke::StrokePoint> stroke_buffer();
What about a single function that returned
std::optional
rather than a separate "has_" function?@ -0,0 +110,4 @@
* A buffer for a single stroke while drawing.
*/
blender::Span<blender::bke::StrokePoint> stroke_buffer();
bool has_stroke_buffer();
Looks like
stroke_buffer()
can beconst
@ -0,0 +180,4 @@
struct GreasePencilLayerMask *next, *prev;
/**
* The name of the layer that is the mask.
* \note Null-terminated.
Pretty sure null-terminated goes without saying for
char *
pointers in DNA, I don't think it's worth mentioning hereNot sure about dynamic names for strings. It is not something typically used in the DNA.
It's been done more recently I think. It's properly integrated with RNA now too, to avoid that boilerplate. It's nice not to have to worry about choosing a future-proof length.
@ -0,0 +247,4 @@
/**
* List of `GreasePencilLayerMask`. Only used for storage in the .blend file.
*/
ListBase masks_storage;
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 havenext
andprev
pointers even during runtime.@ -0,0 +406,4 @@
struct AnimData *adt;
/**
* An array of pointers to drawings. The drawing can own it's data or reference it from another
Grammar:
it's data
->its data
@ -0,0 +414,4 @@
int drawing_array_size;
char _pad[4];
#ifdef __cplusplus
void read_drawing_array(BlendDataReader *reader);
Best to follow the style guidelines and put all methods at the bottom.
I think that's best since you can easily see what's actually stored in the class without digging through a bunch of methods.
@ -0,0 +446,4 @@
*/
GreasePencilRuntimeHandle *runtime;
#ifdef __cplusplus
blender::Span<GreasePencilDrawingBase *> drawings() const;
It probably shouldn't be possible to get a span of non-const pointers from a const
GreasePencil
:Span<const GreasePencilDrawingBase *>
@ -0,0 +449,4 @@
blender::Span<GreasePencilDrawingBase *> drawings() const;
blender::MutableSpan<GreasePencilDrawingBase *> drawings_for_write();
void add_empty_drawings(int n);
void remove_drawing(int index);
This sort of API function (dealing with one index at a time) can be a bit dangerous since it can easily lead to quadratic behavior if it's called many times. I wonder if a
remove_drawings(IndexMask drawings_to_remove)
function would work a bit betterA quick pass of review.
The most unclear part is the amount of TODOs, and some dead/unfinished code. Are those intended to be solved before the merge? If not, it would be nice to have some brief wrap up about what those are about.
Like, not
/* TODO */
but/* TODO: Support reading intent of artists. */
, or/* Reference code implementation */
.@ -0,0 +117,4 @@
*
* | | | | | | | | | | |1|1|1|1|1|1|1|
* Time: |0|1|2|3|4|5|6|7|8|9|0|1|2|3|4|5|6|...
* Frame: [#0 ][#1 ] [#2 ]
I am a bit confused here.
Following the text, it seems it should be
Frame
andDrawing
(notTime
andFrame
).Or, the description needs to be updated (and a good argument made why the common terms
Time
andFrame
are redefined in the grease pencil).Ah yes,
Frame
is the term we use for aDrawing
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.
@ -0,0 +151,4 @@
}
}
radii.finish();
I thought we had some RAII helper which does
finish()
at the end of the scope.Or maybe it was just an idea to have one? Maybe Jacques of Hans remember better :)
Right now regular we do this manually, but true, this could be better to use
BLI_SCOPED_DEFER
.We don't really want to do non-trivial work in a class's destructor. It's also helpful to make this explicit, since sometimes it matters when it happens.
I don't really understand it, and can't say I'm sold on the motivation.
But in any case, at this point the discussion does not affect the PR, so lets consider it resolved here, and any followup we can do in the chat instead.
@ -0,0 +16,4 @@
/* --------------------------------------------------------------------------------------------- */
/* Grease Pencil ID Tests. */
struct GreasePencilIDTestContext {
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 literallygreasepencil
.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.@ -0,0 +1,154 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2022 Blender Foundation. */
2023
@ -0,0 +77,4 @@
const float *sizeinv = DRW_viewport_invert_size_get();
const float4 metrics = {sizeinv[0], sizeinv[1], size[0], size[1]};
if (anti_aliasing_enabled_) {
I would totally split the code into smaller functions. I do not see what are we winning from such inlined code.
@ -0,0 +136,4 @@
manager.submit(edge_detect_ps_);
}
if (anti_aliasing_enabled_) {
Why this extra check is needed?
@ -0,0 +1,325 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2022 Blender Foundation. */
2023
@ -0,0 +19,4 @@