Fix #95419: Sculpt: Invert visible hides all faces with Multires modifier #121929

Raul Fernandez Hernandez · 2024-05-17T23:51:19+02:00

2024-05-17 23:51:19 +02:00

Fix for sculpt mode: invert visible hides all faces with Multires modifier #95419

Grids face indices should not change on the fly based on hidden state.
It caused the rendering glitches shown on the original bug report and the attached recordings.

this PR removes the unnecessary check and dependency of grids visibility with the smooth/sharp of faces.
replaces smooth flag for sharp flag which better express the intent and simplifies the logic.

Fix for sculpt mode: invert visible hides all faces with Multires modifier #95419 Grids face indices should not change on the fly based on hidden state. It caused the rendering glitches shown on the original bug report and the attached recordings. * this PR removes the unnecessary check and dependency of grids visibility with the smooth/sharp of faces. * replaces smooth flag for sharp flag which better express the intent and simplifies the logic.

After-2 hide fix.mkv

844 KiB

After-1 hide fix.mkv

1.4 MiB

Before-1 hide fix.mkv

1.3 MiB

Before-2 hide fix.mkv

894 KiB

Raul Fernandez Hernandez added 4 commits 2024-05-17 23:51:27 +02:00

f0c7cbd9ff possible fix

944389fbfe Revert "possible fix"

This reverts commit f0c7cbd9ff.

b865d31260 possible fix

3268e3d3b1 possible fix

Raul Fernandez Hernandez requested review from Sergey Sharybin 2024-05-17 23:52:04 +02:00

Raul Fernandez Hernandez requested review from Hans Goudey 2024-05-17 23:52:04 +02:00

Raul Fernandez Hernandez added this to the Sculpt, Paint & Texture project 2024-05-17 23:52:19 +02:00

Raul Fernandez Hernandez requested review from Pratik Borhade 2024-05-17 23:58:18 +02:00

Sean Kim commented

2024-05-18 01:01:38 +02:00

Just a nit, PR title format for this would probably be better as something like

Fix #95419: Avoid changing vert index on multires visibility

Just a nit, PR title format for this would probably be better as something like ``` Fix #95419: Avoid changing vert index on multires visibility ```

👍 1

Raul Fernandez Hernandez changed title from ~~Fix sculpt mode: invert visible hides all faces with Multires modifier #95419~~ to Fix #95419: Avoid changing vert index on multires visibility

2024-05-18 02:52:33 +02:00

Hans Goudey reviewed 2024-05-20 01:40:31 +02:00

Hans Goudey left a comment

Since the PR mostly moves code from one place to another it's hard to see where the actual logic change is. Do you think you could structure the final code so the diff is simpler? Or commit a cleanup so that the logic fix is a smaller diff?

Raul Fernandez Hernandez commented

2024-05-20 17:01:48 +02:00

Since the PR mostly moves code from one place to another it's hard to see where the actual logic change is. Do you think you could structure the final code so the diff is simpler? Or commit a cleanup so that the logic fix is a smaller diff?

Disagree, If small PR like this have issues with comprehension then PR's like this https://projects.blender.org/blender/blender/pulls/121835/files are incomprehensible and hence never-to-be-approved?

> Since the PR mostly moves code from one place to another it's hard to see where the actual logic change is. Do you think you could structure the final code so the diff is simpler? Or commit a cleanup so that the logic fix is a smaller diff? Disagree, If small PR like this have issues with comprehension then PR's like this https://projects.blender.org/blender/blender/pulls/121835/files are incomprehensible and hence never-to-be-approved?

Hans Goudey commented

2024-05-20 17:06:58 +02:00

Separating cleanups and logic fixes is the most basic thing we can do for each other to make the code review process easier. This PR is a clear example of where that would help because the logic fix is relatively small and the cleanups are listed as separate changes in the description. This is not just for reviewers but future developers who have to make sense of the git history.

👍 2

Raul Fernandez Hernandez added 1 commit 2024-05-20 21:33:15 +02:00

22dc8b6729 bikeshedding requested commit

Raul Fernandez Hernandez commented

2024-05-20 21:37:41 +02:00

Resolved

Raul Fernandez Hernandez referenced this pull request

2024-05-20 22:06:32 +02:00

Refactor: Split create_index_grids() into 2 separate functions #122020

Raul Fernandez Hernandez added 1 commit 2024-05-21 18:18:58 +02:00

cf28acd367 space

Raul Fernandez Hernandez added 1 commit 2024-05-21 18:23:48 +02:00

345c3da0ad space

Raul Fernandez Hernandez requested review from Hans Goudey 2024-05-21 18:24:41 +02:00

Raul Fernandez Hernandez referenced this issue from a commit

2024-05-22 18:02:13 +02:00

Refactor: Split create_index_grids() into 2 separate functions

