Paint: Add loop select for faces #107653

Merged
Christoph Lendenfeld merged 28 commits from ChrisLend/blender:paint_loop_face into main 2023-06-01 14:59:03 +02:00

Add loop selection identical to the behavior of ALT+Click in Edit Mode.

  • ALT click: select loop
  • ALT Shift click: add loop while retaining current selection
  • ALT Shift Ctrl click: deselect loop

Design Task: #99113: Add Selection functionality to weight painting mode

Add loop selection identical to the behavior of ALT+Click in Edit Mode. * ALT click: select loop * ALT Shift click: add loop while retaining current selection * ALT Shift Ctrl click: deselect loop ------ Design Task: [#99113: Add Selection functionality to weight painting mode](https://projects.blender.org/blender/blender/issues/99113)
Christoph Lendenfeld added 2 commits 2023-05-05 16:01:47 +02:00
Christoph Lendenfeld added 2 commits 2023-05-05 17:26:26 +02:00
Hans Goudey reviewed 2023-05-08 14:21:24 +02:00
Hans Goudey left a comment
Member

In the UI, the term "face corner" is generally preferred over "loop" since it matches the attribute domain name. And also since it's way less confusing IMO.

In the UI, the term "face corner" is generally preferred over "loop" since it matches the attribute domain name. And also since it's way less confusing IMO.
Author
Member

In the UI, the term "face corner" is generally preferred over "loop" since it matches the attribute domain name. And also since it's way less confusing IMO.

but the feature is supposed to be mirroring the ALT-click from Edit Mode for which we do use the term loop AFAIK.
The title might be misleading since it's loop select for faces and not selecting face loops

> In the UI, the term "face corner" is generally preferred over "loop" since it matches the attribute domain name. And also since it's way less confusing IMO. but the feature is supposed to be mirroring the ALT-click from Edit Mode for which we do use the term loop AFAIK. The title might be misleading since it's loop select for faces and not selecting face loops
Member

Ah, I'm sorry, I read PR very briefly and incorrectly! I thought it was talking about the corner of a face (previously MLoop). Thanks for clarifying.

Ah, I'm sorry, I read PR very briefly and incorrectly! I thought it was talking about the corner of a face (previously `MLoop`). Thanks for clarifying.
Christoph Lendenfeld added 2 commits 2023-05-11 11:37:22 +02:00
Christoph Lendenfeld added 1 commit 2023-05-11 15:22:32 +02:00
Christoph Lendenfeld added 1 commit 2023-05-11 15:32:08 +02:00
Christoph Lendenfeld added 3 commits 2023-05-11 16:36:47 +02:00
Christoph Lendenfeld changed title from WIP: Paint: Add face loop select to Paint: Add face loop select 2023-05-11 16:36:59 +02:00
Christoph Lendenfeld added 1 commit 2023-05-11 16:44:24 +02:00
Christoph Lendenfeld changed title from Paint: Add face loop select to Paint: Add loop select for faces 2023-05-11 16:58:02 +02:00
Christoph Lendenfeld added this to the Animation & Rigging project 2023-05-11 17:14:49 +02:00
Christoph Lendenfeld requested review from Hans Goudey 2023-05-11 17:14:56 +02:00
Hans Goudey requested changes 2023-05-12 17:11:43 +02:00
Hans Goudey left a comment
Member

I didn't really test yet, but generally the approach looks good. There's a fair amount of small cleanup to do though.

I didn't really test yet, but generally the approach looks good. There's a fair amount of small cleanup to do though.
@ -250,3 +251,3 @@
for (const int inner_edge :
poly_edges.slice(poly_loop_index, poly_edges.size() - poly_loop_index)) {
poly_edges.slice(poly_loop_index, poly_edges.size() - poly_loop_index))
Member

Format on save and make format don't change this for me. Maybe some configuration thing to look into?

Format on save and `make format` don't change this for me. Maybe some configuration thing to look into?
@ -348,6 +350,196 @@ void paintface_select_linked(bContext *C, Object *ob, const int mval[2], const b
paintface_flush_flags(C, ob, true, false);
}
static int get_closest_edge_index(ARegion *region,
Member

Suggest renaming get_closest_edge_index to find_closest_edge_in_poly. The fact that it returns an index isn't that helpful

Suggest renaming `get_closest_edge_index` to `find_closest_edge_in_poly`. The fact that it returns an index isn't that helpful
ChrisLend marked this conversation as resolved
@ -351,0 +353,4 @@
static int get_closest_edge_index(ARegion *region,
blender::Span<blender::int2> edges,
blender::Span<int> poly_edges,
blender::Span<blender::float3> verts,
Member

The array of vertex positions should be called positions or vert_positions. "verts" could mean any attribute stored per vertex.

The array of vertex positions should be called `positions` or `vert_positions`. "verts" could mean any attribute stored per vertex.
ChrisLend marked this conversation as resolved
@ -351,0 +359,4 @@
using namespace blender;
int closest_edge_index;
const float mval_f[2] = {float(mval[0]), float(mval[1])};
Member

Use float2 here, and math::distance_squared below.

Use `float2` here, and `math::distance_squared` below.
ChrisLend marked this conversation as resolved
@ -351,0 +366,4 @@
const int2 edge = edges[i];
const float3 v1 = verts[edge[0]];
const float3 v2 = verts[edge[1]];
float3 edge_vert_average;
Member

If you include BLI_math_vector.hh, this can be much simpler:

    const float3 edge_position = math::midpoint(positions[edge[0]], positions[edge[1]]);
If you include `BLI_math_vector.hh`, this can be much simpler: ```cpp const float3 edge_position = math::midpoint(positions[edge[0]], positions[edge[1]]); ```
ChrisLend marked this conversation as resolved
@ -351,0 +383,4 @@
return closest_edge_index;
}
static int get_opposing_edge_index(blender::IndexRange &poly,
Member

IndexRange is a small struct and should be passed by value: const IndexRange poly

`IndexRange` is a small struct and should be passed by value: `const IndexRange poly`
ChrisLend marked this conversation as resolved
@ -351,0 +387,4 @@
const blender::Span<int> corner_edges,
const int current_edge_index)
{
for (int i = 0; i < poly.size(); i++) {
Member

I'm guessing this can be implemented without a for loop here if you use something like corner_edges.slice(poly).first_index(current_edge_index)

I'm guessing this can be implemented without a for loop here if you use something like `corner_edges.slice(poly).first_index(current_edge_index)`
ChrisLend marked this conversation as resolved
@ -351,0 +396,4 @@
if (i - 2 >= 0) {
return corner_edges[poly[i - 2]];
}
else {
Member

else after return

else after return
ChrisLend marked this conversation as resolved
@ -351,0 +403,4 @@
return -1;
}
/* Follow quads around the mesh by finding opposing edges. Bool return value indicates if the whole
Member

Use doxygen syntax-- suggestion:

/**
 * Follow quads around the mesh by finding opposing edges. 
 * \return True if the search has looped back on itself, finding the same face twice.
 */
Use doxygen syntax-- suggestion: ``` /** * Follow quads around the mesh by finding opposing edges. * \return True if the search has looped back on itself, finding the same face twice. */ ```
ChrisLend marked this conversation as resolved
@ -351,0 +407,4 @@
* loop has been traced. */
static bool follow_face_loop(const int poly_start_index,
const int edge_start_index,
const blender::offset_indices::OffsetIndices<int> polys,
Member

offset_indices::OffsetIndices -> OffsetIndices

`offset_indices::OffsetIndices` -> `OffsetIndices`
ChrisLend marked this conversation as resolved
@ -351,0 +408,4 @@
static bool follow_face_loop(const int poly_start_index,
const int edge_start_index,
const blender::offset_indices::OffsetIndices<int> polys,
const blender::VArray<bool> hide_poly,
Member

Pass virtual arrays by const reference

Pass virtual arrays by const reference
ChrisLend marked this conversation as resolved
@ -351,0 +410,4 @@
const blender::offset_indices::OffsetIndices<int> polys,
const blender::VArray<bool> hide_poly,
const blender::Span<int> corner_edges,
blender::Vector<int> &r_loop_polys,
Member

Return arguments should come last

Return arguments should come last
ChrisLend marked this conversation as resolved
@ -351,0 +411,4 @@
const blender::VArray<bool> hide_poly,
const blender::Span<int> corner_edges,
blender::Vector<int> &r_loop_polys,
const blender::Array<blender::Vector<int, 2>> edge_to_poly_map)
Member

Passing this array by value means the entire contents are copied, watch out for that! Passing it as Span<Vector<>> will fix that

Passing this array by value means the entire contents are copied, watch out for that! Passing it as `Span<Vector<>>` will fix that
Author
Member

how is that with the new GroupedSpan
does that need to be passed as reference?

how is that with the new `GroupedSpan` does that need to be passed as reference?
@ -351,0 +419,4 @@
while (current_edge_index > 0) {
int next_poly_index;
bool found_poly = false;
Member

found_poly can be removed by initializing next_poly_index to -1 and checking that below instead of !found_poly

`found_poly` can be removed by initializing `next_poly_index` to -1 and checking that below instead of `!found_poly`
ChrisLend marked this conversation as resolved
@ -351,0 +440,4 @@
}
/* Happens if we looped around the mesh. */
if (r_loop_polys.contains(next_poly_index)) {
Member

.contains takes linear time, which makes the runtime of this algorithm quadratic. If you need constant time lookup, better to use VectorSet

`.contains` takes linear time, which makes the runtime of this algorithm quadratic. If you need constant time lookup, better to use `VectorSet`
ChrisLend marked this conversation as resolved
@ -351,0 +473,4 @@
/* Need to use the evaluated mesh for projection to region space. */
Scene *scene_eval = DEG_get_evaluated_scene(vc.depsgraph);
Mesh *mesh_eval = mesh_get_eval_final(vc.depsgraph, scene_eval, ob_eval, &CD_MASK_BAREMESH);
Member

mesh_get_eval_final potentially reevaluates the object and generally shouldn't be used in new code. Something like BKE_object_get_evaluated_mesh should be fine here.

Generally it seems weird to use the evaluated mesh just to check if it's empty or has no faces. Is that really necessary?

`mesh_get_eval_final` potentially reevaluates the object and generally shouldn't be used in new code. Something like `BKE_object_get_evaluated_mesh` should be fine here. Generally it seems weird to use the evaluated mesh just to check if it's empty or has no faces. Is that really necessary?
Author
Member

you are right that wasn't actually needed anymore

you are right that wasn't actually needed anymore
@ -351,0 +493,4 @@
const OffsetIndices polys = mesh->polys();
const Span<int2> edges = mesh->edges();
IndexRange poly = polys[poly_pick_index];
Member

Declare variables const

Declare variables const
ChrisLend marked this conversation as resolved
@ -351,0 +509,4 @@
const VArray<bool> hide_poly = *attributes.lookup_or_default<bool>(
".hide_poly", ATTR_DOMAIN_FACE, false);
Vector<int, 2> polys_to_closest_edge = edge_to_poly_map[closest_edge_index];
Member

Vector<int, 2> polys_to_closest_edge -> const Span<int> polys_to_closest_edge

`Vector<int, 2> polys_to_closest_edge` -> `const Span<int> polys_to_closest_edge`
ChrisLend marked this conversation as resolved
@ -368,6 +368,7 @@ void PAINT_OT_face_select_all(wmOperatorType *ot);
void PAINT_OT_face_select_more(wmOperatorType *ot);
void PAINT_OT_face_select_less(wmOperatorType *ot);
void PAINT_OT_face_select_hide(wmOperatorType *ot);
void PAINT_OT_face_select_loop(struct wmOperatorType *ot);
Member

struct is unnecessary here (in C++)

`struct` is unnecessary here (in C++)
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 1 commit 2023-05-25 10:41:41 +02:00
Christoph Lendenfeld added 2 commits 2023-05-25 11:44:25 +02:00
Christoph Lendenfeld added 1 commit 2023-05-25 11:49:32 +02:00
Hans Goudey requested changes 2023-05-25 15:36:35 +02:00
@ -4481,2 +4481,4 @@
("paint.face_select_more", {"type": 'NUMPAD_PLUS', "value": 'PRESS', "ctrl": True}, None),
("paint.face_select_less", {"type": 'NUMPAD_MINUS', "value": 'PRESS', "ctrl": True}, None),
("paint.face_select_loop", {"type": "LEFTMOUSE", "value": 'PRESS', "alt": True}, None),
("paint.face_select_loop", {"type": "LEFTMOUSE", "value": 'PRESS', "alt": True, "shift": True},
Member

This doesn't work properly with right click select

This doesn't work properly with right click select
Author
Member

changed to params.select_mouse

changed to `params.select_mouse`
@ -250,3 +252,3 @@
for (const int inner_edge :
poly_edges.slice(poly_loop_index, poly_edges.size() - poly_loop_index)) {
poly_edges.slice(poly_loop_index, poly_edges.size() - poly_loop_index))
Member

Clang format does not change this for me. Maybe you have something configured differently?

Clang format does not change this for me. Maybe you have something configured differently?
Author
Member

I am using VSCode, setup as described in the wiki
https://wiki.blender.org/wiki/Developer_Intro/Environment/Portable_CMake_VSCode

I thought that the formatting style changed recently with a bump to the clang version

I am using VSCode, setup as described in the wiki https://wiki.blender.org/wiki/Developer_Intro/Environment/Portable_CMake_VSCode I thought that the formatting style changed recently with a bump to the clang version
Member

I think make format is the ground truth. Maybe worth running that before committing this anyway, just to be sure.

I think `make format` is the ground truth. Maybe worth running that before committing this anyway, just to be sure.
@ -351,0 +385,4 @@
const blender::Span<int> corner_edges,
const int current_edge_index)
{
for (int i = 0; i < poly.size(); i++) {
Member

for (const int i : poly.index_range()) {

` for (const int i : poly.index_range()) {`
Author
Member

as you mentioned the whole for loop wasn't needed if using corner_edges.slice(poly).first_index

as you mentioned the whole for loop wasn't needed if using `corner_edges.slice(poly).first_index`
@ -351,0 +449,4 @@
r_loop_polys.append(next_poly_index);
IndexRange next_poly = polys[next_poly_index];
Member

Declare variables const

Declare variables const
ChrisLend marked this conversation as resolved
@ -351,0 +471,4 @@
}
uint poly_pick_index = uint(-1);
if (!ED_mesh_pick_face(C, ob_eval, mval, ED_MESH_PICK_DEFAULT_FACE_DIST, &poly_pick_index)) {
Member

This gives you a poly index for the evaluated object, which is then used to index the original faces. Will that work if the topology is changed on the evaluated object? Worth testing IMO.

This gives you a poly index for the evaluated object, which is then used to index the original faces. Will that work if the topology is changed on the evaluated object? Worth testing IMO.
Author
Member

I tried with a decimate and subsurf modifier on top and it still works.
but after testing the un-evaluated object works as well here, so for safety I changed it back

I tried with a decimate and subsurf modifier on top and it still works. but after testing the un-evaluated object works as well here, so for safety I changed it back
ChrisLend marked this conversation as resolved
@ -351,0 +496,4 @@
Array<int> edge_to_loop_offsets;
Array<int> edge_to_loop_indices;
const GroupedSpan<int> edge_to_loop_map = bke::mesh::build_edge_to_loop_map(
Member

This topology map is unused

This topology map is unused
ChrisLend marked this conversation as resolved
@ -351,0 +528,4 @@
bke::SpanAttributeWriter<bool> select_poly = attributes.lookup_or_add_for_write_span<bool>(
".select_poly", ATTR_DOMAIN_FACE);
for (const int poly_index : polys_to_select) {
Member

select_poly.span.fill_indices(polys_to_select.as_span(), select);

` select_poly.span.fill_indices(polys_to_select.as_span(), select);`
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 5 commits 2023-05-26 10:02:03 +02:00
Christoph Lendenfeld added 1 commit 2023-05-26 10:36:22 +02:00
Christoph Lendenfeld added 1 commit 2023-05-26 10:40:18 +02:00
Christoph Lendenfeld added 1 commit 2023-05-26 10:48:07 +02:00
Christoph Lendenfeld requested review from Hans Goudey 2023-05-26 14:58:51 +02:00
Hans Goudey reviewed 2023-05-29 02:54:58 +02:00
@ -351,0 +381,4 @@
return closest_edge_index;
}
static int get_opposing_edge_index(blender::IndexRange poly,
Member

blender::IndexRange poly -> const blender::IndexRange poly

`blender::IndexRange poly` -> `const blender::IndexRange poly`
@ -351,0 +385,4 @@
const blender::Span<int> corner_edges,
const int current_edge_index)
{
const int poly_index = corner_edges.slice(poly).first_index(current_edge_index);
Member

How about index_in_poly? This name confused me at first since it's not really the index of a poly

How about `index_in_poly`? This name confused me at first since it's not really the index of a poly
@ -351,0 +410,4 @@
int current_poly_index = poly_start_index;
int current_edge_index = edge_start_index;
VectorSet<int> vector_set;
Member

Could r_loop_polys be a VectorSet so it doesn't have to be duplicated here?

Could `r_loop_polys` be a `VectorSet` so it doesn't have to be duplicated here?
Author
Member

yes, good point

yes, good point
Christoph Lendenfeld added 3 commits 2023-06-01 10:57:30 +02:00
Hans Goudey approved these changes 2023-06-01 12:27:38 +02:00
Christoph Lendenfeld added 1 commit 2023-06-01 14:56:28 +02:00
Christoph Lendenfeld merged commit 98d48c16a3 into main 2023-06-01 14:59:03 +02:00
Christoph Lendenfeld deleted branch paint_loop_face 2023-06-01 14:59:04 +02:00
Christoph Lendenfeld removed this from the Animation & Rigging project 2023-10-31 14:29:48 +01:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107653
No description provided.