Fix #125157: Sculpt multires crashes on box mask and duplicated stroke #125268

Sean Kim · 2024-07-23T02:33:06+02:00

Sean Kim commented

2024-07-23 02:33:06 +02:00

Introduced in d527e3a6bd.

The change to update PBVH normals before destroying the PBVH to fix
shading on duplicate meshes issues had the unfortunate side effect of
causing crashes for multires due to a race condition. This commit
simply avoids performing this flushing for multires to allow for
further iteration in the future instead of reverting the offending
commit entirely.

Introduced in d527e3a6bd787c2f58837c6f93ccd7d8b466c779. The change to update PBVH normals before destroying the PBVH to fix shading on duplicate meshes issues had the unfortunate side effect of causing crashes for multires due to a race condition. This commit simply avoids performing this flushing for multires to allow for further iteration in the future instead of reverting the offending commit entirely.

Sean Kim added 1 commit 2024-07-23 02:33:14 +02:00

Revert "Fix #122947 : Curve stroke on duplicate object doesn't update normals" 7cb142e61d

This reverts commit d527e3a6bd.

Sean Kim added this to the Sculpt, Paint & Texture project 2024-07-23 02:33:18 +02:00

Sean Kim requested review from Sergey Sharybin 2024-07-23 02:33:34 +02:00

Sean Kim requested review from Hans Goudey 2024-07-23 02:33:41 +02:00

Sean Kim commented

2024-07-23 02:38:17 +02:00

For more information, the unreported issue that's alluded to in the original post of this PR is likely a race condition that I missed encountering when testing the initial implementation. That crash can be reproduced in current main, hence why I'm advocating to revert this in both this branch and in the 4.2 release branch.

Also, the reason the mask tool encounters this error in 4.2 but not in current main is because prior to 90c4c48bbf (diff-7ea99a817b555b40cfc6c3601aeb8a8de5048f54) the mask gestures would tag a normal update for multires for some reason. While removing this call in the 4.2 branch fixes the problem for the mask tool, the larger multires crash with sculpting on duplicated objects still remains.

For more information, the unreported issue that's alluded to in the original post of this PR is likely a race condition that I missed encountering when testing the initial implementation. That crash can be reproduced in current main, hence why I'm advocating to revert this in both this branch and in the 4.2 release branch. Also, the reason the mask tool encounters this error in 4.2 but not in current main is because prior to https://projects.blender.org/blender/blender/commit/90c4c48bbf1fe0000d82316ea5196bac394aeefb#diff-7ea99a817b555b40cfc6c3601aeb8a8de5048f54 the mask gestures would tag a normal update for multires for some reason. While removing this call in the 4.2 branch fixes the problem for the mask tool, the larger multires crash with sculpting on duplicated objects still remains.

Sean Kim commented

2024-07-23 03:28:19 +02:00

An alternative here is to apply the following fix to simply ignore this update for multires, I'm currently undecided if this seems less risky & worth doing. It has the benefit of keeping curve stroke normals working for normal meshes:

