blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 765 Projects 19 Wiki Activity

Refactor: Weight Paint Select Linked Faces #104577

Merged
Christoph Lendenfeld merged 15 commits from ChrisLend/blender:weight_paint_refactor_face_select into main 2023-02-23 08:26:44 +01:00
Conversation 3 Commits 15 Files Changed 1 +24 -14
1 changed files with 24 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 187bcb9f13 - Show all commits

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

@ -212,6 +212,7 @@ void paintface_reveal(bContext *C, Object *ob, const bool select)
paintface_flush_flags(C, ob, true, true);
}
/* Select faces connected to the given face_indices. Seams are treated as separation. */
static void paintface_select_linked_faces(Mesh *mesh,
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 16:42:01 +01:00
Review
Copy Link

Mesh *mesh -> Mesh &mesh

Using a reference indicates that the pointer won't be null

`Mesh *mesh` -> `Mesh &mesh` Using a reference indicates that the pointer won't be null
const blender::Span<int> face_indices,
const bool select)
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 18:26:22 +01:00
Review
Copy Link

@param islands -> \param islands:
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments

`@param islands` -> `\param islands:` https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments
@ -232,16 +233,21 @@ static void paintface_select_linked_faces(Mesh *mesh,
* are not a seam) we can find connected faces. */
threading::parallel_for(polys.index_range(), 1024, [&](const IndexRange range) {
for (const MPoly &poly : polys.slice(range)) {
const MLoop *prev_loop = &loops[poly.loopstart];
/* A Poly cannot be made of only 1 loop. So this should be save. */
const MLoop *current_loop = &loops[poly.loopstart + 1];
for (int b = 1; b < poly.totloop; b++, current_loop++, prev_loop++) {
if ((edges[current_loop->e].flag & ME_SEAM) != 0 ||
(edges[prev_loop->e].flag & ME_SEAM) != 0) {
/* All edges of the face need to be joined in the DisjointSet. Unless the are marked as seam.
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 16:42:29 +01:00
Review
Copy Link

Grammar: "Unless the are"

Also, the previous comment said the same thing, this is redundant.

Grammar: "Unless the are" Also, the previous comment said the same thing, this is redundant.
*/
for (int outer_loop_index = 0; outer_loop_index < poly.totloop; outer_loop_index++) {
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 16:44:18 +01:00
Review
Copy Link

for (const int loop : IndexRange(poly.loopstart, poly.totloop)) {

`for (const int loop : IndexRange(poly.loopstart, poly.totloop)) {`
const MLoop *outer_mloop = &loops[poly.loopstart + outer_loop_index];
if ((edges[outer_mloop->e].flag & ME_SEAM) != 0) {
continue;
}
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 18:35:54 +01:00
Review
Copy Link

I suggest using a separate const Span<MLoop> poly_loops = loops.slice(poly.loopstart, poly.totloop); variable. Then the next loop can use for (const int outer_loop_index : poly_loops.index_range()) { and you don't have to add poly.totloop every time a loop in the poly is accessed.

I suggest using a separate `const Span<MLoop> poly_loops = loops.slice(poly.loopstart, poly.totloop);` variable. Then the next loop can use `for (const int outer_loop_index : poly_loops.index_range()) {` and you don't have to add `poly.totloop` every time a loop in the poly is accessed.
islands.join(prev_loop->e, current_loop->e);
for (int inner_loop_idx = outer_loop_index + 1; inner_loop_idx < poly.totloop;
Hans Goudey commented 2023-02-10 18:27:31 +01:00
Review
Copy Link

Use references instead of pointers, that should be your default in C++ code.

Use references instead of pointers, that should be your default in C++ code.
Christoph Lendenfeld commented 2023-02-16 10:02:23 +01:00
Review
Copy Link

sure thing, missed that
out of curiosity: what is the reason for it?

sure thing, missed that out of curiosity: what is the reason for it?
Hans Goudey commented 2023-02-16 13:51:55 +01:00
Review
Copy Link

There are quite a few benefits-- they indicate non-null, they allow using . instead of ->, they can't be indexed (so they can't be mistaken for pointers to arrays). There's more info here:

https://stackoverflow.com/questions/57483/what-are-the-differences-between-a-pointer-variable-and-a-reference-variable

There are quite a few benefits-- they indicate non-null, they allow using `.` instead of `->`, they can't be indexed (so they can't be mistaken for pointers to arrays). There's more info here: https://stackoverflow.com/questions/57483/what-are-the-differences-between-a-pointer-variable-and-a-reference-variable
👍 1
inner_loop_idx++) {
const MLoop *inner_mloop = &loops[poly.loopstart + inner_loop_idx];
if ((edges[inner_mloop->e].flag & ME_SEAM) != 0) {
continue;
}
islands.join(inner_mloop->e, outer_mloop->e);
}
}
}
});
@ -251,6 +257,9 @@ static void paintface_select_linked_faces(Mesh *mesh,
MPoly poly = polys[i];
const MLoop *loop = &loops[poly.loopstart];
for (int i = 0; i < poly.totloop; i++, loop++) {
if ((edges[loop->e].flag & ME_SEAM) != 0) {
continue;
}
const int root = islands.find_root(loop->e);
selected_roots.add(root);
}
@ -283,29 +292,30 @@ void paintface_select_linked(bContext *C, Object *ob, const int mval[2], const b
blender::Vector<int> indices;
blender::bke::MutableAttributeAccessor attributes = me->attributes_for_write();
blender::bke::SpanAttributeWriter<bool> select_poly =
attributes.lookup_or_add_for_write_span<bool>(".select_poly", ATTR_DOMAIN_FACE);
if (mval) {
uint index = uint(-1);
if (!ED_mesh_pick_face(C, ob, mval, ED_MESH_PICK_DEFAULT_FACE_DIST, &index)) {
return;
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 16:52:47 +01:00
Review
Copy Link

Looks like this early return will skip select_poly.finish();. That will probably print a warning to the terminal.

Looks like this early return will skip `select_poly.finish();`. That will probably print a warning to the terminal.
}
select_poly.span[index] = true;
indices.append(index);
}
else {
blender::bke::MutableAttributeAccessor attributes = me->attributes_for_write();
blender::bke::SpanAttributeWriter<bool> select_poly =
attributes.lookup_or_add_for_write_span<bool>(".select_poly", ATTR_DOMAIN_FACE);
for (const int i : select_poly.span.index_range()) {
if (!select_poly.span[i]) {
continue;
}
indices.append(i);
}
ChrisLend marked this conversation as resolved
Hans Goudey commented 2023-02-10 18:31:00 +01:00
Review
Copy Link

Declare this closer to where it's first used, right above if (mval) {

Declare this closer to where it's first used, right above `if (mval) {`
select_poly.finish();
}
select_poly.finish();
paintface_select_linked_faces(me, indices, select);
paintface_flush_flags(C, ob, true, false);
}