Animation: Weight Paint select more/less for faces #105607

Christoph Lendenfeld · 2023-03-09T16:39:16+01:00

2023-03-09 16:39:16 +01:00

This adds the select more/less operators to the weight paint mode face selection.

Just like in edit mode, press CTRL+Numpad Plus/Minus to use them.
They have also been added to the Select menu.

This adds the select more/less operators to the weight paint mode face selection. Just like in edit mode, press `CTRL`+`Numpad Plus/Minus` to use them. They have also been added to the `Select` menu.

❤️ 1

Christoph Lendenfeld added 3 commits 2023-03-09 16:39:28 +01:00

73ea99d29d implement select more operator

c91d16b4cd implement select less operator

buildbot/vexp-code-patch-coordinator Build done. Details

ad7e632371 add threading

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-03-09 16:57:27 +01:00

Christoph Lendenfeld requested review from Marion Stalke 2023-03-09 16:57:45 +01:00

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-03-09 16:57:53 +01:00

Hans Goudey requested review from Hans Goudey 2023-03-09 16:58:48 +01:00

Hans Goudey requested changes 2023-03-09 17:10:19 +01:00

Hans Goudey left a comment

Relying on the existing selection flushing between domains seems like a nice way to do this.

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +401,4 @@

				  paintface_flush_flags(C, ob, true, false);

				}

				void paintface_select_less(bContext *C, Object *ob, const bool face_step)

Hans Goudey commented

2023-03-09 17:01:38 +01:00

It might be clearer for these functions to have slightly lower level arguments, like (Mesh &mesh, const bool face_step) in this case. That separates the abstraction levels more clearly, and means this function could be used in other situations where the context is different or the update tags aren't the same.

It might be clearer for these functions to have slightly lower level arguments, like `(Mesh &mesh, const bool face_step)` in this case. That separates the abstraction levels more clearly, and means this function could be used in other situations where the context is different or the update tags aren't the same.

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +419,4 @@

				  const Span<MLoop> loops = mesh->loops();

				  const Span<MEdge> edges = mesh->edges();

				  std::vector<bool> verts_of_unselected_faces(mesh->totvert, false);

Hans Goudey commented

2023-03-09 16:59:53 +01:00

I assume you're using std::vector<bool> because it works as a bitmap internally? blender::BitVector should be a better choice here. See the reasoning at the top of that header.

I assume you're using `std::vector<bool>` because it works as a bitmap internally? `blender::BitVector` should be a better choice here. See the reasoning at the top of that header.

ChrisLend marked this conversation as resolved

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +422,4 @@

				  std::vector<bool> verts_of_unselected_faces(mesh->totvert, false);

				  /* Find all vertices of unselected faces to help find neighboring faces after. */

				  for (const int i : select_poly.span.index_range()) {

Hans Goudey commented

2023-03-09 17:04:44 +01:00

Small thing, but maybe it's a bit clearer to write polys.index_range() than select_poly.span.index_range(). That's just semantically closer to the goal of iterating over all polys.

Small thing, but maybe it's a bit clearer to write `polys.index_range()` than `select_poly.span.index_range()`. That's just semantically closer to the goal of iterating over all polys.

ChrisLend marked this conversation as resolved

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +439,4 @@

				      }

				      const MPoly &poly = polys[i];

				      for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {

Hans Goudey commented

2023-03-09 17:09:58 +01:00

What do you think about splitting this part into a separate function-- something like bool poly_has_unselected_neighbor(Span<MLoop> poly_loops, bool face_step)?

I think that would make the logic here a bit simpler, and avoid the need for breaking inside the loop.

Something similar would be helpful above too.

What do you think about splitting this part into a separate function-- something like ` bool poly_has_unselected_neighbor(Span<MLoop> poly_loops, bool face_step)`? I think that would make the logic here a bit simpler, and avoid the need for breaking inside the loop. Something similar would be helpful above too.

Christoph Lendenfeld commented

2023-03-10 14:20:13 +01:00

@blender-bot package

Blender Bot commented

2023-03-10 14:20:16 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105607) when ready.

