blender
/
blender
133
398
Fork 579
Code Issues 6.3k Pull Requests 746 Projects 19 Wiki Activity

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
Conversation 6 Commits 13 Files Changed 7 +36 -31
3 changed files with 36 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 24aee56b7f - Show all commits

4
source/blender/editors/include/ED_mesh.h
View File

@ -422,8 +422,8 @@ void paintface_select_linked(struct bContext *C,
/** 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 bContext *C, struct Object *ob, bool face_step);
void paintface_select_less(struct bContext *C, struct Object *ob, bool face_step);
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);

43
source/blender/editors/mesh/editface.cc
View File

@ -350,13 +350,9 @@ void paintface_select_linked(bContext *C, Object *ob, const int mval[2], const b
paintface_flush_flags(C, ob, true, false);
}
void paintface_select_more(bContext *C, Object *ob, const bool face_step)
void paintface_select_more(Mesh *mesh, const bool face_step)
Hans Goudey commented 2023-03-24 14:46:03 +01:00
Outdated
Review
Copy Link

Canonical variable name for the edges of a face is poly_edges

Canonical variable name for the edges of a face is `poly_edges`
{
using namespace blender;
Mesh *mesh = BKE_mesh_from_object(ob);
if (mesh == nullptr || mesh->totpoly == 0) {
return;
}
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<bool> select_poly = attributes.lookup_or_add_for_write_span<bool>(
@ -398,40 +394,32 @@ void paintface_select_more(bContext *C, Object *ob, const bool face_step)
select_poly.finish();
select_vert.finish();
paintface_flush_flags(C, ob, true, false);
}
Sybren A. Stüvel commented 2023-03-31 14:28:56 +02:00
Review
Copy Link

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 commented 2023-03-31 14:46:15 +02:00
Review
Copy Link

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
static bool poly_has_unselected_neighbour(const MPoly &poly,
blender::Span<MLoop> poly_loops,
static bool poly_has_unselected_neighbour(blender::Span<MLoop> poly_loops,
Hans Goudey commented 2023-03-23 15:19:50 +01:00
Outdated
Review
Copy Link

Blender uses American English spelling, so neighbor instead of neighbor

Blender uses American English spelling, so `neighbor` instead of `neighbor`
Christoph Lendenfeld commented 2023-03-24 14:10:04 +01:00
Outdated
Review
Copy Link

thanks, that always gets me

thanks, that always gets me
blender::Span<MEdge> edges,
blender::BitVector<> &verts_of_unselected_faces,
blender::BitSpan verts_of_unselected_faces,
const bool face_step)
{
for (const MLoop &loop : poly_loops.slice(poly.loopstart, poly.totloop)) {
for (const MLoop &loop : poly_loops) {
Hans Goudey commented 2023-03-09 17:01:38 +01:00
Outdated
Review
Copy Link

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.
Christoph Lendenfeld commented 2023-03-16 10:53:36 +01:00
Outdated
Review
Copy Link

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
Hans Goudey commented 2023-03-18 02:19:02 +01:00
Outdated
Review
Copy Link

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.
Christoph Lendenfeld commented 2023-03-23 10:55:00 +01:00
Outdated
Review
Copy Link

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
const MEdge &edge = edges[loop.e];
bool unselected_neighbor = false;
if (face_step) {
unselected_neighbor = verts_of_unselected_faces[edge.v1].test() ||
verts_of_unselected_faces[edge.v2].test();
if (verts_of_unselected_faces[edge.v1] || verts_of_unselected_faces[edge.v2]) {
ChrisLend marked this conversation as resolved Outdated
Hans Goudey commented 2023-03-18 02:22:44 +01:00
Outdated
Review
Copy Link

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.
return true;
}
}
else {
unselected_neighbor = verts_of_unselected_faces[edge.v1].test() &&
verts_of_unselected_faces[edge.v2].test();
}
if (unselected_neighbor) {
return true;
if (verts_of_unselected_faces[edge.v1] && verts_of_unselected_faces[edge.v2]) {
Hans Goudey commented 2023-03-18 02:26:49 +01:00
Outdated
Review
Copy Link

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.
Christoph Lendenfeld commented 2023-03-23 10:52:16 +01:00
Outdated
Review
Copy Link

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
return true;
}
ChrisLend marked this conversation as resolved Outdated
Hans Goudey commented 2023-03-18 02:23:42 +01:00
Outdated
Review
Copy Link

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?
Christoph Lendenfeld commented 2023-03-23 10:48:53 +01:00
Outdated
Review
Copy Link

yes good idea, done that

yes good idea, done that
}
}
return false;
}
void paintface_select_less(bContext *C, Object *ob, const bool face_step)
void paintface_select_less(Mesh *mesh, const bool face_step)
{
using namespace blender;
ChrisLend marked this conversation as resolved Outdated
Hans Goudey commented 2023-03-09 16:59:53 +01:00
Outdated
Review
Copy Link

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.
Mesh *mesh = BKE_mesh_from_object(ob);
if (mesh == nullptr || mesh->totpoly == 0) {
return;
}
bke::MutableAttributeAccessor attributes = mesh->attributes_for_write();
bke::SpanAttributeWriter<bool> select_poly = attributes.lookup_or_add_for_write_span<bool>(
ChrisLend marked this conversation as resolved Outdated
Hans Goudey commented 2023-03-09 17:04:44 +01:00
Outdated
Review
Copy Link

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.
@ -446,7 +434,7 @@ void paintface_select_less(bContext *C, Object *ob, const bool face_step)
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()) {
for (const int i : polys.index_range()) {
if (select_poly.span[i]) {
continue;
}
@ -462,15 +450,16 @@ void paintface_select_less(bContext *C, Object *ob, const bool face_step)
continue;
}
const MPoly &poly = polys[i];
if (poly_has_unselected_neighbour(
poly, loops, edges, verts_of_unselected_faces, face_step)) {
if (poly_has_unselected_neighbour(loops.slice(poly.loopstart, poly.totloop),
edges,
verts_of_unselected_faces,
Hans Goudey commented 2023-03-24 15:02:40 +01:00
Outdated
Review
Copy Link

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

`vert_index` -> `vert` here too, though I mentioned that in chat
face_step)) {
select_poly.span[i] = false;
}
}
});
select_poly.finish();
paintface_flush_flags(C, ob, true, false);
}
bool paintface_deselect_all_visible(bContext *C, Object *ob, int action, bool flush_flags)

20
source/blender/editors/sculpt_paint/paint_utils.c
View File

@ -695,8 +695,16 @@ void PAINT_OT_face_select_all(wmOperatorType *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(C, CTX_data_active_object(C), 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;
}
@ -718,8 +726,16 @@ void PAINT_OT_face_select_more(wmOperatorType *ot)
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(C, CTX_data_active_object(C), 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;
}