Fix #108105: Fix operators affecting hidden geometry #119168

Mangal Kushwah · 2024-03-07T16:06:45+01:00

Mangal Kushwah commented

2024-03-07 16:06:45 +01:00

First-time contributor

Problem:
These operators in sculpt mode affect hidden geometry which is undesirable.

bpy.ops.sculpt.face_set_edit 
bpy.ops.sculpt.face_sets_init
bpy.ops.sculpt.face_sets_create(mode='SELECTION')
bpy.ops.mesh.paint_mask_extract()
bpy.ops.mesh.paint_mask_slice()

Solution:

This PR adds checks for hidden faces in these operators.

For operator bpy.ops.sculpt.face_sets_init it also modifies the way face sets indices were generated so generated indices is unique. This in needed so new initialized face sets do not get same indices as hidden face sets.
For generating unique face set index i have created a set container which stores hidden face sets indices. Before assigning indices to a face set it checks if it is already in set container, if it is then it'll keep increasing index by 1 until it is not in set container.

Modifying Operator bpy.ops.mesh.paint_mask_slice() is little complex, it will not affect hidden geometry only when fill holes option is unchecked, because currently filling hole code does not take hidden geometry into account and because of this in some cases hidden part of geometry can be trapped inside mesh.

Problem: These operators in sculpt mode affect hidden geometry which is undesirable. ``` bpy.ops.sculpt.face_set_edit bpy.ops.sculpt.face_sets_init bpy.ops.sculpt.face_sets_create(mode='SELECTION') bpy.ops.mesh.paint_mask_extract() bpy.ops.mesh.paint_mask_slice() ``` Solution: This PR adds checks for hidden faces in these operators. For operator `bpy.ops.sculpt.face_sets_init` it also modifies the way face sets indices were generated so generated indices is unique. This in needed so new initialized face sets do not get same indices as hidden face sets. For generating unique face set index i have created a set container which stores hidden face sets indices. Before assigning indices to a face set it checks if it is already in set container, if it is then it'll keep increasing index by 1 until it is not in set container. Modifying Operator `bpy.ops.mesh.paint_mask_slice()` is little complex, it will not affect hidden geometry only when fill holes option is unchecked, because currently filling hole code does not take hidden geometry into account and because of this in some cases hidden part of geometry can be trapped inside mesh.

Mangal Kushwah added 1 commit 2024-03-07 16:07:01 +01:00

82c531af36 Disable "Modifiy Hidden" property by default for Grow/Shrink, Fair or Delete face sets

Mangal Kushwah changed title from ~~WIP Fix #108105: Fix operators that affect hidden geometry~~ to WIP: Fix #108105: Fix operators that affect hidden geometry

2024-03-07 16:08:23 +01:00

Iliya Katushenock added this to the Sculpt, Paint & Texture project 2024-03-07 16:09:29 +01:00

Mangal Kushwah added 1 commit 2024-03-28 11:39:18 +01:00

475a313561 Ensure sculpt_face_sets_init_flood_fill() doesn't alter hidden face sets

This commit modify the sculpt_face_sets_init_flood_fill() so it doesn't
modify hidden geometry.

Mangal Kushwah commented

2024-03-28 11:53:20 +01:00

First-time contributor

With this commit bpy.ops.sculpt.face_sets_init operations will not modify the hidden face sets, except Initialize Face Sets by Material.

I'm confused for Face Sets by Materials would it be better if it can modify hidden geometry?

With this commit `bpy.ops.sculpt.face_sets_init` operations will not modify the hidden face sets, except Initialize Face Sets by **Material**. I'm confused for Face Sets by Materials would it be better if it can modify hidden geometry?

Mangal Kushwah commented

2024-03-28 12:02:11 +01:00

First-time contributor

I'm confused for Face Sets by Materials would it be better if it can modify hidden geometry?

@HooglyBoogly @Sergey @JulienKaspar.

> I'm confused for Face Sets by Materials would it be better if it can modify hidden geometry? @HooglyBoogly @Sergey @JulienKaspar.

Sergey Sharybin commented

