blender
/
blender
133
398
Fork 579
Code Issues 6.3k Pull Requests 747 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 +44 -40
1 changed files with 44 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ad7e632371 - Show all commits

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

@ -370,29 +370,31 @@ void paintface_select_more(bContext *C, Object *ob, const bool face_step)
const Span<MLoop> loops = mesh->loops();
const Span<MEdge> edges = mesh->edges();
for (const int i : select_poly.span.index_range()) {
if (select_poly.span[i] || hide_poly[i]) {
continue;
}
const MPoly &poly = polys[i];
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];
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
const MEdge &edge = edges[loop.e];
/* 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 {
selected_neighbor = select_vert.span[edge.v1] && select_vert.span[edge.v2];
}
if (selected_neighbor) {
select_poly.span[i] = true;
break;
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
const MEdge &edge = edges[loop.e];
/* 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 {
Hans Goudey commented 2023-03-23 15:19:29 +01:00
Outdated
Review
Copy Link

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.
selected_neighbor = select_vert.span[edge.v1] && select_vert.span[edge.v2];
}
if (selected_neighbor) {
select_poly.span[i] = true;
break;
}
}
}
}
});
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
select_poly.finish();
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
select_vert.finish();
@ -430,29 +432,31 @@ void paintface_select_less(bContext *C, Object *ob, const bool face_step)
}
}
for (const int i : select_poly.span.index_range()) {
if (!select_poly.span[i] || hide_poly[i]) {
continue;
}
const MPoly &poly = polys[i];
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];
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
const MEdge &edge = edges[loop.e];
bool unselected_neighbor = false;
if (face_step) {
unselected_neighbor = verts_of_unselected_faces[edge.v1] ||
verts_of_unselected_faces[edge.v2];
}
else {
unselected_neighbor = verts_of_unselected_faces[edge.v1] &&
verts_of_unselected_faces[edge.v2];
}
if (unselected_neighbor) {
select_poly.span[i] = false;
break;
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
Hans Goudey commented 2023-03-09 17:09:58 +01:00
Outdated
Review
Copy Link

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-16 10:59:32 +01:00
Outdated
Review
Copy Link

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 2023-03-18 02:28:44 +01:00
Review
Copy Link

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 MEdge &edge = edges[loop.e];
Hans Goudey commented 2023-03-23 15:17:40 +01:00
Outdated
Review
Copy Link

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)) { ... } ```
bool unselected_neighbor = false;
if (face_step) {
unselected_neighbor = verts_of_unselected_faces[edge.v1] ||
verts_of_unselected_faces[edge.v2];
}
else {
Hans Goudey commented 2023-03-18 02:28:33 +01:00
Outdated
Review
Copy Link

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 commented 2023-03-23 10:47:26 +01:00
Outdated
Review
Copy Link

yep agreed, missed that one :)

yep agreed, missed that one :)
unselected_neighbor = verts_of_unselected_faces[edge.v1] &&
verts_of_unselected_faces[edge.v2];
}
if (unselected_neighbor) {
select_poly.span[i] = false;
break;
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
}
}
}
}
});
select_poly.finish();
paintface_flush_flags(C, ob, true, false);