This implements a Face Set Extract operator, which is similar to mask
extract. This operator uses a picker to select and Face Set in the mesh
and extract the geometry directly to a new object.
Details
Details
- Reviewers
Sergey Sharybin (sergey) - Commits
- rBafb43b881cc3: Sculpt: Face Set Extract Operator
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- extract-face-set (branched from master)
- Build Status
Buildable 9578 Build 9578: arc lint + arc unit
Event Timeline
Comment Actions
This is not usable from the current UI because the active face set depends on the cursor position and this operator is only found in a menu.
What is in Blender is considered used by artists, even if it's not in default keymap. Operator could be exposed to a menu, could be assigned to a custom keymap.
Exposing option which is knowingly broken is not a very good decision.
In a later patch I will make this operator a tool with all options exposed to be able to click on a Face Set and extract it.
Why not to delive working operator to begin with? Could either be a tool which makes you click first, could be a separate operator added to a shortcut which will fill in mouse position in its invoke.
Comment Actions
Separate operator for extracting a Face Set
Is the operator considered usable by artists now?
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 342 | Such things should really be handled by a modal keymap rather than being hardcoded. | |
| 356–358 | If you split up paint_mask_extract_exec in a way that paint_mask_extract_exec() gets properties from the operator and passes them to, say, paint_mask_extract_apply as function arguments, you wouldn't need to go into such string-hardcoded-arguments-assignment (which is fragile, proone to errors, verbose and so on). | |
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 342 | Is there a simple operator like this that uses a modal keymap to use as a reference? This code is copied from the sample_detail_size operator. If not using modal keymaps for this is a problem, then I should probably create a separate tasks to use them in all sculpt operators, because right now all of them are coded this way and they have a lot more key combinations hardcoded. | |
Comment Actions
How about using modal keymap?
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 395–403 | What is the purpose of this? Is it something what is used somewhere else? | |
Comment Actions
Would it be possible to adjust how this feature works to extract multiple face sets at the same time?
I was creating a hard surface sci fi character with this feature the other day and I used the face sets smoothing and mask extract feature to great effect to create individual panels of the character's body panels, but I had to do each one individually and it was quite time consuming.
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 395–403 | I'm not sure, this part was copied from here https://developer.blender.org/diffusion/B/browse/master/source/blender/editors/sculpt_paint/sculpt_detail.c$241 When I need the cursor position in the region in other functions I usually use CTX_wm_region directly. | |
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 395–403 | This is weird. And don't think we should be copying weird-looking code without understanding why exactly its needed. @Julian Eisel (Severin), maybe you can share some clues here? | |
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 395–403 | This is a modal operator. I think the event system doesn't update the context region while the modal runs, it keeps the one active that invoked the modal. | |
Comment Actions
Moving some inlined discussion to comment, because easier.
Is there a simple operator like this that uses a modal keymap to use as a reference?
Ugh, missed that.
I would think so, yes. Otherwise such operators are always in a way of creating custom keymap. It might even conflict with the mentality of the LMB/RMB default mouse select.
Again, something where having thoughts from @Julian Eisel (Severin) would not hurt at all.
This is a modal operator. I think the event system doesn't update the context region while the modal runs, it keeps the one active that invoked the modal.
So to me it looks like the code ensures that the mouse is clicking within any viewport region. Doing it this way seems reasonable, although a comment should explain it (or maybe a wrapper function?).
Moving this to an utility function seems more reasonable though ;)
Can we have one? Is it generic enough to justify its existence?
Comment Actions
I think if operators only hardcode the RMB/LMB interaction, and if it's done in a way that is compatible with both RCS and LCS, then it's fine I think. Customizability can be overdone too.
An example of a modal operator keymap is knifetool_modal_keymap().
Comment Actions
I see, that operator a lot more options in the modal keymap, so it makes sense. Probably for this one it should be ok to leave it like this, I don't think any user would like to change the key used to click on a face set with an eyedropper.
Comment Actions
@Julian Eisel (Severin) can I leave final word about keymap and the BKE_screen_find_region_by_type_and_xy to you?
| source/blender/blenkernel/BKE_screen.h | ||
|---|---|---|
| 402–405 ↗ | (On Diff #27993) | Don't think it's specific to events only. So suggest call it x and y. |
| source/blender/blenkernel/intern/screen.c | ||
| 1059–1062 ↗ | (On Diff #27993) | Use early output instead of ternary operator. |
Comment Actions
If the mentioned points are addressed, things seem fine to me.
| source/blender/blenkernel/intern/screen.c | ||
|---|---|---|
| 1054–1055 ↗ | (On Diff #27993) | It's not clear that this function is about the main (WINDOW) region, either use main_region in the function name or allow passing the region type as argument. |
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
| 396–397 | I'd still suggest adding a comment here explaining why this is done the way it is. I don't see a nice way to encode that in the function name itself (except maybe ED_view3d_find_main_region_at_xy(), but that seems a bit overkill and doesn't explain why it's not using CTX_wm_region() either). | |
| source/blender/blenkernel/intern/screen.c | ||
|---|---|---|
| 1054–1055 ↗ | (On Diff #27993) | Is the "main" a common refer to "RGN_TYPE_WINDOW" region in Blender? |
Comment Actions
@Sergey Sharybin (sergey) @Julian Eisel (Severin) Is this OK to commit? I would like to fix the multires mask/face set extraction on top of this patch.
Comment Actions
Well, taking into account the current state of Blender code, there is some minor feedback from me.
However, this is not how abstraction of code works. The proper way is to pass the logic (implemented in geometry_extract_tag_masked_faces i.e.) as a functors with own contexts (so that face set extraction has no data mixed with mask extraction) rather than an enumerator followed with a switch. Would be nice if proper patterns started to be followed ;)
| source/blender/editors/mesh/editmesh_mask_extract.c | ||
|---|---|---|
| 122–125 | Keep definition close to use. | |
| 164 | Should denote that this is a number. | |