PBVH: update mesh data pointers stored in pbvh #106271

Jacques Lucke · 2023-03-29T18:36:59+02:00

Jacques Lucke commented

2023-03-29 18:36:59 +02:00

This is necessary for #106228. Without this, PBVH might contain pointers to data that either does not exist anymore or that must not be edited (because it is shared).

The file below shows a simple way to reproduce the issue. When only #106228 is applied, weight painting does not work correctly and also crashes in an ASAN build.

ASAN report: https://hastebin.com/share/goziwepawi.yaml

This is necessary for #106228. Without this, `PBVH` might contain pointers to data that either does not exist anymore or that must not be edited (because it is shared). The file below shows a simple way to reproduce the issue. When only #106228 is applied, weight painting does not work correctly and also crashes in an ASAN build. ASAN report: https://hastebin.com/share/goziwepawi.yaml

weight_paint_bug.blend

833 KiB

Jacques Lucke added 5 commits 2023-03-29 18:37:10 +02:00

685a1cde14 initial commit

1328866ea5 cleanup

71e1654fb9 cleanup

b7e54e33b5 update mesh pointers

ee06d21f50 fix

Jacques Lucke added 2 commits 2023-04-07 13:05:22 +02:00

56eaab98f4 Merge branch 'main' into pbvh-update-pointers

c53e82af66 fix

Jacques Lucke added 1 commit 2023-04-07 13:15:08 +02:00

2e59f29854 cleanup

Jacques Lucke added 1 commit 2023-04-07 13:17:49 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

0feb21e8df cleanup

Jacques Lucke changed title from ~~WIP: Sculpt: update mesh data pointers stored in pbvh~~ to PBVH: update mesh data pointers stored in pbvh

2023-04-07 13:22:57 +02:00

Jacques Lucke commented

2023-04-07 13:23:05 +02:00

@blender-bot build

Jacques Lucke requested review from Hans Goudey 2023-04-07 13:23:59 +02:00

Jacques Lucke requested review from Joseph Eagar 2023-04-07 13:24:35 +02:00

~~Jacques Lucke referenced this pull request 2023-04-07 13:26:21 +02:00~~

Custom Data: support implicit sharing for custom data layers #106228

Hans Goudey approved these changes 2023-04-07 23:03:17 +02:00

Hans Goudey left a comment

I left some comments about potential improvements inline, but on the whole I think this makes sense as one step on a path to a saner ownership model here. It looks good to me.

source/blender/blenkernel/intern/paint.cc

						
				@ -2204,3 +2183,4 @@

				  BKE_pbvh_build_mesh(pbvh, me);

				  const bool is_deformed = check_sculpt_object_deformed(ob, true);

				  if (is_deformed && me_eval_deform != nullptr) {

Hans Goudey commented

2023-04-07 22:55:02 +02:00

I wonder if it would make sense to check this before BKE_pbvh_build_mesh and give BKE_pbvh_build_mesh an optional argument for positions not owned by the mesh. That would go a long way to help clarify whether PBVH should reuse the arrays for the mesh's looptris and normals too, which is the biggest remaining pain point in this area IMO.

Something similar would apply to sculpt_update_object, where the chunks of code currently calling BKE_pbvh_vert_coords_apply could be moved to happen before BKE_sculpt_object_pbvh_ensure (which would also get a new non-mesh positions argument).

Not sure if you'd consider this related (feel free to disregard it). If not, I'll probably look into this in the near future.

I wonder if it would make sense to check this before `BKE_pbvh_build_mesh` and give `BKE_pbvh_build_mesh` an optional argument for positions not owned by the mesh. That would go a long way to help clarify whether `PBVH` should reuse the arrays for the mesh's looptris and normals too, which is the biggest remaining pain point in this area IMO. Something similar would apply to `sculpt_update_object`, where the chunks of code currently calling `BKE_pbvh_vert_coords_apply` could be moved to happen before `BKE_sculpt_object_pbvh_ensure` (which would also get a new non-mesh positions argument). Not sure if you'd consider this related (feel free to disregard it). If not, I'll probably look into this in the near future.

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

						
				@ -840,0 +834,4 @@

				  pbvh->polys = mesh->polys();

				  pbvh->corner_verts = mesh->corner_verts().data();

				  if (!pbvh->deformed) {

				    /* If it was deformed, a new array is allocated. */

Hans Goudey commented

2023-04-07 22:58:13 +02:00

This comment is a bit confusing. I think a more correct phrasing would be Deformed positions not matching the original mesh are owned directly by the PBVH, and are set separately by #BKE_pbvh_vert_coords_apply (though I do suggest tweaking that a bit below).

This comment is a bit confusing. I think a more correct phrasing would be `Deformed positions not matching the original mesh are owned directly by the PBVH, and are set separately by #BKE_pbvh_vert_coords_apply` (though I do suggest tweaking that a bit below).

JacquesLucke marked this conversation as resolved

Jacques Lucke reviewed 2023-04-08 12:48:53 +02:00

source/blender/blenkernel/intern/paint.cc

						
				@ -2204,3 +2183,4 @@

				  BKE_pbvh_build_mesh(pbvh, me);

				  const bool is_deformed = check_sculpt_object_deformed(ob, true);

				  if (is_deformed && me_eval_deform != nullptr) {

Jacques Lucke commented

2023-04-08 12:47:32 +02:00

Yeah that would probably improve the code even more, but won't integrate it into this patch.

Jacques Lucke added 2 commits 2023-04-08 12:49:12 +02:00

0df81b5ff8 Merge branch 'main' into pbvh-update-pointers

88e780afb3 improve comment

Jacques Lucke merged commit cde565c26c into main

2023-04-08 13:29:49 +02:00

Jacques Lucke referenced this issue from a commit

2023-04-08 13:29:49 +02:00

PBVH: update mesh data pointers stored in pbvh

Jacques Lucke deleted branch pbvh-update-pointers

2023-04-08 13:29:50 +02:00

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

PBVH: update mesh data pointers stored in pbvh #106271