blender
/
blender
134
398
Fork 579
Code Issues 6.3k Pull Requests 750 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 +30 -19
1 changed files with 30 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 158d68471e - Show all commits

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

@ -401,6 +401,30 @@ void paintface_select_more(bContext *C, Object *ob, const bool face_step)
paintface_flush_flags(C, ob, true, false);
}
static bool poly_has_unselected_neighbour(const MPoly &poly,
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
blender::Span<MLoop> poly_loops,
blender::Span<MEdge> edges,
blender::BitVector<> &verts_of_unselected_faces,
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.
const bool face_step)
{
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
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
if (face_step) {
unselected_neighbor = verts_of_unselected_faces[edge.v1].test() ||
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
verts_of_unselected_faces[edge.v2].test();
}
else {
unselected_neighbor = verts_of_unselected_faces[edge.v1].test() &&
verts_of_unselected_faces[edge.v2].test();
}
if (unselected_neighbor) {
return true;
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.
}
}
return false;
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.
}
void paintface_select_less(bContext *C, Object *ob, const bool face_step)
{
using namespace blender;
@ -419,7 +443,7 @@ void paintface_select_less(bContext *C, Object *ob, const bool face_step)
const Span<MLoop> loops = mesh->loops();
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)) { ... } ```
const Span<MEdge> edges = mesh->edges();
std::vector<bool> verts_of_unselected_faces(mesh->totvert, false);
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
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 :)
@ -428,32 +452,19 @@ void paintface_select_less(bContext *C, Object *ob, const bool face_step)
}
const MPoly &poly = polys[i];
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
verts_of_unselected_faces[loop.v] = true;
verts_of_unselected_faces[loop.v].set(true);
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
}
}
threading::parallel_for(select_poly.span.index_range(), 1024, [&](const IndexRange range) {
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];
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;
}
if (poly_has_unselected_neighbour(
poly, loops, edges, verts_of_unselected_faces, face_step)) {
select_poly.span[i] = false;
}
}
});