Sculpt: Use C++ Set to store PBVH Node BMesh elements #113907

Hans Goudey · 2023-10-18T20:12:27+02:00

Hans Goudey commented

2023-10-18 20:12:27 +02:00

Mainly to simplify code and also add some add type safety, replace
GSet with blender::Set for the storage of BMesh triangles and
vertices on each PBVH node. Some initial tests point to better
performance too, but the numbers are hard to verify so far.

Because of the larger PBVHNode, memory usage slightly increases
(observed a 2% increase with a 1M face grid) for regular Mesh sculpting,
but it seems Set is more memory efficient than GSet, because I also
observed a 10% decrease in memory usage for dynamic topology.
In the future nodes can be split in a more data-oriented fashion to
reduce memory usage overall.

This also makes it simpler to switch to another type in the future.

Mainly to simplify code and also add some add type safety, replace `GSet` with `blender::Set` for the storage of BMesh triangles and vertices on each PBVH node. Some initial tests point to better performance too, but the numbers are hard to verify so far. Because of the larger `PBVHNode`, memory usage slightly increases (observed a 2% increase with a 1M face grid) for regular Mesh sculpting, but it seems `Set` is more memory efficient than `GSet`, because I also observed a 10% decrease in memory usage for dynamic topology. In the future nodes can be split in a more data-oriented fashion to reduce memory usage overall. This also makes it simpler to switch to another type in the future.

Hans Goudey added 1 commit 2023-10-18 20:12:42 +02:00

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

2bdb3b1ce5 Cleanup: Use C++ Set to store BMesh faces

Mainly to simplify code and also add some add type safety, replace
`GSet` with `blender::Set` for the storage of BMesh triangles on
each PBVH node. Faces aren't used that much in sculpt code so the code
change is relatively simple.

Hans Goudey changed title from ~~Cleanup: Use C++ Set to store BMesh faces~~ to Cleanup: Use C++ Set to store PBVH Node BMesh faces

2023-10-18 20:14:26 +02:00

Hans Goudey commented

2023-10-18 22:08:08 +02:00

@blender-bot build

Hans Goudey changed title from ~~Cleanup: Use C++ Set to store PBVH Node BMesh faces~~ to Cleanup: Use C++ Set to store PBVH Node BMesh elements

2023-10-18 23:23:00 +02:00

Hans Goudey added 2 commits 2023-10-18 23:32:44 +02:00

14a54fdb56 Merge branch 'main' into cleanup-pbvh-bmesh-set

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

db89402a6e Move vertices to use blender::Set too

Hans Goudey commented

2023-10-18 23:33:04 +02:00

@blender-bot build

Hans Goudey requested review from Sergey Sharybin 2023-10-18 23:33:40 +02:00

Hans Goudey added this to the Sculpt, Paint & Texture project 2023-10-18 23:33:53 +02:00

Hans Goudey added the

Module

Sculpt, Paint & Texture

label 2023-10-18 23:34:00 +02:00

Hans Goudey changed title from ~~Cleanup: Use C++ Set to store PBVH Node BMesh elements~~ to Sculpt: Use C++ Set to store PBVH Node BMesh elements

2023-10-18 23:34:13 +02:00

Jesse Yurkovich reviewed 2023-10-19 00:25:07 +02:00

source/blender/blenkernel/intern/pbvh_intern.hh Outdated

						
				@ -114,3 +117,1 @@

				  GSet *bm_faces = nullptr;

				  GSet *bm_unique_verts = nullptr;

				  GSet *bm_other_verts = nullptr;

				  blender::Set<BMFace *> bm_faces;

Jesse Yurkovich commented

2023-10-19 00:25:07 +02:00

Storing the blender::Set directly like this increases the size of PBVHNode from 272 to 608 bytes (each set is 120). Is that alright?

Storing the blender::Set directly like this increases the size of `PBVHNode` from 272 to 608 bytes (each set is 120). Is that alright?

Sergey Sharybin commented

2023-10-19 10:01:39 +02:00

Would be nice to double-check on some production high-poky mesh.

AFAIK, the nodes are quite "big" in the terms of amount of geometry they hold, so intuitively I would not consider it a worrysome increase for the files.

In the future we'd need to look into making nodes much smaller, in both memory foorptintm, but also in terms of amount of geometry per leaf node.

That being said, maybe it would be better to do std::unique_ptr<blender::Set> so that the increase of memory footprint only happen for BMesh case (where a lot more of memory will be allocated on heap for GSet anyway) ?

Would be nice to double-check on some production high-poky mesh. AFAIK, the nodes are quite "big" in the terms of amount of geometry they hold, so intuitively I would not consider it a worrysome increase for the files. In the future we'd need to look into making nodes much smaller, in both memory foorptintm, but also in terms of amount of geometry per leaf node. That being said, maybe it would be better to do `std::unique_ptr<blender::Set>` so that the increase of memory footprint only happen for BMesh case (where a lot more of memory will be allocated on heap for GSet anyway) ?

