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 +17 -16
1 changed files with 17 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e7f242e374 - Show all commits

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

@ -363,7 +363,7 @@ void paintface_select_more(Mesh *mesh, const bool face_step)
".hide_poly", ATTR_DOMAIN_FACE, false);
const Span<MPoly> polys = mesh->polys();
const Span<MLoop> loops = mesh->loops();
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) {
@ -373,8 +373,8 @@ void paintface_select_more(Mesh *mesh, const bool face_step)
}
const MPoly &poly = polys[i];
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
const MEdge &edge = edges[loop.e];
for (const int edge_index : corner_edges.slice(poly.loopstart, poly.totloop)) {
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. */
bool selected_neighbor = false;
@ -396,13 +396,13 @@ void paintface_select_more(Mesh *mesh, const bool face_step)
select_vert.finish();
}
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(blender::Span<MLoop> poly_loops,
blender::Span<MEdge> edges,
blender::BitSpan verts_of_unselected_faces,
const bool face_step)
static bool poly_has_unselected_neighbor(blender::Span<int> edge_indices,
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::BitSpan verts_of_unselected_faces,
const bool face_step)
{
for (const MLoop &loop : poly_loops) {
const MEdge &edge = edges[loop.e];
for (const int edge_index : edge_indices) {
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[edge_index];
if (face_step) {
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;
@ -428,7 +428,8 @@ void paintface_select_less(Mesh *mesh, const bool face_step)
".hide_poly", ATTR_DOMAIN_FACE, false);
const Span<MPoly> polys = mesh->polys();
const Span<MLoop> loops = mesh->loops();
const Span<int> corner_verts = mesh->corner_verts();
const Span<int> corner_edges = mesh->corner_edges();
const Span<MEdge> edges = mesh->edges();
BitVector<> verts_of_unselected_faces(mesh->totvert, false);
@ -439,8 +440,8 @@ void paintface_select_less(Mesh *mesh, const bool face_step)
continue;
}
const MPoly &poly = polys[i];
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.
for (const MLoop &loop : loops.slice(poly.loopstart, poly.totloop)) {
verts_of_unselected_faces[loop.v].set(true);
for (const int vert_index : corner_verts.slice(poly.loopstart, poly.totloop)) {
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)) { ... } ```
verts_of_unselected_faces[vert_index].set(true);
}
}
@ -450,10 +451,10 @@ void paintface_select_less(Mesh *mesh, const bool face_step)
continue;
}
const MPoly &poly = polys[i];
if (poly_has_unselected_neighbour(loops.slice(poly.loopstart, poly.totloop),
edges,
verts_of_unselected_faces,
face_step)) {
if (poly_has_unselected_neighbor(corner_edges.slice(poly.loopstart, poly.totloop),
edges,
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
verts_of_unselected_faces,
face_step)) {
select_poly.span[i] = false;
}
}