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

Merged
Christoph Lendenfeld merged 13 commits from ChrisLend/blender:weight_paint_grow_sel_face into main 2023-03-31 14:53:12 +02:00
7 changed files with 201 additions and 0 deletions

View File

@ -4410,6 +4410,8 @@ def km_face_mask(params):
{"properties": [("deselect", False)]}),
("paint.face_select_linked_pick", {"type": 'L', "value": 'PRESS', "shift": True},
{"properties": [("deselect", True)]}),
("paint.face_select_more", {"type": 'NUMPAD_PLUS', "value": 'PRESS', "ctrl": True}, None),
("paint.face_select_less", {"type": 'NUMPAD_MINUS', "value": 'PRESS', "ctrl": True}, None),
])
return keymap

View File

@ -2011,6 +2011,9 @@ class VIEW3D_MT_select_paint_mask(Menu):
layout.operator("paint.face_select_all", text="None").action = 'DESELECT'
layout.operator("paint.face_select_all", text="Invert").action = 'INVERT'
layout.operator("paint.face_select_more")
layout.operator("paint.face_select_less")
layout.separator()
layout.operator("view3d.select_box")

View File

@ -419,6 +419,11 @@ void paintface_select_linked(struct bContext *C,
struct Object *ob,
const int mval[2],
bool select);
/** Grow the selection of faces.
* \param face_step If true will also select faces that only touch on the corner.
*/
void paintface_select_more(struct Mesh *mesh, bool face_step);
void paintface_select_less(struct Mesh *mesh, bool face_step);
bool paintface_minmax(struct Object *ob, float r_min[3], float r_max[3]);
void paintface_hide(struct bContext *C, struct Object *ob, bool unselected);

View File

@ -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> poly_edges,

Canonical variable name for the edges of a face is poly_edges

Canonical variable name for the edges of a face is `poly_edges`
blender::Span<MEdge> edges,
blender::Span<bool> select_vert,
const bool face_step)
{
for (const int edge_index : poly_edges) {
const MEdge &edge = edges[edge_index];
/* If a poly is selected, all of its verts are selected too, meaning that neighboring faces
* will have some vertices selected. */
if (face_step) {
if (select_vert[edge.v1] || select_vert[edge.v2]) {
return true;
}
}
else {
if (select_vert[edge.v1] && select_vert[edge.v2]) {
return true;
}
}
}
return false;
}
void paintface_select_more(Mesh *mesh, const bool face_step)
{
using namespace blender;
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<bool> select_poly = attributes.lookup_or_add_for_write_span<bool>(
".select_poly", ATTR_DOMAIN_FACE);
bke::SpanAttributeWriter<bool> select_vert = attributes.lookup_or_add_for_write_span<bool>(
".select_vert", ATTR_DOMAIN_POINT);
const VArray<bool> hide_poly = attributes.lookup_or_default<bool>(
".hide_poly", ATTR_DOMAIN_FACE, false);
const Span<MPoly> polys = mesh->polys();

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.
const Span<int> corner_edges = mesh->corner_edges();
const Span<MEdge> edges = mesh->edges();
threading::parallel_for(select_poly.span.index_range(), 1024, [&](const IndexRange range) {
for (const int i : range) {
if (select_poly.span[i] || hide_poly[i]) {
continue;
}
const MPoly &poly = polys[i];
if (poly_has_selected_neighbor(corner_edges.slice(poly.loopstart, poly.totloop),

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.

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

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
edges,

Blender uses American English spelling, so neighbor instead of neighbor

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

thanks, that always gets me

thanks, that always gets me
select_vert.span,
face_step)) {
select_poly.span[i] = true;
}
}

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.

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

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.

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
});
select_poly.finish();
ChrisLend marked this conversation as resolved Outdated

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.
select_vert.finish();
}
static bool poly_has_unselected_neighbor(blender::Span<int> poly_edges,
blender::Span<MEdge> edges,

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.

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

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
blender::BitSpan verts_of_unselected_faces,
const bool face_step)
ChrisLend marked this conversation as resolved Outdated

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?

yes good idea, done that