2024-03-28 16:34:49 +01:00

Not sure why material case is special here. From the perspective of operators not modifying invisible data, it will just initialize face sets based on material slots for the visible geometry.

👍 1

Hans Goudey commented

2024-03-28 21:01:27 +01:00

I'm confused for Face Sets by Materials would it be better if it can modify hidden geometry?

They should just follow the operator "Modify Hidden" option. And if there is no such option for an operator, hidden geometry should generally not be modified.

> I'm confused for Face Sets by Materials would it be better if it can modify hidden geometry? They should just follow the operator "Modify Hidden" option. And if there is no such option for an operator, hidden geometry should generally not be modified.

👍 1

Mangal Kushwah added 3 commits 2024-03-29 14:40:18 +01:00

5ee36fe1f6 Ensure Intialize Face Set by Material doesn't alter hidden face sets

This add checks for hidden faces and also modify the face_set
index generation so that the face doesn't get the same face set
index as the hidden face_set.

fd348ce083 Ensure face set from edit mode selction doesn't change hidden face sets

c79e279bd6 Ensure Mask Extract not create face for hidden faces

Mangal Kushwah added 1 commit 2024-03-29 18:40:03 +01:00

c523317439 Ensure Mask Slice operations doesn't effect hidden faces

It will work only when fill_hole is not checked

Mangal Kushwah changed title from ~~WIP: Fix #108105: Fix operators that affect hidden geometry~~ to Fix #108105: Fix operators that affect hidden geometry

2024-03-29 18:41:28 +01:00

Mangal Kushwah changed title from ~~Fix #108105: Fix operators that affect hidden geometry~~ to Fix #108105: Fix operators affecting hidden geometry

2024-03-29 18:45:42 +01:00

Mangal Kushwah changed title from ~~Fix #108105: Fix operators affecting hidden geometry~~ to WIP: Fix #108105: Fix operators affecting hidden geometry

2024-03-29 18:46:28 +01:00

Mangal Kushwah changed title from ~~WIP: Fix #108105: Fix operators affecting hidden geometry~~ to Fix #108105: Fix operators affecting hidden geometry

2024-03-29 18:46:43 +01:00

Mangal Kushwah requested review from Sergey Sharybin 2024-03-29 18:48:01 +01:00

Mangal Kushwah requested review from Hans Goudey 2024-03-29 18:48:02 +01:00

Sergey Sharybin reviewed 2024-04-02 15:49:00 +02:00

Sergey Sharybin left a comment

Just some quick notes. Did not have time yet to verify the behavior on algorithmical level.

👍 1