Sergey Sharybin commented

2024-05-23 10:09:18 +02:00

For the title of the PR, follow the commit message guidelines. It should be explaining what is the user-level break. The details about vertex indices can go into the fuller description.

I am not sure if i can do proper review here, as I don't seem to be able to reproduce the issue. From reading the code I can not really understand why do we need to follow different ways (tris vs. quads) depending on the hidden state. Both of the tris and quads code paths seems to do hidden check, so indeed the extra logic can be removed.

For the title of the PR, follow the [commit message guidelines](https://developer.blender.org/docs/handbook/guidelines/commit_messages/#bug-fixes). It should be explaining what is the user-level break. The details about vertex indices can go into the fuller description. I am not sure if i can do proper review here, as I don't seem to be able to reproduce the issue. From reading the code I can not really understand why do we need to follow different ways (tris vs. quads) depending on the hidden state. Both of the tris and quads code paths seems to do hidden check, so indeed the extra logic can be removed.

👍 1

Sergey Sharybin reviewed 2024-05-23 10:10:31 +02:00

source/blender/draw/intern/draw_pbvh.cc Outdated

						
				@ -1183,2 +1168,2 @@

				      if (!smooth) {

				      bool sharp = (!sharp_faces.is_empty() && sharp_faces[grid_to_face_map[grid_index]]);

				      if (sharp) {

Sergey Sharybin commented

2024-05-23 10:10:14 +02:00

I'd do it as if (!sharp_faces.is_empty() && sharp_faces[grid_to_face_map[grid_index]]). No need to have an extra variable like that.

I'd do it as `if (!sharp_faces.is_empty() && sharp_faces[grid_to_face_map[grid_index]])`. No need to have an extra variable like that.

👍 1

Iliya Katushenock commented

2024-05-23 12:01:39 +02:00

Even better:

if (is_empty()){
  continue;
}
if (!sharp_faces[grid_to_face_map[grid_index]]) {
  continue;
}
needs_tri_index = false;
break;

Even better: ```Cpp if (is_empty()){ continue; } if (!sharp_faces[grid_to_face_map[grid_index]]) { continue; } needs_tri_index = false; break; ```

Sergey Sharybin commented

2024-05-23 12:08:15 +02:00

I don't think this is necessarily better. It'll bite you back when/if there will be another condition under which the needs_tri_index would need to be disabled.

I don't think this is necessarily better. It'll bite you back when/if there will be another condition under which the `needs_tri_index` would need to be disabled.

👍 1

farsthary marked this conversation as resolved

Sean Kim commented

2024-05-23 11:12:01 +02:00

For the title of the PR, follow the commit message guidelines. It should be explaining what is the user-level break. The details about vertex indices can go into the fuller description.

Whoops, my mistake on the recommendation for the commit message - I forgot that the title should still be the user-facing impact. Apologies for the added confusion

> For the title of the PR, follow the [commit message guidelines](https://developer.blender.org/docs/handbook/guidelines/commit_messages/#bug-fixes). It should be explaining what is the user-level break. The details about vertex indices can go into the fuller description. Whoops, my mistake on the recommendation for the commit message - I forgot that the title should still be the user-facing impact. Apologies for the added confusion

Raul Fernandez Hernandez changed title from ~~Fix #95419: Avoid changing vert index on multires visibility~~ to Fix #95419: sculpt mode: invert visible hides all faces with Multires modifier

2024-05-27 17:24:22 +02:00

Raul Fernandez Hernandez added 1 commit 2024-05-27 17:43:06 +02:00

0ad476c92e feedback

Sergey Sharybin approved these changes 2024-05-28 10:39:50 +02:00

Sergey Sharybin left a comment

Think i've managed to reproduce the issue on macOS.
Can confirm the issue demonstarted by Raul, and the original issue is fixed with this PR.

Think i've managed to reproduce the issue on macOS. Can confirm the issue demonstarted by Raul, and the original issue is fixed with this PR.

👍 1

Hans Goudey approved these changes 2024-05-28 19:01:13 +02:00

Hans Goudey changed title from ~~Fix #95419: sculpt mode: invert visible hides all faces with Multires modifier~~ to Fix #95419: Sculpt: Invert visible hides all faces with Multires modifier

2024-05-28 19:01:47 +02:00

Raul Fernandez Hernandez merged commit 8182ebd4d2 into main

2024-05-29 06:03:07 +02:00

Raul Fernandez Hernandez deleted branch grids_visibility_fix

2024-05-29 06:03:09 +02:00

Raul Fernandez Hernandez referenced this issue from a commit

2024-05-29 06:03:11 +02:00

Fix #95419: Sculpt: Invert visible hides all faces with Multires modifier

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 #95419: Sculpt: Invert visible hides all faces with Multires modifier #121929