Christoph Lendenfeld added 2 commits 2023-03-16 10:58:41 +01:00

c98c34d8c4 Merge branch 'main' into weight_paint_grow_sel_face

buildbot/vexp-code-patch-coordinator Build done. Details

158d68471e implement hans' feedback

Christoph Lendenfeld reviewed 2023-03-16 11:00:14 +01:00

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +401,4 @@

				  paintface_flush_flags(C, ob, true, false);

				}

				void paintface_select_less(bContext *C, Object *ob, const bool face_step)

Christoph Lendenfeld commented

2023-03-16 10:53:36 +01:00

This might be my limited understanding of C++ but I can't get this to work.
Could it be that because the header file is ED_mesh.h meaning it's pure C so it doesn't understand references?

Edit since it seems to not have linked to your comment.
It was about passing in Mesh &mesh instead of bContext and Object

This might be my limited understanding of C++ but I can't get this to work. Could it be that because the header file is `ED_mesh.h` meaning it's pure C so it doesn't understand references? Edit since it seems to not have linked to your comment. It was about passing in `Mesh &mesh` instead of bContext and Object

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +439,4 @@

				  const VArray<bool> hide_poly = attributes.lookup_or_default<bool>(

				      ".hide_poly", ATTR_DOMAIN_FACE, false);

				  const Span<MPoly> polys = mesh->polys();

Christoph Lendenfeld commented

2023-03-16 10:59:32 +01:00

I've split up this part. the argument list is a bit long though so I am a bit unsure if that is an improvement. Let me know what you think

Christoph Lendenfeld commented

2023-03-16 11:02:54 +01:00

@blender-bot package

Blender Bot commented

2023-03-16 11:02:56 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105607) when ready.

Christoph Lendenfeld requested review from Hans Goudey 2023-03-16 11:07:07 +01:00