source/blender/editors/mesh/editmesh_mask_extract.cc

						
				@ -246,1 +246,4 @@

				    BMIter face_iter;

				    if (BM_elem_flag_test_bool(f, BM_ELEM_HIDDEN)) {

				      keep_face = false;

				      BM_elem_flag_set(f, BM_ELEM_TAG, !keep_face);

Sergey Sharybin commented

2024-04-02 15:23:25 +02:00

This can just be BM_elem_flag_set(f, BM_ELEM_TAG, true); ?

This can just be `BM_elem_flag_set(f, BM_ELEM_TAG, true);` ?

Mangal Kushwah commented

2024-04-18 15:16:50 +02:00

First-time contributor

Done

Mangal-Kushwah marked this conversation as resolved

source/blender/editors/mesh/editmesh_mask_extract.cc

						
				@ -418,6 +423,9 @@ static void slice_paint_mask(BMesh *bm, bool invert, bool fill_holes, float mask

				        break;

				      }

				    }

				    if (!fill_holes && BM_elem_flag_test_bool(f, BM_ELEM_HIDDEN)) {

Sergey Sharybin commented

2024-04-02 15:25:36 +02:00

Why is the visibility only checked when !fill_holes ?

Why is the visibility only checked when `!fill_holes` ?

Mangal Kushwah commented

2024-04-02 16:30:50 +02:00

First-time contributor

Currently filling hole code does not take hidden geometry into account and because of this in some cases hidden part of geometry can be trapped inside mesh which i assumed is bad thing not entirely sure though.

Mangal Kushwah commented

2024-04-26 12:23:16 +02:00

First-time contributor

Removed it

Mangal-Kushwah marked this conversation as resolved

source/blender/editors/mesh/editmesh_mask_extract.cc

						
				@ -421,2 +427,4 @@

				      keep_face = false;

				    };

				    if (invert) {

				      keep_face = !keep_face;

Sergey Sharybin commented

2024-04-02 15:45:20 +02:00

This invert behavior is a bit fragile, as it potentially marks faces which are hidden. From reading the code it is seems fine from the current usages of this function. However, it worth:

Double-checking that Paint Mask Slice with hidden faces behaves correctly.
Adding a comment about it.

There might be some smarter and less error-prone we can do here, but I do not have quick concrete proposals.

This `invert` behavior is a bit fragile, as it potentially marks faces which are hidden. From reading the code it is seems fine from the current usages of this function. However, it worth: - Double-checking that `Paint Mask Slice` with hidden faces behaves correctly. - Adding a comment about it. There might be some smarter and less error-prone we can do here, but I do not have quick concrete proposals.

Mangal Kushwah commented

2024-04-02 17:41:51 +02:00

First-time contributor

Will look into it tomorrow

Mangal Kushwah commented

2024-04-26 12:22:07 +02:00

First-time contributor

Added the comment about invert

Mangal-Kushwah marked this conversation as resolved

source/blender/editors/sculpt_paint/sculpt_face_set.cc Outdated

						
				@ -834,0 +862,4 @@

				          ".hide_poly", bke::AttrDomain::Face, false);

				      const Array<int> prev_face_sets = duplicate_face_sets(*mesh);

				      Set<int> hidden_face_sets;

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

Sergey Sharybin commented

2024-04-02 15:48:11 +02:00

Perhaps it will be more readable if the code is moved to gather_hidden_face_sets (or some similar name)? Currently the code seems to be duplicated between multiple operators.

Perhaps it will be more readable if the code is moved to `gather_hidden_face_sets` (or some similar name)? Currently the code seems to be duplicated between multiple operators.

Mangal Kushwah commented

2024-04-02 16:44:04 +02:00

First-time contributor

Ok, Should I make it static?

Mangal-Kushwah marked this conversation as resolved

Mangal Kushwah added 2 commits 2024-04-03 17:57:16 +02:00

917ae1c07d Merge branch 'main' into operators-hidden-geometry

05b3797bc3 Refactor: Implement function to gather hidden face sets

Mangal Kushwah commented

2024-04-06 17:11:30 +02:00

First-time contributor

Why is the visibility only checked when !fill_holes ?

@Sergey I disabled visibility checks for fill holes because of this (check the video).

I have some question:

Is this a bad thing?
If yes, do you have any suggestions about how I should proceed?

Currently I'm thinking about modifying function responsible for filling holes so it takes hidden geometry into account.

> Why is the visibility only checked when `!fill_holes` ? @Sergey I disabled visibility checks for fill holes because of this (check the video). I have some question: - Is this a bad thing? - If yes, do you have any suggestions about how I should proceed? Currently I'm thinking about modifying function responsible for filling holes so it takes hidden geometry into account.

Screencast from 06-04-24 07:57:24 PM IST.webm

4.6 MiB

Sergey Sharybin commented

2024-04-19 11:38:57 +02:00

It might be worth involving input from @DanielBystedt.

To me intuitively the algorithm should give the same exact results as if the hidden part of the mesh simply did not exist at the time when an operator runs, and was added later. Hope the analogy is clear :) But not sure if it has some negative implications.

It might be worth involving input from @DanielBystedt. To me intuitively the algorithm should give the same exact results as if the hidden part of the mesh simply did not exist at the time when an operator runs, and was added later. Hope the analogy is clear :) But not sure if it has some negative implications.

👍 2

Mangal Kushwah added 2 commits 2024-04-26 12:20:44 +02:00

bed4956131 Refactor: Remove redundant line in geometry_extract_tag_masked_faces

