GPv3: Weight Paint tools (Draw, Blur, Average, Smear, Sample weight) #118347

Merged
Falk David merged 48 commits from SietseB/blender:gpv3-weight-paint-tools into main 2024-04-25 15:21:26 +02:00
Member

This PR implements the Weight Paint tools for GPv3.

Tools:

  • Draw, for assigning weight to stroke points
  • Blur, smooths out weight using adjacent stroke point weights
  • Average, smooths weight using the average weight under the brush
  • Smear, like finger painting, drags weights in the direction of the brush
  • Sample weight, sets the brush weight to the weight under the cursor

The weights are assigned to the active vertex group. When there is no
active vertex group, a group is automatically created.

When the Auto Normalize option is enabled, it is ensured that all
bone-deforming vertex groups add up to the weight of 1.0.
When a vertex group is locked, it's weights will not be altered by
Auto Normalize.

The PR already supports multi frame editing, including the use of a
falloff (defined by a curve).

The implementation is in accordance with the Weight Paint tools in GPv2.

This PR implements the Weight Paint tools for GPv3. Tools: - Draw, for assigning weight to stroke points - Blur, smooths out weight using adjacent stroke point weights - Average, smooths weight using the average weight under the brush - Smear, like finger painting, drags weights in the direction of the brush - Sample weight, sets the brush weight to the weight under the cursor The weights are assigned to the active vertex group. When there is no active vertex group, a group is automatically created. When the Auto Normalize option is enabled, it is ensured that all bone-deforming vertex groups add up to the weight of 1.0. When a vertex group is locked, it's weights will not be altered by Auto Normalize. The PR already supports multi frame editing, including the use of a falloff (defined by a curve). The implementation is in accordance with the Weight Paint tools in GPv2.
Sietse Brouwer added 15 commits 2024-02-15 21:38:43 +01:00
Sietse Brouwer added this to the Grease Pencil project 2024-02-15 21:39:25 +01:00
Sietse Brouwer requested review from Falk David 2024-02-15 21:39:33 +01:00
Author
Member

Note: this PR doesn't contain the viewport overlay for showing vertex weights. The overlay code is in a separate PR: #118273: GPv3: Overlay for Weight Paint mode.
So, for testing the tools, as long as that PR isn't merged in main, the advise is to merge that PR into the build, otherwise there isn't much to see.

Note: this PR doesn't contain the viewport overlay for showing vertex weights. The overlay code is in a separate PR: [#118273: GPv3: Overlay for Weight Paint mode](https://projects.blender.org/blender/blender/pulls/118273). So, for testing the tools, as long as that PR isn't merged in `main`, the advise is to merge that PR into the build, otherwise there isn't much to see.
Author
Member

<video src="/attachments/03b7d1e0-3aec-4dd3-8405-0ead82f59cfc" title="Weight Paint tools GPv3.mp4" controls></video>
Falk David requested changes 2024-02-16 13:53:31 +01:00
Dismissed
Falk David left a comment
Member

Thank you for working on this and getting a PR up so quickly!

Did a first pass on this. I think my biggest concern is that there is one paint operation for all the tools.

I would much prefer if each tool was their own GreasePencilStrokeOperation. I can see that there are many places where the weightpaint_tool is checked (inside the operation) to do something specific for that tool. Seperating them would make this much cleaner and easier to follow.

After seperating them, it should also be more clear what code can and cannot be shared.

Thank you for working on this and getting a PR up so quickly! Did a first pass on this. I think my biggest concern is that there is one paint operation for all the tools. I would much prefer if each tool was their own `GreasePencilStrokeOperation`. I can see that there are many places where the `weightpaint_tool` is checked (inside the operation) to do something specific for that tool. Seperating them would make this much cleaner and easier to follow. After seperating them, it should also be more clear what code can and cannot be shared.
@ -320,0 +331,4 @@
start_sample.mouse_position = float2(mouse);
start_sample.pressure = 0.0f;
GreasePencilStrokeOperation *operation = greasepencil::new_weight_paint_operation().release();
Member

There should be a switch statement with the different tools here. Each tool creating a GreasePencilStrokeOperation e.g. new_weight_paint_blur_operation()
The implementation of each tool should be in it's own file.

There should be a switch statement with the different tools here. Each tool creating a `GreasePencilStrokeOperation` e.g. `new_weight_paint_blur_operation()` The implementation of each tool should be in it's own file.
SietseB marked this conversation as resolved
@ -22,2 +22,4 @@
virtual void on_stroke_extended(const bContext &C, const InputSample &extension_sample) = 0;
virtual void on_stroke_done(const bContext &C) = 0;
BrushStrokeMode brush_mode;
Member

The idea is that GreasePencilStrokeOperation is a pure virutal class.
Instead, you can just pass this mode to the constructor of your class that implements GreasePencilStrokeOperation.