Hans Goudey requested changes 2023-03-18 02:28:57 +01:00

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +401,4 @@

				  paintface_flush_flags(C, ob, true, false);

				}

				static bool poly_has_unselected_neighbour(const MPoly &poly,

Hans Goudey commented

2023-03-18 02:19:02 +01:00

Yeah right, a couple options-- keep a function with this signature in the public header, and a static function with the signature I suggested, or use the signature I suggested with a pointer instead of a reference (that's probably the better option IMO).

Mainly I think it's nice to avoid just using the object argument to retrieve the mesh, and it's nice to avoid the null check because this function really shouldn't be concerned with whether there is no mesh, that's the job of somewhere else.

Yeah right, a couple options-- keep a function with this signature in the public header, and a static function with the signature I suggested, or use the signature I suggested with a pointer instead of a reference (that's probably the better option IMO). Mainly I think it's nice to avoid just using the object argument to retrieve the mesh, and it's nice to avoid the null check because this function really shouldn't be concerned with whether there is no mesh, that's the job of somewhere else.

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +404,4 @@

				static bool poly_has_unselected_neighbour(const MPoly &poly,

				                                          blender::Span<MLoop> poly_loops,

				                                          blender::Span<MEdge> edges,

				                                          blender::BitVector<> &verts_of_unselected_faces,

Hans Goudey commented

2023-03-18 02:22:44 +01:00

Replace the BitVector reference with const BitSpan, that will give a better idea of ownership and the fact that this doesn't need to modify the data.

Replace the `BitVector` reference with `const BitSpan`, that will give a better idea of ownership and the fact that this doesn't need to modify the data.

ChrisLend marked this conversation as resolved

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +409,4 @@

				{

				  for (const MLoop &loop : poly_loops.slice(poly.loopstart, poly.totloop)) {

				    const MEdge &edge = edges[loop.e];

				    bool unselected_neighbor = false;

Hans Goudey commented

2023-03-18 02:26:49 +01:00

It's not a big deal, but the function could be a bit simpler without the unselected_neighbor variable. It's nice to assign a variable a meaningful value in the same expression you initialize it, and that's not really the case here. Given the function name, returning early reads as "poly has an unselected neighbor" anyway.

It's not a big deal, but the function could be a bit simpler without the `unselected_neighbor` variable. It's nice to assign a variable a meaningful value in the same expression you initialize it, and that's not really the case here. Given the function name, returning early reads as "poly has an unselected neighbor" anyway.

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +411,4 @@

				    const MEdge &edge = edges[loop.e];

				    bool unselected_neighbor = false;

				    if (face_step) {

				      unselected_neighbor = verts_of_unselected_faces[edge.v1].test() ||

Hans Goudey commented

2023-03-18 02:23:42 +01:00

What do you think about removing the .test() and using BitRef's implicit bool conversion?

What do you think about removing the `.test()` and using `BitRef`'s implicit bool conversion?

ChrisLend marked this conversation as resolved

source/blender/editors/mesh/editface.cc

						
				@ -353,0 +439,4 @@

				  const VArray<bool> hide_poly = attributes.lookup_or_default<bool>(

				      ".hide_poly", ATTR_DOMAIN_FACE, false);

				  const Span<MPoly> polys = mesh->polys();

Hans Goudey commented

2023-03-18 02:28:44 +01:00

poly_loops is generally the name for a span containing the loops of a single polygon. The poly argument could be removed by slicing the loops span before passing it to the function.

`poly_loops` is generally the name for a span containing the loops of a single polygon. The `poly` argument could be removed by slicing the `loops` span before passing it to the function.

ChrisLend marked this conversation as resolved

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +446,4 @@

				  BitVector<> verts_of_unselected_faces(mesh->totvert, false);

				  /* Find all vertices of unselected faces to help find neighboring faces after. */

				  for (const int i : select_poly.span.index_range()) {

Hans Goudey commented

2023-03-18 02:28:33 +01:00

Might as well change this to polys.index_range() instead of select_poly.span.index_range() for the same reason I mentioned earlier too-- it just says more clearly "we're iterating over all faces" rather than "we're iterating over the face selection".

I realize that's a bit nitpicky, just hoping you might agree and appreciate the more literal semantic argument :P

Might as well change this to `polys.index_range()` instead of `select_poly.span.index_range()` for the same reason I mentioned earlier too-- it just says more clearly "we're iterating over all faces" rather than "we're iterating over the face selection". I realize that's a bit nitpicky, just hoping you might agree and appreciate the more literal semantic argument :P

Christoph Lendenfeld added 1 commit 2023-03-23 10:54:31 +01:00

24aee56b7f implement hans' feedback

Christoph Lendenfeld reviewed 2023-03-23 10:55:11 +01:00

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +401,4 @@

				                                          blender::BitSpan verts_of_unselected_faces,

				                                          const bool face_step)

				{

				  for (const MLoop &loop : poly_loops) {

Christoph Lendenfeld commented

2023-03-23 10:55:00 +01:00

now takes a Mesh*
a potential clean up is to check if that can be done for other functions as well

now takes a `Mesh*` a potential clean up is to check if that can be done for other functions as well

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +409,4 @@

				{

				  for (const MLoop &loop : poly_loops.slice(poly.loopstart, poly.totloop)) {

				    const MEdge &edge = edges[loop.e];

				    bool unselected_neighbor = false;

Christoph Lendenfeld commented

2023-03-23 10:52:16 +01:00

I thought to make it explicit what it is that the bool logic tests for, but I agree it's in the function name anyway

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +411,4 @@

				    const MEdge &edge = edges[loop.e];

				    bool unselected_neighbor = false;

				    if (face_step) {

				      unselected_neighbor = verts_of_unselected_faces[edge.v1].test() ||

Christoph Lendenfeld commented

2023-03-23 10:48:53 +01:00

yes good idea, done that

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +446,4 @@

				  BitVector<> verts_of_unselected_faces(mesh->totvert, false);

				  /* Find all vertices of unselected faces to help find neighboring faces after. */

				  for (const int i : select_poly.span.index_range()) {

Christoph Lendenfeld commented

2023-03-23 10:47:26 +01:00

yep agreed, missed that one :)

Christoph Lendenfeld requested review from Hans Goudey 2023-03-23 10:55:53 +01:00

Hans Goudey requested changes 2023-03-23 15:20:57 +01:00

Hans Goudey left a comment

Almost there, thanks for the updates. Will need to be updated for #104424 though.

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +385,4 @@

				          selected_neighbor = select_vert.span[edge.v1] && select_vert.span[edge.v2];

				        }

				        if (selected_neighbor) {

				          select_poly.span[i] = true;

Hans Goudey commented

2023-03-23 15:19:29 +01:00

This is feeling a bit picky, sorry about that, but might as well extract this check for the poly instead of using a break like below (poly_has_unselected_neighbour) I think it makes sense for them to be consistent anyway.

This is feeling a bit picky, sorry about that, but might as well extract this check for the poly instead of using a `break` like below (`poly_has_unselected_neighbour`) I think it makes sense for them to be consistent anyway.

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +396,4 @@

				  select_vert.finish();

				}

				static bool poly_has_unselected_neighbour(blender::Span<MLoop> poly_loops,

Hans Goudey commented

2023-03-23 15:19:50 +01:00

Blender uses American English spelling, so neighbor instead of neighbor

Blender uses American English spelling, so `neighbor` instead of `neighbor`

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +440,4 @@

				    }

				    const MPoly &poly = polys[i];

				    for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {

				      verts_of_unselected_faces[loop.v].set(true);

Hans Goudey commented

2023-03-23 15:17:40 +01:00

MLoop has been replaced by two arrays in main. This loop can be simplified now:

for (const int vert : corner_verts.slice(poly.loopstart, poly.totloop)) {
  ...
}

`MLoop` has been replaced by two arrays in `main`. This loop can be simplified now: ``` for (const int vert : corner_verts.slice(poly.loopstart, poly.totloop)) { ... } ```

Christoph Lendenfeld referenced this pull request

2023-03-24 11:24:08 +01:00

Add Selection functionality to weight painting mode #99113

Christoph Lendenfeld added 3 commits 2023-03-24 14:28:50 +01:00

5f14cfd973 Merge branch 'main' into weight_paint_grow_sel_face

e7f242e374 implement changes after MLoop was removed

d05334b691 extract function poly_has_selected_neighbor

Christoph Lendenfeld commented

2023-03-24 14:31:30 +01:00

@HooglyBoogly I've implemented the changes for MLoop and extracted the function poly_has_selected_neighbor
it takes a Span<bool> as opposed to poly_has_unselected_neighbor which takes a BitSpan
apart from that the functions are identical and I am wondering if they can be merged

@HooglyBoogly I've implemented the changes for `MLoop` and extracted the function `poly_has_selected_neighbor` it takes a `Span<bool>` as opposed to `poly_has_unselected_neighbor` which takes a `BitSpan` apart from that the functions are identical and I am wondering if they can be merged

Christoph Lendenfeld requested review from Hans Goudey 2023-03-24 14:33:15 +01:00

Hans Goudey approved these changes 2023-03-24 14:46:55 +01:00

Hans Goudey left a comment

Just one comment about an argument name, otherwise looks good!

source/blender/editors/mesh/editface.cc Outdated

						
				@ -350,6 +350,131 @@ void paintface_select_linked(bContext *C, Object *ob, const int mval[2], const b

				  paintface_flush_flags(C, ob, true, false);

				}

				static bool poly_has_selected_neighbor(blender::Span<int> edge_indices,

Hans Goudey commented

2023-03-24 14:46:03 +01:00

Canonical variable name for the edges of a face is poly_edges

Canonical variable name for the edges of a face is `poly_edges`

Christoph Lendenfeld added 1 commit 2023-03-24 14:54:37 +01:00

40d0722dea change edge_indices to poly_edges

Christoph Lendenfeld added 1 commit 2023-03-24 15:01:09 +01:00

793b5d5e8f remove default false from BitVector

Hans Goudey reviewed 2023-03-24 15:02:40 +01:00

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +452,4 @@

				      continue;

				    }

				    const MPoly &poly = polys[i];

				    for (const int vert_index : corner_verts.slice(poly.loopstart, poly.totloop)) {

Hans Goudey commented

2023-03-24 15:02:40 +01:00

vert_index -> vert here too, though I mentioned that in chat

`vert_index` -> `vert` here too, though I mentioned that in chat

Christoph Lendenfeld added 1 commit 2023-03-24 15:10:58 +01:00

f65e124d32 vert_index -> vert

Christoph Lendenfeld added 1 commit 2023-03-31 14:28:43 +02:00

242b70ff9f Merge branch 'main' into weight_paint_grow_sel_face

Sybren A. Stüvel approved these changes 2023-03-31 14:31:34 +02:00

Sybren A. Stüvel left a comment

LGTM!

Just one inline note, but no strong feelings about it -- just wanted to offer an alternative view. I'll leave the decision to you @ChrisLend.

LGTM! Just one inline note, but no strong feelings about it -- just wanted to offer an alternative view. I'll leave the decision to you @ChrisLend.

source/blender/editors/mesh/editface.cc

						
				@ -353,0 +395,4 @@

				        continue;

				      }

				      const MPoly &poly = polys[i];

				      if (poly_has_selected_neighbor(corner_edges.slice(poly.loopstart, poly.totloop),

Sybren A. Stüvel commented

2023-03-31 14:28:56 +02:00

The condition can be avoided by doing something like:

const bool has_selected_neighbour = poly_has_selected_neighbor(...);
select_poly.span[i] |= has_selected_neighbour;

Not sure if it's worth it though, you choose @ChrisLend. Could be applied below as well.

The condition can be avoided by doing something like: ```cpp const bool has_selected_neighbour = poly_has_selected_neighbor(...); select_poly.span[i] |= has_selected_neighbour; ``` Not sure if it's worth it though, you choose @ChrisLend. Could be applied below as well.

Christoph Lendenfeld reviewed 2023-03-31 14:46:19 +02:00

source/blender/editors/mesh/editface.cc

						
				@ -353,0 +395,4 @@

				        continue;

				      }

				      const MPoly &poly = polys[i];

				      if (poly_has_selected_neighbor(corner_edges.slice(poly.loopstart, poly.totloop),

Christoph Lendenfeld commented

2023-03-31 14:46:15 +02:00

had a look at it but I think it's a bit clearer if the bool is set explicitly so I left it as is

source/blender/editors/mesh/editface.cc Outdated

						
				@ -353,0 +396,4 @@

				  select_vert.finish();

				}

				static bool poly_has_unselected_neighbour(blender::Span<MLoop> poly_loops,

Christoph Lendenfeld commented

2023-03-24 14:10:04 +01:00

thanks, that always gets me

Christoph Lendenfeld merged commit 0187943a3d into main

2023-03-31 14:53:12 +02:00

Christoph Lendenfeld referenced this issue from a commit

2023-03-31 14:53:12 +02:00

Animation: Weight Paint select more/less for faces

Christoph Lendenfeld deleted branch weight_paint_grow_sel_face

2023-03-31 14:53:12 +02:00

Philipp Oeser commented

2023-03-31 15:14:51 +02:00

Just noting this is also for other modes using paint mask (vertexpaint & imagepaint equally benefit from it), noice!

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Animation: Weight Paint select more/less for faces #105607