Animation: Weight Paint select more/less for faces #105607
|
@ -4411,6 +4411,7 @@ def km_face_mask(params):
|
|||
("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
|
||||
|
|
|
@ -2012,6 +2012,7 @@ class VIEW3D_MT_select_paint_mask(Menu):
|
|||
layout.operator("paint.face_select_all", text="Invert").action = 'INVERT'
|
||||
|
||||
layout.operator("paint.face_select_more")
|
||||
layout.operator("paint.face_select_less")
|
||||
|
||||
layout.separator()
|
||||
|
||||
|
|
|
@ -423,6 +423,7 @@ void paintface_select_linked(struct bContext *C,
|
|||
* \param face_step If true will also select faces that only touch on the corner.
|
||||
*/
|
||||
void paintface_select_more(struct bContext *C, struct Object *ob, bool face_step);
|
||||
void paintface_select_less(struct bContext *C, struct Object *ob, 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);
|
||||
|
|
|
@ -370,8 +370,6 @@ void paintface_select_more(bContext *C, Object *ob, const bool face_step)
|
|||
const Span<MLoop> loops = mesh->loops();
|
||||
const Span<MEdge> edges = mesh->edges();
|
||||
|
||||
Vector<int> indices;
|
||||
|
||||
for (const int i : select_poly.span.index_range()) {
|
||||
if (select_poly.span[i] || hide_poly[i]) {
|
||||
continue;
|
||||
|
@ -380,11 +378,16 @@ void paintface_select_more(bContext *C, Object *ob, const bool face_step)
|
|||
|
||||
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
|
||||
const MEdge &edge = edges[loop.e];
|
||||
if (face_step && (select_vert.span[edge.v1] || select_vert.span[edge.v2])) {
|
||||
select_poly.span[i] = true;
|
||||
break;
|
||||
/* If a poly is selected, all of its verts are selected too, meaning that neighboring faces
|
||||
* will have some vertices selected. */
|
||||
bool selected_neighbor = false;
|
||||
if (face_step) {
|
||||
selected_neighbor = select_vert.span[edge.v1] || select_vert.span[edge.v2];
|
||||
}
|
||||
else if (select_vert.span[edge.v1] && select_vert.span[edge.v2]) {
|
||||
else {
|
||||
selected_neighbor = select_vert.span[edge.v1] && select_vert.span[edge.v2];
|
||||
|
||||
}
|
||||
if (selected_neighbor) {
|
||||
select_poly.span[i] = true;
|
||||
break;
|
||||
}
|
||||
|
@ -396,6 +399,65 @@ void paintface_select_more(bContext *C, Object *ob, const bool face_step)
|
|||
paintface_flush_flags(C, ob, true, false);
|
||||
Hans Goudey
commented
Blender uses American English spelling, so Blender uses American English spelling, so `neighbor` instead of `neighbor`
|
||||
}
|
||||
|
||||
void paintface_select_less(bContext *C, Object *ob, const bool face_step)
|
||||
{
|
||||
using namespace blender;
|
||||
Hans Goudey
commented
It might be clearer for these functions to have slightly lower level arguments, like 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. Edit since it seems to not have linked to your comment. 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
Hans Goudey
commented
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.
|
||||
Mesh *mesh = BKE_mesh_from_object(ob);
|
||||
if (mesh == nullptr || mesh->totpoly == 0) {
|
||||
return;
|
||||
ChrisLend marked this conversation as resolved
Outdated
Hans Goudey
commented
Replace the 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.
|
||||
}
|
||||
|
||||
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);
|
||||
Hans Goudey
commented
It's not a big deal, but the function could be a bit simpler without the 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.
|
||||
const VArray<bool> hide_poly = attributes.lookup_or_default<bool>(
|
||||
".hide_poly", ATTR_DOMAIN_FACE, false);
|
||||
ChrisLend marked this conversation as resolved
Outdated
Hans Goudey
commented
What do you think about removing the What do you think about removing the `.test()` and using `BitRef`'s implicit bool conversion?
|
||||
|
||||
const Span<MPoly> polys = mesh->polys();
|
||||
const Span<MLoop> loops = mesh->loops();
|
||||
const Span<MEdge> edges = mesh->edges();
|
||||
|
||||
std::vector<bool> verts_of_unselected_faces(mesh->totvert, false);
|
||||
|
||||
/* Find all vertices of unselected faces to help find neighboring faces after. */
|
||||
ChrisLend marked this conversation as resolved
Outdated
Hans Goudey
commented
I assume you're using 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.
|
||||
for (const int i : select_poly.span.index_range()) {
|
||||
if (select_poly.span[i]) {
|
||||
continue;
|
||||
ChrisLend marked this conversation as resolved
Outdated
Hans Goudey
commented
Small thing, but maybe it's a bit clearer to write 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.
|
||||
}
|
||||
const MPoly &poly = polys[i];
|
||||
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
|
||||
verts_of_unselected_faces[loop.v] = true;
|
||||
}
|
||||
}
|
||||
|
||||
for (const int i : select_poly.span.index_range()) {
|
||||
if (!select_poly.span[i] || hide_poly[i]) {
|
||||
continue;
|
||||
}
|
||||
const MPoly &poly = polys[i];
|
||||
|
||||
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
|
||||
const MEdge &edge = edges[loop.e];
|
||||
bool unselected_neighbor = false;
|
||||
if (face_step) {
|
||||
Hans Goudey
commented
What do you think about splitting this part into a separate function-- something like 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
Hans Goudey
commented
`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.
|
||||
unselected_neighbor = verts_of_unselected_faces[edge.v1] ||
|
||||
Hans Goudey
commented
`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)) {
...
}
```
|
||||
verts_of_unselected_faces[edge.v2];
|
||||
}
|
||||
else {
|
||||
unselected_neighbor = verts_of_unselected_faces[edge.v1] &&
|
||||
verts_of_unselected_faces[edge.v2];
|
||||
}
|
||||
Hans Goudey
commented
Might as well change this to 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
|
||||
if (unselected_neighbor) {
|
||||
select_poly.span[i] = false;
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
Hans Goudey
commented
`vert_index` -> `vert` here too, though I mentioned that in chat
|
||||
|
||||
select_poly.finish();
|
||||
paintface_flush_flags(C, ob, true, false);
|
||||
}
|
||||
|
||||
bool paintface_deselect_all_visible(bContext *C, Object *ob, int action, bool flush_flags)
|
||||
{
|
||||
using namespace blender;
|
||||
|
|
|
@ -379,6 +379,7 @@ 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);
|
||||
|
|
|
@ -1519,6 +1519,7 @@ void ED_operatortypes_paint(void)
|
|||
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);
|
||||
|
|
|
@ -693,7 +693,6 @@ void PAINT_OT_face_select_all(wmOperatorType *ot)
|
|||
WM_operator_properties_select_all(ot);
|
||||
}
|
||||
|
||||
/* face-select ops */
|
||||
static int paint_select_more_exec(bContext *C, wmOperator *op)
|
||||
{
|
||||
const bool face_step = RNA_boolean_get(op->ptr, "face_step");
|
||||
|
@ -717,6 +716,29 @@ void PAINT_OT_face_select_more(wmOperatorType *ot)
|
|||
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)
|
||||
{
|
||||
const bool face_step = RNA_boolean_get(op->ptr, "face_step");
|
||||
paintface_select_less(C, CTX_data_active_object(C), face_step);
|
||||
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);
|
||||
|
|
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.