yes good idea, done that
{
for (const int edge_index : poly_edges) {
const MEdge &edge = edges[edge_index];
if (face_step) {
if (verts_of_unselected_faces[edge.v1] || verts_of_unselected_faces[edge.v2]) {
return true;
}
}
ChrisLend marked this conversation as resolved Outdated

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.
else {
if (verts_of_unselected_faces[edge.v1] && verts_of_unselected_faces[edge.v2]) {
return true;
ChrisLend marked this conversation as resolved Outdated

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.
}
}
}
return false;
}
void paintface_select_less(Mesh *mesh, const bool face_step)
{
using namespace blender;
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<bool> select_poly = attributes.lookup_or_add_for_write_span<bool>(
".select_poly", ATTR_DOMAIN_FACE);
const VArray<bool> hide_poly = attributes.lookup_or_default<bool>(
".hide_poly", ATTR_DOMAIN_FACE, false);
const Span<MPoly> polys = mesh->polys();

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.

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

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
Review

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.
const Span<int> corner_verts = mesh->corner_verts();

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)) { ... } ```
const Span<int> corner_edges = mesh->corner_edges();
const Span<MEdge> edges = mesh->edges();
BitVector<> verts_of_unselected_faces(mesh->totvert);
/* Find all vertices of unselected faces to help find neighboring faces after. */

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

yep agreed, missed that one :)

yep agreed, missed that one :)
for (const int i : polys.index_range()) {
if (select_poly.span[i]) {
continue;
}
const MPoly &poly = polys[i];
for (const int vert : corner_verts.slice(poly.loopstart, poly.totloop)) {

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

`vert_index` -> `vert` here too, though I mentioned that in chat
verts_of_unselected_faces[vert].set(true);
}
}
threading::parallel_for(polys.index_range(), 1024, [&](const IndexRange range) {
for (const int i : range) {
if (!select_poly.span[i] || hide_poly[i]) {
continue;
}
const MPoly &poly = polys[i];
if (poly_has_unselected_neighbor(corner_edges.slice(poly.loopstart, poly.totloop),
edges,
verts_of_unselected_faces,
face_step)) {
select_poly.span[i] = false;
}
}
});
select_poly.finish();
}
bool paintface_deselect_all_visible(bContext *C, Object *ob, int action, bool flush_flags)
{
using namespace blender;

View File

@ -378,6 +378,8 @@ void BRUSH_OT_sculpt_curves_falloff_preset(struct wmOperatorType *ot);
void PAINT_OT_face_select_linked(struct wmOperatorType *ot);
void PAINT_OT_face_select_linked_pick(struct wmOperatorType *ot);
void PAINT_OT_face_select_all(struct wmOperatorType *ot);
void PAINT_OT_face_select_more(struct wmOperatorType *ot);
void PAINT_OT_face_select_less(struct wmOperatorType *ot);
void PAINT_OT_face_select_hide(struct wmOperatorType *ot);
void PAINT_OT_face_vert_reveal(struct wmOperatorType *ot);

View File

@ -1518,6 +1518,8 @@ void ED_operatortypes_paint(void)
WM_operatortype_append(PAINT_OT_face_select_linked);
WM_operatortype_append(PAINT_OT_face_select_linked_pick);
WM_operatortype_append(PAINT_OT_face_select_all);
WM_operatortype_append(PAINT_OT_face_select_more);
WM_operatortype_append(PAINT_OT_face_select_less);
WM_operatortype_append(PAINT_OT_face_select_hide);
WM_operatortype_append(PAINT_OT_face_vert_reveal);

View File

@ -693,6 +693,68 @@ void PAINT_OT_face_select_all(wmOperatorType *ot)
WM_operator_properties_select_all(ot);
}
static int paint_select_more_exec(bContext *C, wmOperator *op)
{
Object *ob = CTX_data_active_object(C);
Mesh *mesh = BKE_mesh_from_object(ob);
if (mesh == NULL || mesh->totpoly == 0) {
return OPERATOR_CANCELLED;
}
const bool face_step = RNA_boolean_get(op->ptr, "face_step");
paintface_select_more(mesh, face_step);
paintface_flush_flags(C, ob, true, false);
ED_region_tag_redraw(CTX_wm_region(C));
return OPERATOR_FINISHED;
}
void PAINT_OT_face_select_more(wmOperatorType *ot)
{
ot->name = "Select More";
ot->description = "Select Faces connected to existing selection";
ot->idname = "PAINT_OT_face_select_more";
ot->exec = paint_select_more_exec;
ot->poll = facemask_paint_poll;
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
RNA_def_boolean(
ot->srna, "face_step", true, "Face Step", "Also select faces that only touch on a corner");
}
static int paint_select_less_exec(bContext *C, wmOperator *op)
{
Object *ob = CTX_data_active_object(C);
Mesh *mesh = BKE_mesh_from_object(ob);
if (mesh == NULL || mesh->totpoly == 0) {
return OPERATOR_CANCELLED;
}
const bool face_step = RNA_boolean_get(op->ptr, "face_step");
paintface_select_less(mesh, face_step);
paintface_flush_flags(C, ob, true, false);
ED_region_tag_redraw(CTX_wm_region(C));
return OPERATOR_FINISHED;
}
void PAINT_OT_face_select_less(wmOperatorType *ot)
{
ot->name = "Select Less";
ot->description = "Deselect Faces connected to existing selection";
ot->idname = "PAINT_OT_face_select_less";
ot->exec = paint_select_less_exec;
ot->poll = facemask_paint_poll;
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
RNA_def_boolean(
ot->srna, "face_step", true, "Face Step", "Also deselect faces that only touch on a corner");
}
static int vert_select_all_exec(bContext *C, wmOperator *op)
{
Object *ob = CTX_data_active_object(C);