Hans Goudey commented

2023-10-19 10:18:08 +02:00

Using a zero inline buffer size brought the size per set down to 64 bytes. With the total PBVHNode size now at 416. That's still not a great increase, but at leas the size isn't double anymore.

My hope for PBVH leaf nodes it to separate them into separate structs for each different PBVH type. That way the size of these sets wouldn't influence the size of nodes for multires or Mesh sculpting. I'd also separate things like PBVHColorBufferNode and PBVHPixelsNode so we don't always have to store them. Non-leaf nodes wouldn't store any of this data at all.

If it helps I can make a design/todo task about that.

For dynamic topology, I think storing the sets directly is nice and will help reduce indirection.

Using a zero inline buffer size brought the size per set down to 64 bytes. With the total `PBVHNode` size now at 416. That's still not a great increase, but at leas the size isn't double anymore. My hope for PBVH leaf nodes it to separate them into separate structs for each different PBVH type. That way the size of these sets wouldn't influence the size of nodes for multires or Mesh sculpting. I'd also separate things like `PBVHColorBufferNode` and `PBVHPixelsNode` so we don't always have to store them. Non-leaf nodes wouldn't store any of this data at all. If it helps I can make a design/todo task about that. For dynamic topology, I think storing the sets directly is nice and will help reduce indirection.

Sergey Sharybin commented

2023-10-19 10:36:39 +02:00

That's cool improvement!

Still think it would be nice to get some numbers for memory consumption before/after.

To give you some background. This is 50% increase of node size. Increasing BVH2 node size in used in Cycles will lead to around 30% (give of take) overall memory usage for scenes with complex geometry and procedural materials. On a typical production file it will be 10-15% hit. Is not something we should be accepting that easily.

As I've said before, I don't really expect it to be that big of a problem for the PBVH, but it would be very nice to see some number to confirm our expectations. Should be an easy test to do: subdivide cube to 1M faces, enabled dyntopo, compare total memory usage before/after patch.

That's cool improvement! Still think it would be nice to get some numbers for memory consumption before/after. To give you some background. This is 50% increase of node size. Increasing BVH2 node size in used in Cycles will lead to around 30% (give of take) overall memory usage for scenes with complex geometry and procedural materials. On a typical production file it will be 10-15% hit. Is not something we should be accepting that easily. As I've said before, I don't really expect it to be that big of a problem for the PBVH, but it would be very nice to see some number to confirm our expectations. Should be an easy test to do: subdivide cube to 1M faces, enabled dyntopo, compare total memory usage before/after patch.

Hans Goudey commented

2023-10-19 10:55:31 +02:00

I did some basic testing with a 1 million face grid.

Before: Dyntopo: 1.84 GB  Mesh: 367 MB
After:  Dyntopo: 1.67 GB  Mesh: 374 MB

Conclusion: 2% memory usage increase for mesh sculpting, 10% less memory usage in dynamic topology. The idea of splitting the PBVHNode struct in the future is enough to make this seem worthwhile currently IMO.

I did some basic testing with a 1 million face grid. ``` Before: Dyntopo: 1.84 GB Mesh: 367 MB After: Dyntopo: 1.67 GB Mesh: 374 MB ``` Conclusion: 2% memory usage increase for mesh sculpting, 10% less memory usage in dynamic topology. The idea of splitting the `PBVHNode` struct in the future is enough to make this seem worthwhile currently IMO.

Sergey Sharybin commented

2023-10-19 10:58:30 +02:00

Think 2% is a fine trade-off.

Jesse Yurkovich reviewed 2023-10-19 00:25:24 +02:00

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

						
				@ -230,3 +227,3 @@

				}

				static void partialvis_update_bmesh_faces(GSet *faces)

				static void partialvis_update_bmesh_faces(const blender::Set<BMFace *> faces)

Jesse Yurkovich commented

2023-10-19 00:25:24 +02:00

Pass by reference here?

HooglyBoogly marked this conversation as resolved

Sergey Sharybin reviewed 2023-10-19 10:01:53 +02:00

source/blender/blenkernel/BKE_pbvh_api.hh Outdated

						
				@ -522,0 +520,4 @@

				  std::optional<blender::Set<BMVert *>::Iterator> bm_unique_verts;

				  std::optional<blender::Set<BMVert *>::Iterator> bm_unique_verts_end;

				  std::optional<blender::Set<BMVert *>::Iterator> bm_other_verts;

				  std::optional<blender::Set<BMVert *>::Iterator> bm_other_verts_end;