diff --git a/source/blender/blenkernel/intern/paint.cc b/source/blender/blenkernel/intern/paint.cc
index dc6512bebd9..401fe3bc4ff 100644
--- a/source/blender/blenkernel/intern/paint.cc
+++ b/source/blender/blenkernel/intern/paint.cc
@@ -2049,7 +2049,7 @@ void BKE_sculpt_update_object_before_eval(Object *ob_eval)
 
   if (ss && ss->building_vp_handle == false) {
     if (!ss->cache && !ss->filter_cache && !ss->expand_cache) {
-      if (ss->pbvh && BKE_pbvh_type(*ss->pbvh) != PBVH_GRIDS) {
+      if (ss->pbvh) {
         /* PBVH nodes may contain dirty normal tags. To avoid losing that information when the PBVH
          * is deleted, make sure all tagged geometry normals are up to date.
          * See #122947 for more information. */

An alternative here is to apply the following fix to simply ignore this update for multires, I'm currently undecided if this seems less risky & worth doing. It has the benefit of keeping curve stroke normals working for normal meshes: ``` diff --git a/source/blender/blenkernel/intern/paint.cc b/source/blender/blenkernel/intern/paint.cc index dc6512bebd9..401fe3bc4ff 100644 --- a/source/blender/blenkernel/intern/paint.cc +++ b/source/blender/blenkernel/intern/paint.cc @@ -2049,7 +2049,7 @@ void BKE_sculpt_update_object_before_eval(Object *ob_eval) if (ss && ss->building_vp_handle == false) { if (!ss->cache && !ss->filter_cache && !ss->expand_cache) { - if (ss->pbvh && BKE_pbvh_type(*ss->pbvh) != PBVH_GRIDS) { + if (ss->pbvh) { /* PBVH nodes may contain dirty normal tags. To avoid losing that information when the PBVH * is deleted, make sure all tagged geometry normals are up to date. * See #122947 for more information. */ ```

Sergey Sharybin commented

2024-07-23 13:06:36 +02:00

I think it will be fine to go with your suggestion about BKE_pbvh_type(*ss->pbvh) != PBVH_GRIDS. Just add a comment with a reference to the report/PR, to make it easier to re-iterate in the future.

I think it will be fine to go with your suggestion about `BKE_pbvh_type(*ss->pbvh) != PBVH_GRIDS`. Just add a comment with a reference to the report/PR, to make it easier to re-iterate in the future.

Sean Kim added 2 commits 2024-07-23 16:05:26 +02:00

Reapply "Fix #122947 : Curve stroke on duplicate object doesn't update normals" 54a0784311

This reverts commit 7cb142e61d.

Option 2 b897c53e1f

Sean Kim added 1 commit 2024-07-23 16:10:33 +02:00

Add comment ad2b0224e7

Sean Kim changed title from ~~Revert "Fix #122947: Curve stroke on duplicate object doesn't update normals"~~ to Fix #125157: Sculpt multires crashes on box mask and duplicated stroke

2024-07-23 16:12:27 +02:00

Sergey Sharybin approved these changes 2024-07-23 16:23:44 +02:00

source/blender/blenkernel/intern/paint.cc Outdated

						
				@ -2052,1 +2052,3 @@

				      if (ss->pbvh) {

				      /* Avoid performing the following normal update for Multires, as it causes race conditions

				       * and other intermittent crashes with shared meshes.

				       * See !125268 and #125157 for more information.  */

Sergey Sharybin commented

2024-07-23 16:23:35 +02:00

. */ -> . */

`. */` -> `. */`

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-07-23 16:43:51 +02:00

Remove extra space e198530304

Hans Goudey approved these changes 2024-07-23 22:16:06 +02:00

Hans Goudey left a comment

Yes, just reverting the change for multires makes sense to me.

Sean Kim added 2 commits 2024-07-23 22:51:00 +02:00

Merge branch 'main' of projects.blender.org:blender/blender into fix-125157 530c592b74

Adjust after merge 6d4bbf3996

Sean Kim merged commit 338140f1e6 into main

2024-07-23 22:56:45 +02:00

Sean Kim deleted branch fix-125157

2024-07-23 22:56:48 +02:00

Sean Kim referenced this issue from a commit

2024-07-23 22:56:51 +02:00

Fix #125157: Sculpt multires crashes on box mask and duplicated stroke

Roman commented

2024-07-24 01:37:03 +02:00

First-time contributor

4.2 building errors with: if (ss->pbvh && ss->pbvh->type() != blender::bke::pbvh::Type::Grids) {

but: if (ss->pbvh && BKE_pbvh_type(*ss->pbvh) != PBVH_GRIDS) { compiled fine

<img width="920" alt="image" src="attachments/01d8b596-234d-4d8e-abd7-f67ec87fa94d"> 4.2 building errors with: if (ss->pbvh && ss->pbvh->type() != blender::bke::pbvh::Type::Grids) { <img width="893" alt="image" src="attachments/383499c8-9e25-4489-95ab-64888b69ac10"> but: if (ss->pbvh && BKE_pbvh_type(*ss->pbvh) != PBVH_GRIDS) { compiled fine <img width="917" alt="image" src="attachments/d79b5aee-e0ed-4e9e-8ce9-3a5be947f82c">

image.png

77 KiB

image.png

49 KiB

image.png

86 KiB

Roman commented

2024-07-24 01:38:31 +02:00

First-time contributor

Refactored in 4.3 with this commit: 7daefd730b

Refactored in 4.3 with this commit: https://projects.blender.org/blender/blender/commit/7daefd730b2c16c52f27bf9d5cf8adf17e4de41f

~~Sean Kim referenced this pull request 2024-07-24 01:44:57 +02:00~~