The idea is that `GreasePencilStrokeOperation` is a pure virutal class. Instead, you can just pass this mode to the constructor of your class that implements `GreasePencilStrokeOperation`.
SietseB marked this conversation as resolved
@ -0,0 +529,4 @@
DrawingWeightData &drawing_weight = drawing_weights[drawing_index];
for (const BrushPoint &point : drawing_weight.points_in_brush) {
switch (this->brush->weightpaint_tool) {
Member

These cases should be their own GreasePencilStrokeOperation.

These cases should be their own `GreasePencilStrokeOperation`.
SietseB marked this conversation as resolved
Hans Goudey requested changes 2024-02-16 18:16:41 +01:00
Hans Goudey left a comment
Member

I didn't read the brush implementation yet, but I did a quick read-through of the rest of the PR.

I didn't read the brush implementation yet, but I did a quick read-through of the rest of the PR.
@ -321,0 +380,4 @@
}
if (!editable_drawings.is_empty()) {
drawings_per_frame.append(editable_drawings);
Member

std::move(editable_drawings)

`std::move(editable_drawings)`
SietseB marked this conversation as resolved
@ -545,0 +627,4 @@
const bke::AttributeAccessor attributes = curves.attributes();
/* Propagate the material index to the points. */
const VArray<int> materials = *attributes.lookup<int>("material_index", bke::AttrDomain::Point);
Member

I'd suggest using lookup_or_default here (with 0 as the default), then using get_if_single after to support any single value (in the future we might store attributes with single values). It's also just a bit safer/cleaner-feeling than dealing with empty-ness

I'd suggest using `lookup_or_default` here (with 0 as the default), then using `get_if_single` after to support any single value (in the future we might store attributes with single values). It's also just a bit safer/cleaner-feeling than dealing with empty-ness
SietseB marked this conversation as resolved
@ -0,0 +47,4 @@
BKE_object_defgroup_add_name(ob, pchan->name);
return (BKE_object_defgroup_active_index_get(ob) - 1);
}
else {
Member

else after return is unnecessary

else after return is unnecessary
SietseB marked this conversation as resolved
@ -0,0 +108,4 @@
* Returns false when the normalization failed due to too many locked vertex groups. In that case a
* second pass can be done with the active vertex group unlocked.
*/
static bool normalize_vertex_weights_try(const MDeformVert &dvert,
Member

I'd expect the same functionality to be needed for mesh weight painting. Did you look into sharing code with that?

I'd expect the same functionality to be needed for mesh weight painting. Did you look into sharing code with that?
Author
Member

I did indeed, when I wrote the normalization for GPv2. The short answer is: they differ a bit, therefore the code can't be shared just out of the box.
The longer answer is: the starting point of painting weight on meshes differs from GP, since armatures are parented with automatic weights most of the time – so it's more about correcting weights. In GP parenting with automatic weights hardly works, so weight painting starts with vertex groups all at zero. That gives other edge cases.
Of course it must be possible to merge the two, but my proposal would be to create a task for that separate from this PR, with the Animation module heavily involved. I wouldn't feel comfortable making changes just for GP's sake in code that is heavily depended on by many studios and animators.

I did indeed, when I wrote the normalization for GPv2. The short answer is: they differ a bit, therefore the code can't be shared just out of the box. The longer answer is: the starting point of painting weight on meshes differs from GP, since armatures are parented _with automatic weights_ most of the time – so it's more about correcting weights. In GP parenting with automatic weights hardly works, so weight painting starts with vertex groups all at zero. That gives other edge cases. Of course it must be possible to merge the two, but my proposal would be to create a task for that separate from this PR, with the Animation module heavily involved. I wouldn't feel comfortable making changes just for GP's sake in code that is heavily depended on by many studios and animators.
Member

Okay, fair enough!

Okay, fair enough!
SietseB marked this conversation as resolved
@ -0,0 +111,4 @@
static bool normalize_vertex_weights_try(const MDeformVert &dvert,
const int vertex_groups_num,
const int locked_active_vertex_group,
const Vector<bool> &vertex_group_is_locked,
Member

In general Span should be used instead of references to other containers: const Span<bool>

In general Span should be used instead of references to other containers: `const Span<bool>`
SietseB marked this conversation as resolved
@ -0,0 +252,4 @@
const Object *ob_eval = DEG_get_evaluated_object(vc.depsgraph, const_cast<Object *>(vc.obact));
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(vc.obact->data);
const Vector<ed::greasepencil::DrawingInfo> drawings =
ed::greasepencil::retrieve_visible_drawings(*vc.scene, grease_pencil);
Member

ed::greasepencil should be unnecessary here, since this code is also in the same namespace

`ed::greasepencil` should be unnecessary here, since this code is also in the same namespace
SietseB marked this conversation as resolved
@ -0,0 +315,4 @@
/* From the closest point found, get the vertex weight in the active vertex group. */
const float new_weight = math::clamp(
bke::varray_for_deform_verts(closest.drawing->strokes().deform_verts(),
Member

IMO it would look a bit nicer with the varray declared as a variable on a separate line

IMO it would look a bit nicer with the varray declared as a variable on a separate line
SietseB marked this conversation as resolved
@ -236,2 +244,4 @@
IndexMaskMemory &memory);
/** Create a vertex group in a GP object with a general name or the name of an active bone. */
int create_vertex_group_in_object(Object *ob);
Member

Use a reference if the object is expected not to be null

Use a reference if the object is expected not to be null
SietseB marked this conversation as resolved
@ -238,0 +252,4 @@
/** For a point in a stroke, normalize the weights of vertex groups deformed by bones so that the
* sum is 1.0f. */
void normalize_vertex_weights(const MDeformVert &dvert,
const int active_vertex_group,
Member

const is meaningless for arguments passed by value in the declaration, we typically don't include it for that reason.

`const` is meaningless for arguments passed by value in the declaration, we typically don't include it for that reason.
SietseB marked this conversation as resolved
@ -0,0 +91,4 @@
/* The number of points stored in the stroke point buffers. Per frame group.
* Note: we can't use Array or Vector here, because it doesn't support atomic types. */
std::vector<std::atomic<int>> points_in_stroke_num;
Member

Looks like this should use the Blender Vector and Map containers

Looks like this should use the Blender `Vector` and `Map` containers
Author
Member

Maybe I missed something, but I couldn't get Vector<std::atomic<int>> compiled. I concluded that Vector doesn't support atomic types. Hence the std::vector.

Maybe I missed something, but I couldn't get `Vector<std::atomic<int>>` compiled. I concluded that `Vector` doesn't support atomic types. Hence the `std::vector`.
Member

I think std::atomic<int> doesn't have a move constructor.
How about Array instead of Vector?

I think `std::atomic<int>` doesn't have a move constructor. How about `Array` instead of `Vector`?
Author
Member

I tried a second time, but no luck with Array either.

I tried a second time, but no luck with `Array` either.
Member

I left some comments that would probably remove the necessity for std::atomic and make this simpler.

I left some comments that would probably remove the necessity for `std::atomic` and make this simpler.
SietseB marked this conversation as resolved
Sietse Brouwer added 5 commits 2024-02-19 21:59:32 +01:00
Author
Member

Thanks for the fast replies!

@filedescriptor Maybe I should tell a bit about what the tools share in code:

  • Getting/creating the active vertex group in object and drawings.
  • Getting locked and bone-deformed vertex groups in object and drawings.
  • Translating brush settings to brush influence per stroke point.
  • Handling multi frame editing.
  • Putting the stroke points under the brush in a buffer (needed for three out of four tools). E.g. for the Average tool you need the weights of all the points under the brush before you can apply the effect. Hence the buffer.
  • Adding stroke points to a KDtree during a stroke operation (needed for two out of four tools).
  • Handling auto-normalize weights.

I would say that's at least 80% of all the code in grease_pencil_paint_weight.cc. It would feel really weird to duplicate that file four times and let 80% of the code intact in all of those. To name something: when there is a bug in any of the above, you would have to correct it in four places.
So could I gently ask to have a second look at that aspect of your review? ;-)

Thanks for the fast replies! @filedescriptor Maybe I should tell a bit about what the tools share in code: - Getting/creating the active vertex group in object and drawings. - Getting locked and bone-deformed vertex groups in object and drawings. - Translating brush settings to brush influence per stroke point. - Handling multi frame editing. - Putting the stroke points under the brush in a buffer (needed for three out of four tools). E.g. for the Average tool you need the weights of all the points under the brush before you can apply the effect. Hence the buffer. - Adding stroke points to a KDtree during a stroke operation (needed for two out of four tools). - Handling auto-normalize weights. I would say that's at least 80% of all the code in `grease_pencil_paint_weight.cc`. It would feel really weird to duplicate that file four times and let 80% of the code intact in all of those. To name something: when there is a bug in any of the above, you would have to correct it in four places. So could I gently ask to have a second look at that aspect of your review? ;-)
Author
Member

Btw, I did a rewrite of retrieve_editable_drawings_grouped_per_frame() in grease_pencil_utils.cc, because the previous version could return duplicate drawings.

Btw, I did a rewrite of `retrieve_editable_drawings_grouped_per_frame()` in `grease_pencil_utils.cc`, because the previous version could return duplicate drawings.
Member

@SietseB I'm not suggesting to duplicate everything four times (although that might be a good way to start the refactor). I'm just saying that all of this code shouldn't be in a single GreasePencilStrokeOperation. All of the shared code you mentioned could be implemented in helper functions outside of the GreasePencilStrokeOperations.
Maybe declaring those in grease_pencil_intern.hh and putting the implementation in their own file like grease_pencil_weight_utils.cc would be a good option.

This would mean that:

  1. The implementation for each tool is somwhat short.
  2. It's clear what the tool uses from the helper functions used in the implementation (e.g. a KDTree).
  3. The helper functions live in a single place (thus fixing bugs shouldn't be an issue).

Let me know if you have concerns with this.

@SietseB I'm not suggesting to duplicate everything four times (although that might be a good way to start the refactor). I'm just saying that all of this code shouldn't be in a single `GreasePencilStrokeOperation`. All of the shared code you mentioned could be implemented in helper functions outside of the `GreasePencilStrokeOperation`s. Maybe declaring those in `grease_pencil_intern.hh` and putting the implementation in their own file like `grease_pencil_weight_utils.cc` would be a good option. This would mean that: 1) The implementation for each tool is somwhat short. 2) It's clear what the tool uses from the helper functions used in the implementation (e.g. a KDTree). 3) The helper functions live in a single place (thus fixing bugs shouldn't be an issue). Let me know if you have concerns with this.
Sietse Brouwer added 1 commit 2024-02-21 00:03:32 +01:00
Author
Member

I've reworked the whole WeightPaintOperation shebang. I hope it's progress, I'm not entirely sure about that. The code base grew from 600 to 1000 lines. But when you want to read the tools at a higher level, that's possible now, I guess.

I've reworked the whole `WeightPaintOperation` shebang. I hope it's progress, I'm not entirely sure about that. The code base grew from 600 to 1000 lines. But when you want to read the tools at a higher level, that's possible now, I guess.
Sietse Brouwer added 2 commits 2024-02-23 16:17:11 +01:00
Member

@SietseB Thanks, will continue reviewing next week :) IMHO 400 lines more are worth the readability ;)

@SietseB Thanks, will continue reviewing next week :) IMHO 400 lines more are worth the readability ;)
Iliya Katushenock reviewed 2024-02-24 12:44:29 +01:00
@ -0,0 +106,4 @@
* Returns false when the normalization failed due to too many locked vertex groups. In that case a
* second pass can be done with the active vertex group unlocked.
*/
static bool normalize_vertex_weights_try(const MDeformVert &dvert,

const MDeformVert &dvert -> MDeformVert &dvert
You actually change this object inside a function, no reason to mark this as const.

`const MDeformVert &dvert` -> `MDeformVert &dvert` You actually change this object inside a function, no reason to mark this as const.
SietseB marked this conversation as resolved
@ -0,0 +118,4 @@
}
/* Get the sum of weights of bone-deformed vertex groups. */
float sum_weights_total = 0.0f, sum_weights_locked = 0.0f, sum_weights_unlocked = 0.0f;

Don't define multiple variables at single line.

Don't define multiple variables at single line.
SietseB marked this conversation as resolved
@ -0,0 +120,4 @@
/* Get the sum of weights of bone-deformed vertex groups. */
float sum_weights_total = 0.0f, sum_weights_locked = 0.0f, sum_weights_unlocked = 0.0f;
int locked_num = 0, unlocked_num = 0;
for (int i = 0; i < dvert.totweight; i++) {

for (const int i : IndexRange(dvert.totweight))

`for (const int i : IndexRange(dvert.totweight))`
SietseB marked this conversation as resolved
@ -0,0 +150,4 @@
/* Any unlocked vertex group to normalize? */
if (unlocked_num == 0) {
/* We don't need a second pass when there is only one locked group (the active group). */
return (locked_num == 1);

return (locked_num == 1); -> return locked_num == 1;

`return (locked_num == 1);` -> `return locked_num == 1;`
SietseB marked this conversation as resolved
@ -0,0 +206,4 @@
void normalize_vertex_weights(const MDeformVert &dvert,
const int active_vertex_group,
const Span<bool> &vertex_group_is_locked,

const Span<bool> &vertex_group_is_locked -> const Span<bool> vertex_group_is_locked

`const Span<bool> &vertex_group_is_locked` -> `const Span<bool> vertex_group_is_locked`
SietseB marked this conversation as resolved
@ -0,0 +217,4 @@
vertex_group_is_locked,
vertex_group_is_bone_deformed);
if (!success) {
if (success) {
  return;
}
```Cpp if (success) { return; } ```
SietseB marked this conversation as resolved
Falk David requested changes 2024-02-26 15:22:11 +01:00
Dismissed
Falk David left a comment
Member

Really nice :) Thank you for splitting the tools into their own file. Was much easier for me to go through and review. I left some cleanup comments and some comments about parallel_for grain sizes. I also think that you can get rid of the uses of std::mutex (and probably the std::atomic<int> for the points_in_stroke_num).

Really nice :) Thank you for splitting the tools into their own file. Was much easier for me to go through and review. I left some cleanup comments and some comments about `parallel_for` grain sizes. I also think that you can get rid of the uses of `std::mutex` (and probably the `std::atomic<int>` for the `points_in_stroke_num`).
@ -0,0 +38,4 @@
/* Look for an active bone in armature to name the vertex group after. */
Object *ob_armature = BKE_modifiers_is_deformed_by_armature(&ob);
if (ob_armature != nullptr) {
Bone *actbone = ((bArmature *)ob_armature->data)->act_bone;
Member

Use C++ casts, so reinterpret_cast here.

Use C++ casts, so `reinterpret_cast` here.
SietseB marked this conversation as resolved
@ -0,0 +191,4 @@
const float weight_remainder = math::clamp(
(1.0f - sum_weights_locked) / unlocked_num, 0.0f, 1.0f);
for (int i = 0; i < dvert.totweight; i++) {
Member

Same as mentioned by @mod_moder, for (const int i : IndexRange(dvert.totweight))

Same as mentioned by @mod_moder, `for (const int i : IndexRange(dvert.totweight))`
SietseB marked this conversation as resolved
@ -0,0 +69,4 @@
/* Collect all stroke points under the brush in a buffer. */
threading::parallel_for(
drawing_weights.index_range(), 1, [&](const IndexRange drawing_range) {
Member

The grain_size should somewhat reflect how much work is being done in the loop in each iteration. It's a bit more complicated, but effectively:

  • The entire index range is split into grain_sized chunks
  • A thread is spawned for each chunk and executes it

But spawning threads is somewhat expensive! It can add up quickly if we do it too much.
So if each iteration is doing little work, a high (4096) grain size is best, because we will spawn less threads and they will run through the loop very quickly.
If each iteration is doing a ton of work, then we can think about lower grain sizes and trying to spawn more threads.

The `grain_size` should somewhat reflect how much work is being done in the loop in each iteration. It's a bit more complicated, but effectively: * The entire index range is split into `grain_sized` chunks * A thread is spawned for each chunk and executes it But spawning threads is somewhat expensive! It can add up quickly if we do it too much. So if each iteration is doing little work, a high (`4096`) grain size is best, because we will spawn less threads and they will run through the loop very quickly. If each iteration is doing a ton of work, then we can think about lower grain sizes and trying to spawn more threads.
Author
Member

The comment was probably a bit confusing, I changed it. And I changed the for loop into threading::parallel_for_each(drawing_weights, [&](DrawingWeightData &drawing_weight) {, reflecting the situation better.
This loop is iterating over the layers at a certain key frame. So a grain size of 1 seems appropriate to me?
Within the loop the stroke point are handled in serial: for (const int point_index : drawing_weight.point_positions.index_range()) {.

The comment was probably a bit confusing, I changed it. And I changed the for loop into `threading::parallel_for_each(drawing_weights, [&](DrawingWeightData &drawing_weight) {`, reflecting the situation better. This loop is iterating over the layers at a certain key frame. So a grain size of 1 seems appropriate to me? Within the loop the stroke point are handled in serial: `for (const int point_index : drawing_weight.point_positions.index_range()) {`.
Member

@SietseB Inside we're not doing enough work to justify a grain size of 1. This should be more like 1024 I think.

@SietseB Inside we're not doing enough work to justify a grain size of 1. This should be more like 1024 I think.
Author
Member

For the case we have more than 1024 layers? ;-)
It's just like the threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) { we do in many other operators, only now at a certain key frame.
Each thread handles all the stroke points at that layer, so there enough work to do, I would say.

For the case we have more than 1024 layers? ;-) It's just like the `threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) {` we do in many other operators, only now at a certain key frame. Each thread handles all the stroke points at that layer, so there enough work to do, I would say.
Member

Ah I see, so the outer loop is only for multi-frame editing essentially and the inner loop for the layers. I suppose it's ok to use parallel_for_each then. But all uses of parallel_for with a grain size of 1 should be replaced by that then.

Ah I see, so the outer loop is only for multi-frame editing essentially and the inner loop for the layers. I suppose it's ok to use `parallel_for_each` then. But all uses of `parallel_for` with a grain size of `1` should be replaced by that then.
Author
Member

For the outer loop that's not an option right now, unfortunately. We need the frame_group index inside the loop; with parallel_for_each we don't have that index.
I'll improve the comment above the outer loop, explaining a bit better what the setup is.

For the outer loop that's not an option right now, unfortunately. We need the `frame_group` index inside the loop; with `parallel_for_each` we don't have that index. I'll improve the comment above the outer loop, explaining a bit better what the setup is.
Member

In that case you should be able to use an index range with parallel_for_each and loop over each index.

In that case you should be able to use an index range with `parallel_for_each` and loop over each index.
SietseB marked this conversation as resolved
@ -0,0 +29,4 @@
* point weights, based on the distance of the neighbour point to A. So points closer to A
* contribute more to the average than points farther away from A. */
float distance_sum = 0.0f;
for (int i = 0; i < point_num; i++) {
Member

for (const int i : IndexRange(point_num))

`for (const int i : IndexRange(point_num))`
SietseB marked this conversation as resolved
@ -0,0 +36,4 @@
return;
}
float blur_weight_sum = 0.0f;
for (int i = 0; i < point_num; i++) {
Member

Same as above

Same as above
SietseB marked this conversation as resolved
@ -0,0 +94,4 @@
std::atomic<bool> balance_kdtree = false;
/* Collect all stroke points under the brush in a buffer. */
threading::parallel_for(
Member

There is not really a point in doing this in a parallel_for, when you can't do it in parallel and need a mutex.
Especially when the loop doesn't do much work. This is probably faster with a regular-old for loop.
I would also get rid of the mutex argument to add_point_to_stroke_buffer.

There is not really a point in doing this in a `parallel_for`, when you can't do it in parallel and need a mutex. Especially when the loop doesn't do much work. This is probably faster with a regular-old `for` loop. I would also get rid of the `mutex` argument to `add_point_to_stroke_buffer`.
Author
Member

Ha, when the mutex was used all the time, I would fully agree, of course. But adding stroke points to the brush buffer runs perfectly parallel, no mutex used there. The only rare moment the mutex is used, is when a point is added to the KDtree and the buffer size is exceeded. So 1 in 1024 times at max, with the current settings.
IMO, it would really be a missed opportunity to remove the parallel execution here, because that would mean that in the majority of use cases (when no multi frame editing is used) weight painting is running on a single thread, just like in GPv2.

Ha, when the mutex was used all the time, I would fully agree, of course. But adding stroke points to the brush buffer runs perfectly parallel, no mutex used there. The only rare moment the mutex is used, is when a point is added to the KDtree and the buffer size is exceeded. So 1 in 1024 times at max, with the current settings. IMO, it would really be a missed opportunity to remove the parallel execution here, because that would mean that in the majority of use cases (when no multi frame editing is used) weight painting is running on a single thread, just like in GPv2.
Member

Why can add_point_under_brush_to_brush_buffer run in parallel?

Why can `add_point_under_brush_to_brush_buffer` run in parallel?
Author
Member

The layers (threads) are keeping their own record of points under the brush, so just for the points on that layer. That way the threads can operate independently, no shared memory, no need for atomic counters.

For the KDtree buffer that setup is not possible of course. We want to find nearest points like it is one big layer, so the KDtree has to be shared by all the layers (threads). Hence the atomic counter (how many points are added to the tree) and the mutex, in case the maximum size of the KDtree is reached.

The layers (threads) are keeping their own record of points under the brush, so just for the points on that layer. That way the threads can operate independently, no shared memory, no need for atomic counters. For the KDtree buffer that setup is not possible of course. We want to find nearest points like it is one big layer, so the KDtree _has_ to be shared by all the layers (threads). Hence the atomic counter (how many points are added to the tree) and the mutex, in case the maximum size of the KDtree is reached.
SietseB marked this conversation as resolved
@ -0,0 +76,4 @@
const float distance_normalizer = (min_distance == max_distance) ?
1.0f :
(0.95f / (max_distance - min_distance));
for (int i = 0; i < point_num; i++) {
Member

`for (const int i : IndexRange(point_num))

`for (const int i : IndexRange(point_num))
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-02-26 19:55:48 +01:00
Falk David requested review from Hans Goudey 2024-02-29 11:29:21 +01:00
Falk David requested changes 2024-03-01 12:51:32 +01:00
Dismissed
Falk David left a comment
Member

Went over this together with @LukasTonne. Apart from the way the KDTree is built, there is not much to say. A few cleanups here and there. We looked at a way to make the code for creating the KDTree better and easier to read. Here is our suggestion:

  • Make the creation of the KDTree single threaded. KD-Trees are really not meant to be created or resized, it's best to insert all the points at once and then balance the tree.
  • The easiest way to do that is probably to have a Array<bool> in DrawingWeightData that stores true for the points that are not under the brush. The array can be pre-allocated with the size of the points in the drawing. Then after the parallel_for_each over the drawing_weights, create, populate and balance the tree all at once.
  • Then there is no need for the mutex, the vector of atomic ints and the recreating of the kdtree.
Went over this together with @LukasTonne. Apart from the way the `KDTree` is built, there is not much to say. A few cleanups here and there. We looked at a way to make the code for creating the `KDTree` better and easier to read. Here is our suggestion: * Make the creation of the `KDTree` single threaded. KD-Trees are really not meant to be created or resized, it's best to insert all the points at once and then balance the tree. * The easiest way to do that is probably to have a `Array<bool>` in `DrawingWeightData` that stores `true` for the points that are not under the brush. The array can be pre-allocated with the size of the points in the drawing. Then after the `parallel_for_each` over the `drawing_weights`, create, populate and balance the tree all at once. * Then there is no need for the mutex, the vector of atomic ints and the recreating of the kdtree.
@ -0,0 +33,4 @@
namespace blender::ed::greasepencil {
int create_vertex_group_in_object(Object &ob)
Member

This function does multiple things that are not obvious when looking at its name. It should be split.
This function can take a name and just do:

if (name.is_empty()) {
   /* Create a vertex group with a general name. */
   BKE_object_defgroup_add(&ob);
   return 0;
}
const int def_nr = BKE_object_defgroup_name_index(&ob, name);
if (def_nr == -1) {
   BKE_object_defgroup_add_name(&ob, name);
   return BKE_object_defgroup_active_index_get(&ob) - 1;
}
return def_nr;

Then finding the active bone channel name can be it's own function, e.g. (find_active_bone_channel_name).

This function does multiple things that are not obvious when looking at its name. It should be split. This function can take a `name` and just do: ``` if (name.is_empty()) { /* Create a vertex group with a general name. */ BKE_object_defgroup_add(&ob); return 0; } const int def_nr = BKE_object_defgroup_name_index(&ob, name); if (def_nr == -1) { BKE_object_defgroup_add_name(&ob, name); return BKE_object_defgroup_active_index_get(&ob) - 1; } return def_nr; ``` Then finding the active bone channel name can be it's own function, e.g. (`find_active_bone_channel_name`).
SietseB marked this conversation as resolved
@ -0,0 +58,4 @@
return 0;
}
Set<std::string> get_bone_deformed_vertex_groups(Object &object)
Member

get_bone_deformed_vertex_group_names

`get_bone_deformed_vertex_group_names`
SietseB marked this conversation as resolved
@ -0,0 +70,4 @@
/* Lambda function for finding deforming bones with a name matching a vertex group. */
Set<std::string> bone_deformed_vgroups;
const auto find_pose_channels = [&](ModifierData *md) {
for (; md; md = md->next) {
Member

Would be a bit more readable if this loop was outside the lambda.

Would be a bit more readable if this loop was outside the lambda.
SietseB marked this conversation as resolved
@ -0,0 +94,4 @@
};
/* Inspect all armature modifiers in the object. */
VirtualModifierData virtual_modifier_data;
Member

This should be

VirtualModifierData virtual_modifier_data;
ModifierData *md = BKE_modifiers_get_virtual_modifierlist(&object, &virtual_modifier_data);
for (; md; md = md->next) {
   find_pose_channels(md);
}

BKE_modifiers_get_virtual_modifierlist prepends to the objects modifier list.

This should be ``` VirtualModifierData virtual_modifier_data; ModifierData *md = BKE_modifiers_get_virtual_modifierlist(&object, &virtual_modifier_data); for (; md; md = md->next) { find_pose_channels(md); } ``` `BKE_modifiers_get_virtual_modifierlist` prepends to the objects modifier list.
SietseB marked this conversation as resolved
@ -0,0 +106,4 @@
* Returns false when the normalization failed due to too many locked vertex groups. In that case a
* second pass can be done with the active vertex group unlocked.
*/
static bool normalize_vertex_weights_try(MDeformVert &dvert,
Member

Note: maybe instead of locked_active_vertex_groupand vertex_group_is_locked you could pass a lambda that checkes both the span and the active index.

Note: maybe instead of `locked_active_vertex_group`and `vertex_group_is_locked` you could pass a lambda that checkes both the span and the active index.
SietseB marked this conversation as resolved
@ -0,0 +238,4 @@
ed::curves::FindClosestData elem = {};
};
static int sample_weight(bContext *C, wmOperator * /*op*/, const wmEvent *event)
Member

weight_sample_invoke

`weight_sample_invoke`
SietseB marked this conversation as resolved
@ -238,0 +253,4 @@
* sum is 1.0f. */
void normalize_vertex_weights(MDeformVert &dvert,
int active_vertex_group,
const Span<bool> vertex_group_is_locked,
Member

const not needed in the declaration for arguments that are passed by value.

`const` not needed in the declaration for arguments that are passed by value.
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-03-03 10:32:01 +01:00
Author
Member

Thanks again for the (joint) review!
All right, the KDTree. If I'm understanding correctly, your proposal is to build a KDTree at every on_stroke_extended event. So, at a rate of let's say 20 fps, deleting the old tree and creating + populating + balancing a new one. Wouldn't that be a waste of resources? I'm sure adding a handful of new points to an existing tree every on_stroke_extended event is more performant.
And I am not sure which problem we solve by that exactly? The recreation of the tree in the very rare case of a buffer overflow? I've increased the initial tree size to 2048, so the code that handles a buffer overflow, will likely never run in practice. It's a fail safe, but nothing more than that.

So, I haven't changed that part of the code yet. If you want me to make the code less performant, you'll have to increase the pressure ;-)

Thanks again for the (joint) review! All right, the KDTree. If I'm understanding correctly, your proposal is to build a KDTree at every `on_stroke_extended` event. So, at a rate of let's say 20 fps, deleting the old tree and creating + populating + balancing a new one. Wouldn't that be a waste of resources? I'm sure adding a handful of new points to an existing tree every `on_stroke_extended` event is more performant. And I am not sure which problem we solve by that exactly? The recreation of the tree in the very rare case of a buffer overflow? I've increased the initial tree size to 2048, so the code that handles a buffer overflow, will likely never run in practice. It's a fail safe, but nothing more than that. So, I haven't changed that part of the code yet. If you want me to make the code less performant, you'll have to increase the pressure ;-)
Member

@SietseB My concern is not performance right now. My concern is that there is too much complexity in that part of the code without any good reason.
When it comes to performance, rule number 1 is: Don't listen to your intuition, don't take anyones word for it, measure!
So, I think that there is no justification for the complexity added. For now, simplicity and readability are more important. If this turns out to be too slow/not performant enough, then complexity can be added given that there are numbers to back it up.
I hope that you can agree to that 😄

@SietseB My concern is not performance right now. My concern is that there is too much complexity in that part of the code without any good reason. When it comes to performance, rule number 1 is: Don't listen to your intuition, don't take anyones word for it, _measure_! So, I think that there is no justification for the complexity added. For now, simplicity and readability are more important. _If_ this turns out to be too slow/not performant enough, then complexity can be added _given that there are numbers to back it up_. I hope that you can agree to that 😄
Sietse Brouwer added 2 commits 2024-03-07 23:11:04 +01:00
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
2f1f297fba
Code review: Build KDTree on every #on_stroke_extended event
Falk David approved these changes 2024-03-11 12:20:30 +01:00
Falk David left a comment
Member

Thanks for updating the PR! This looks good to me now. I'll wait for @HooglyBoogly's feedback.

Thanks for updating the PR! This looks good to me now. I'll wait for @HooglyBoogly's feedback.
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2024-03-11 19:25:08 +01:00
Hans Goudey left a comment
Member

Accepting with minor requests for cleanup

Accepting with minor requests for cleanup
@ -145,3 +153,3 @@
_row, sub = row_for_mirror()
sub.prop(context.object.pose, "use_mirror_x", text="X", toggle=True)
elif mode_string in {'EDIT_MESH', 'PAINT_WEIGHT', 'SCULPT', 'PAINT_VERTEX', 'PAINT_TEXTURE'}:
elif ((not is_greasepencil) and
Member

Trailing whitespace

Trailing whitespace
SietseB marked this conversation as resolved
@ -716,6 +716,12 @@ bool BKE_object_defgroup_check_lock_relative_multi(int defbase_tot,
bool BKE_object_defgroup_active_is_locked(const Object *ob)
{
if (ob->type == OB_GREASE_PENCIL) {
Member

Seems like this should use a switch rather than an early return

Seems like this should use a switch rather than an early return
SietseB marked this conversation as resolved
@ -555,0 +649,4 @@
}
const bke::CurvesGeometry &curves = drawing.strokes();
const IndexRange points_range = drawing.strokes().points_range();
Member

drawing.strokes().points_range() -> curves.points_range()

`drawing.strokes().points_range()` -> `curves.points_range()`
SietseB marked this conversation as resolved
@ -245,2 +253,4 @@
IndexMaskMemory &memory);
/** Returns a set of vertex group names that are deformed by a bone in an armature. */
Set<std::string> get_bone_deformed_vertex_group_names(Object &object);
Member

const Object &

`const Object &`
SietseB marked this conversation as resolved
@ -296,0 +301,4 @@
static bool weight_stroke_test_start(bContext *C, wmOperator *op, const float mouse[2])
{
Member

Unnecessary empty line

Unnecessary empty line
SietseB marked this conversation as resolved
@ -296,0 +309,4 @@
GreasePencilStrokeOperation *operation = nullptr;
Paint *paint = BKE_paint_get_active_from_context(C);
Brush *brush = BKE_paint_brush(paint);
BrushStrokeMode brush_mode = static_cast<BrushStrokeMode>(RNA_enum_get(op->ptr, "mode"));
Member

const BrushStrokeMode brush_mode = BrushStrokeMode(RNA_enum_get(op->ptr, "mode"));

`const BrushStrokeMode brush_mode = BrushStrokeMode(RNA_enum_get(op->ptr, "mode"));`
SietseB marked this conversation as resolved
@ -296,0 +311,4 @@
Brush *brush = BKE_paint_brush(paint);
BrushStrokeMode brush_mode = static_cast<BrushStrokeMode>(RNA_enum_get(op->ptr, "mode"));
switch (brush->weightpaint_tool) {
Member

switch (eBrushWeightPaintTool(brush->weightpaint_tool)) and remove the default for compiler errors when a new operation is added

`switch (eBrushWeightPaintTool(brush->weightpaint_tool))` and remove the default for compiler errors when a new operation is added
SietseB marked this conversation as resolved
@ -296,0 +328,4 @@
break;
}
if (operation) {
Member

Flip the case and return early in the failure case

Flip the case and return early in the failure case
SietseB marked this conversation as resolved
@ -0,0 +7,4 @@
namespace blender::ed::sculpt_paint::greasepencil {
class AverageWeightPaintOperation : public WeightPaintOperation {
private:
Member

private is already the default for classes

`private` is already the default for classes
SietseB marked this conversation as resolved
@ -0,0 +209,4 @@
bke::crazyspace::GeometryDeformation deformation =
bke::crazyspace::get_evaluated_grease_pencil_drawing_deformation(
ob_eval, *this->object, drawing_info.layer_index, drawing_info.frame_number);
drawing_weight_data.point_positions = Array<float2>(deformation.positions.size());
Member

drawing_weight_data.point_positions.reinitialize(deformation.positions.size())

`drawing_weight_data.point_positions.reinitialize(deformation.positions.size())`
SietseB marked this conversation as resolved
@ -0,0 +280,4 @@
}
/* Create KDTree for all stroke points touched by the brush during a weight paint operation. */
PointsTouchedByBrush create_kdtree_of_points_touched_by_the_brush(
Member

How about create_affected_points_kdtree? Much shorter and simpler?

How about `create_affected_points_kdtree`? Much shorter and simpler?
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-03-11 23:10:40 +01:00

I think all virtual methods need to be marked as override, but not sure if this is already made for code in the main..

I think all virtual methods need to be marked as override, but not sure if this is already made for code in the main..
Falk David reviewed 2024-03-12 10:25:58 +01:00
@ -0,0 +26,4 @@
}
public:
void on_stroke_begin(const bContext &C, const InputSample &start_sample)
Member

Right, like @mod_moder said, these should have the override keyword. It's not strictly necessary in C++ (the compiler will do the right thing), but it's a nice indication that these are implementing a virtual method of a base class.

Right, like @mod_moder said, these should have the `override` keyword. It's not strictly necessary in C++ (the compiler will do the right thing), but it's a nice indication that these are implementing a virtual method of a base class.
https://projects.blender.org/blender/blender/commit/0673cd873dbec2cd2e565ae1a6e8a7b9f4242761
Member

final doesn't really make sense. It just means that a class that is derived from e.g. AverageWeightPaintOperation can't override on_stroke_begin. But such a class wouldn't make sense in the first place.
override means that the program won't compile if the base class doesn't have a virtual function on_stroke_begin which is what we want.

`final` doesn't really make sense. It just means that a class that is derived from e.g. `AverageWeightPaintOperation` can't override `on_stroke_begin`. But such a class wouldn't make sense in the first place. `override` means that the program won't compile if the base class doesn't have a virtual function `on_stroke_begin` which is what we want.

Both final and override will ensure that there is base function to override and check if signature is the same. But yeah, not sure in the reason to choose final, i tried to point on the fact that blender had incorrect signature of virtual functions while a long time, so this rule should be more strict for us\

Both `final` and `override` will ensure that there is base function to override and check if signature is the same. But yeah, not sure in the reason to choose `final`, i tried to point on the fact that blender had incorrect signature of virtual functions while a long time, so this rule should be more strict for us\
Member

No this is the point final will not check the base class. See https://en.cppreference.com/w/cpp/language/final. It will only give an error if there is a derived class that wants to override the method!

No this is the point `final` will **not** check the base class. See https://en.cppreference.com/w/cpp/language/final. It will only give an error if there is a derived class that wants to override the method!

Ah, so i just get final specifier ensures that the function is virtual in wrong way/

Ah, so i just get `final specifier ensures that the function is virtual` in wrong way/
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-03-12 11:44:04 +01:00
Member

@SietseB Added a comment about the final. I don't think it makes sense.

@SietseB Added a comment about the `final`. I don't think it makes sense.
Sietse Brouwer added 2 commits 2024-03-12 12:24:27 +01:00
Author
Member

And override it is ☺️

And `override` it is :relaxed:
Sietse Brouwer added 1 commit 2024-03-14 20:09:23 +01:00
Sietse Brouwer added 1 commit 2024-03-15 13:14:23 +01:00
Sietse Brouwer added 1 commit 2024-04-02 19:19:00 +02:00
Sietse Brouwer added 2 commits 2024-04-11 22:56:07 +02:00
Sietse Brouwer added 1 commit 2024-04-16 23:18:47 +02:00
Sietse Brouwer added 1 commit 2024-04-22 23:34:01 +02:00
Falk David added 2 commits 2024-04-23 16:27:05 +02:00
Falk David added 1 commit 2024-04-23 18:26:33 +02:00
Falk David added 1 commit 2024-04-23 18:43:18 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
d8a5b64722
Fixes for tools
Member

@blender-bot build

@blender-bot build
Sietse Brouwer added 2 commits 2024-04-25 00:08:07 +02:00
Falk David added 2 commits 2024-04-25 10:46:56 +02:00
Falk David approved these changes 2024-04-25 10:52:12 +02:00
Falk David left a comment
Member

Some cleanup comments. I think we can merge this soon. Tested it on my end and seems to work correctly now (using the brushes from grease pencil, not meshes).

Some cleanup comments. I think we can merge this soon. Tested it on my end and seems to work correctly now (using the brushes from grease pencil, not meshes).
@ -52,6 +52,7 @@
#include "WM_toolsystem.hh"
#include "WM_types.hh"
#include "ED_grease_pencil.hh"
Member

This include shouldn't be needed.

This include shouldn't be needed.
SietseB marked this conversation as resolved
@ -1031,6 +1031,8 @@ static const EnumPropertyItem *rna_Brush_direction_itemf(bContext *C,
default:
return rna_enum_dummy_DEFAULT_items;
}
case PaintMode::Weight:
Member

Looks like these changes should be removed now?

Looks like these changes should be removed now?
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-04-25 11:40:54 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
bd8de313ab
Code review: A few cleanups
Member

@blender-bot build

@blender-bot build
Sietse Brouwer added 1 commit 2024-04-25 15:16:42 +02:00
Falk David merged commit 5220caeabb into main 2024-04-25 15:21:26 +02:00
Sietse Brouwer deleted branch gpv3-weight-paint-tools 2024-04-25 21:23:22 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#118347
No description provided.