00c8a98250 Sculpt: Ensure visibility checks for mask slicing work in all cases

Daniel Bystedt commented

2024-04-28 11:36:03 +02:00

It might be worth involving input from @DanielBystedt.

To me intuitively the algorithm should give the same exact results as if the hidden part of the mesh simply did not exist at the time when an operator runs, and was added later. Hope the analogy is clear :) But not sure if it has some negative implications.

What @Sergey describes would be ideal (i.e. Hidden part of mesh is not deleted during the operation). I have included a visual "step by step" that should clarify things.

> It might be worth involving input from @DanielBystedt. > > To me intuitively the algorithm should give the same exact results as if the hidden part of the mesh simply did not exist at the time when an operator runs, and was added later. Hope the analogy is clear :) But not sure if it has some negative implications. What @Sergey describes would be ideal (i.e. Hidden part of mesh is not deleted during the operation). I have included a visual "step by step" that should clarify things. ![bild](/attachments/52578279-561a-4756-b4c5-0e9263960425)

bild.png

188 KiB

❤️ 1

Mangal Kushwah commented

2024-04-28 12:27:23 +02:00

First-time contributor

Thanks for visual explanation✨, It is working exactly like that.
I have manually tested it so far everything is working as expected.

Thanks for visual explanation✨, It is working exactly like that. I have manually tested it so far everything is working as expected.

❤️ 1

Sergey Sharybin reviewed 2024-05-01 16:25:38 +02:00

source/blender/editors/sculpt_paint/sculpt_face_set.cc Outdated

						
				@ -735,0 +742,4 @@

				      ".hide_poly", bke::AttrDomain::Face, false);

				  const Array<int> prev_face_sets = duplicate_face_sets(*mesh);

				  Set<int> hidden_face_sets = gather_hidden_face_sets(hide_poly, prev_face_sets);

Sergey Sharybin commented

2024-05-01 16:25:27 +02:00

It doesn't seem we use prev_face_sets for anything by calculating a set if face sets used by hidden faces. So I think there is a potential to optimize some things here, with an idea of making it so gather_hidden_face_sets accesses face set attribute more directly. Perhaps something like this could work?

const Set<int> hidden_face_sets = gather_hidden_face_sets(hide_poly, face_sets.span);

It doesn't seem we use `prev_face_sets ` for anything by calculating a set if face sets used by hidden faces. So I think there is a potential to optimize some things here, with an idea of making it so `gather_hidden_face_sets ` accesses face set attribute more directly. Perhaps something like this could work? ``` const Set<int> hidden_face_sets = gather_hidden_face_sets(hide_poly, face_sets.span); ```

Mangal-Kushwah marked this conversation as resolved

source/blender/editors/sculpt_paint/sculpt_face_set.cc

						
				@ -783,6 +804,18 @@ Array<int> duplicate_face_sets(const Mesh &mesh)

				  return face_sets;

				}

				Set<int> gather_hidden_face_sets(const VArray<bool> &hide_poly, const Span<int> prev_face_sets)

Sergey Sharybin commented

2024-05-01 16:14:28 +02:00

I would call it face_sets. The fact that caller might consider the span to be a "previously used face sets" does not really affect this function.

I would call it `face_sets`. The fact that caller might consider the span to be a "previously used face sets" does not really affect this function.

Mangal-Kushwah marked this conversation as resolved

Mangal Kushwah added 1 commit 2024-05-01 17:06:50 +02:00

8e1b5724cd Refactor: gather_hidden_face_sets accesses face set attribute more directly

Sergey Sharybin approved these changes 2024-05-01 17:52:58 +02:00

Sergey Sharybin left a comment

On a code side it seems correct. But I'd like Hans to also have a look, and maybe help with double-checking that it works good (I didn't have time for that yet).

❤️ 1

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u operators-hidden-geometry:Mangal-Kushwah-operators-hidden-geometry

git checkout Mangal-Kushwah-operators-hidden-geometry

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #108105: Fix operators affecting hidden geometry #119168

Checkout