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

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

@ -47,7 +47,7 @@ class TreeNode : public ::GreasePencilLayerTreeNode, NonMovable {
/**
* \returns true if this node is a LayerGroup.
*/
constexpr bool is_group() const
const 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 == GP_LAYER_TREE_GROUP;
}
@ -55,7 +55,7 @@ class TreeNode : public ::GreasePencilLayerTreeNode, NonMovable {
/**
* \returns true if this node is a Layer.
*/
constexpr bool is_layer() const
const bool is_layer() const
{
return this->type == GP_LAYER_TREE_LEAF;
}
@ -272,7 +272,7 @@ class LayerGroup : public TreeNode {
namespace convert {
void legacy_gpencil_frame_to_grease_pencil_drawing(GreasePencilDrawing &drawing, bGPDframe &gpf);
void legacy_gpencil_frame_to_grease_pencil_drawing(const bGPDframe &gpf, GreasePencilDrawing &r_drawing);
void legacy_gpencil_to_grease_pencil(Main &main, GreasePencil &grease_pencil, bGPdata &gpd);
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 convert
@ -293,8 +293,8 @@ struct StrokePoint {
* Stroke cache for a stroke that is currently being drawn.
*/
struct StrokeCache {
Vector<StrokePoint> points = {};
Vector<uint3> triangles = {};
Vector<StrokePoint> points;
Hans Goudey commented 2023-05-02 23:36:18 +02:00
Outdated
Review
Copy Link

= {} 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
Vector<uint3> triangles;
Falk David commented 2023-04-12 10:41:37 +02:00
Outdated
Review
Copy Link

Put out of line.

Put out of line.
int mat = 0;
void clear()

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

@ -787,7 +787,7 @@ blender::Span<blender::uint3> GreasePencilDrawing::triangles() const
const bke::CurvesGeometry &curves = this->geometry.wrap();
const Span<float3> positions = curves.positions();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
Hans Goudey commented 2023-05-02 23:49:31 +02:00
Outdated
Review
Copy Link

offset_indices::OffsetIndices -> OffsetIndices

Please search the patch for this, I mentioned it last time too

`offset_indices::OffsetIndices` -> `OffsetIndices` Please search the patch for this, I mentioned it last time too
int total_triangles = 0;
Array<int> tris_offests(curves.curves_num());
@ -1141,9 +1141,9 @@ static void read_layer_from_storage(blender::bke::greasepencil::LayerGroup &curr
Vector<LayerMask> masks = new_layer.masks_for_write();
GreasePencilLayerMaskStorage *masks_storage = &node_leaf->layer.masks_storage;
for (int mask_i = 0; mask_i < masks_storage->masks_size; mask_i++) {
GreasePencilLayerMask *mask = &masks_storage->masks[mask_i];
LayerMask new_mask = LayerMask(mask->layer_name);
new_mask.flag = mask->flag;
GreasePencilLayerMask &mask = masks_storage->masks[mask_i];
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`
LayerMask new_mask = LayerMask(mask.layer_name);
new_mask.flag = mask.flag;
masks.append(std::move(new_mask));
}
new_layer.opacity = node_leaf->layer.opacity;
@ -1342,8 +1342,8 @@ void GreasePencil::read_layer_tree_storage(BlendDataReader *reader)
GreasePencilLayerMaskStorage *masks_storage = &node_leaf->layer.masks_storage;
BLO_read_data_address(reader, &masks_storage->masks);
for (int mask_i = 0; mask_i < masks_storage->masks_size; mask_i++) {
GreasePencilLayerMask *mask = &masks_storage->masks[mask_i];
BLO_read_data_address(reader, &mask->layer_name);
GreasePencilLayerMask &mask = masks_storage->masks[mask_i];
BLO_read_data_address(reader, &mask.layer_name);
}
break;
}
@ -1382,8 +1382,8 @@ void GreasePencil::write_layer_tree_storage(BlendWriter *writer)
BLO_write_struct_array(
writer, GreasePencilLayerMask, masks_storage->masks_size, masks_storage->masks);
for (int mask_i = 0; mask_i < masks_storage->masks_size; mask_i++) {
GreasePencilLayerMask *mask = &masks_storage->masks[mask_i];
BLO_write_string(writer, mask->layer_name);
GreasePencilLayerMask &mask = masks_storage->masks[mask_i];
BLO_write_string(writer, mask.layer_name);
}
break;
}
@ -1422,9 +1422,9 @@ void GreasePencil::free_layer_tree_storage()
}
GreasePencilLayerMaskStorage *masks_storage = &node_leaf->layer.masks_storage;
for (int mask_i = 0; mask_i < masks_storage->masks_size; mask_i++) {
GreasePencilLayerMask *mask = &masks_storage->masks[mask_i];
if (mask->layer_name) {
MEM_freeN(mask->layer_name);
GreasePencilLayerMask &mask = masks_storage->masks[mask_i];
if (mask.layer_name) {
MEM_freeN(mask.layer_name);
}
}
if (masks_storage->masks) {

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

@ -19,12 +19,12 @@
filedescriptor marked this conversation as resolved Outdated
Falk David commented 2023-04-12 10:53:05 +02:00
Outdated
Review
Copy Link

legacy_gpencil_frame_to_grease_pencil_drawing

`legacy_gpencil_frame_to_grease_pencil_drawing`
namespace blender::bke::greasepencil::convert {
void legacy_gpencil_frame_to_grease_pencil_drawing(GreasePencilDrawing &drawing, bGPDframe &gpf)
void legacy_gpencil_frame_to_grease_pencil_drawing(const bGPDframe &gpf, GreasePencilDrawing &r_drawing)
Hans Goudey commented 2023-05-02 23:41:46 +02:00
Outdated
Review
Copy Link

Order return argument last, make the gpf argument const

Order return argument last, make the `gpf` argument const
{
/* Construct an empty CurvesGeometry in-place. */
new (&drawing.geometry) CurvesGeometry();
drawing.base.type = GP_DRAWING;
drawing.runtime = MEM_new<bke::GreasePencilDrawingRuntime>(__func__);
new (&r_drawing.geometry) CurvesGeometry();
r_drawing.base.type = GP_DRAWING;
r_drawing.runtime = MEM_new<bke::GreasePencilDrawingRuntime>(__func__);
/* Get the number of points, number of strokes and the offsets for each stroke. */
Vector<int> offsets;
@ -38,7 +38,7 @@ void legacy_gpencil_frame_to_grease_pencil_drawing(GreasePencilDrawing &drawing,
}
/* Resize the CurvesGeometry. */
CurvesGeometry &curves = drawing.geometry.wrap();
CurvesGeometry &curves = r_drawing.geometry.wrap();
curves.resize(num_points, num_strokes);
if (num_strokes > 0) {
curves.offsets_for_write().copy_from(offsets);
@ -241,7 +241,7 @@ void legacy_gpencil_to_grease_pencil(Main &bmain, GreasePencil &grease_pencil, b
grease_pencil.drawing_array[i]);
/* Convert the frame to a drawing. */
legacy_gpencil_frame_to_grease_pencil_drawing(drawing, *gpf);
legacy_gpencil_frame_to_grease_pencil_drawing(*gpf, drawing);
GreasePencilFrame new_frame;
new_frame.drawing_index = i;

4
source/blender/draw/intern/draw_cache_impl_grease_pencil.cc
View File

@ -205,7 +205,7 @@ static void grease_pencil_batches_ensure(GreasePencil &grease_pencil, int cfra)
for (const int drawing_i : drawings.index_range()) {
const GreasePencilDrawing &drawing = *drawings[drawing_i];
const bke::CurvesGeometry &curves = drawing.geometry.wrap();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
Hans Goudey commented 2023-04-25 04:43:13 +02:00
Outdated
Review
Copy Link

More specific/helpful variable names than t and v would be nice here

More specific/helpful variable names than `t` and `v` would be nice here
const VArray<bool> cyclic = curves.cyclic();
int verts_start_offsets_size = curves.curves_num();
@ -282,7 +282,7 @@ static void grease_pencil_batches_ensure(GreasePencil &grease_pencil, int cfra)
const GreasePencilDrawing &drawing = *drawings[drawing_i];
const bke::CurvesGeometry &curves = drawing.geometry.wrap();
const bke::AttributeAccessor attributes = curves.attributes();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
const Span<float3> positions = curves.positions();
Hans Goudey commented 2023-04-25 04:16:22 +02:00
Outdated
Review
Copy Link

These .as_span() calls shouldn't be necessary

These `.as_span()` calls shouldn't be necessary
const VArray<bool> cyclic = curves.cyclic();
const VArray<float> radii = *attributes.lookup_or_default<float>(