Sculpt: Refactor topology islands cache #125907

Hans Goudey · 2024-08-05T16:55:40+02:00

Hans Goudey commented

2024-08-05 16:55:40 +02:00

Specialize the algorithm for each geometry type.
Store the cache in an array instead of an attribute. Attributes are
meant to store user-edited data, not derived data like caches.
Calculate the island IDs with a parallel disjoint set instead of a
flood-fill algorithm.
Avoid storing the island ID array if there is only one island, saving
1 byte per vertex.

Part of #118145. - Specialize the algorithm for each geometry type. - Store the cache in an array instead of an attribute. Attributes are meant to store user-edited data, not derived data like caches. - Calculate the island IDs with a parallel disjoint set instead of a flood-fill algorithm. - Avoid storing the island ID array if there is only one island, saving 1 byte per vertex.

Hans Goudey added 17 commits 2024-08-05 16:55:51 +02:00

Sculpt: Refactor topology islands 81137f8b68

Merge branch 'main' into sculpt-topology-island-refactor 4cb81448a4

Progress a24e24a557

Merge branch 'main' into sculpt-topology-island-refactor 6093279ae1

Progress 4c8e20c0a1

Merge branch 'main' into sculpt-topology-island-refactor 63d4488f9f

Fix compile error 33248c7c41

Merge branch 'main' into sculpt-topology-island-refactor ff86853e44

Fixes ae7597489c

Merge branch 'main' into sculpt-topology-island-refactor 2c097be355

Progress c9b686fe62

Implementation for multires 5cc259698b

Simplify cache storage to how it was before 556a1b4737

Add todo comment about missing visibility support for multires adf865e419

Remove timer cbcbfbf673

Merge branch 'main' into sculpt-topology-island-refactor 56cc852598

Fixes

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

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

Details

6fd549ada9

Hans Goudey commented

2024-08-05 16:57:55 +02:00

@blender-bot build

Hans Goudey requested review from Sean Kim 2024-08-05 17:03:30 +02:00

Iliya Katushenock reviewed 2024-08-05 18:11:32 +02:00

source/blender/blenkernel/BKE_paint.hh Outdated

						
				@ -480,2 +478,4 @@

				};

				struct SculptTopologyIslandCache {

				  blender::Array<uint8_t> vert_island_ids;

Iliya Katushenock commented

2024-08-05 18:11:32 +02:00

Is there is some reason to have such limit for number of islands (or only legacy/)?

Hans Goudey commented

2024-08-05 18:22:59 +02:00

Yeah, in the previous version of this branch I stored this in a full integer if necessary. I removed that for now to make the change smaller. It's easy to add later now if we want.

👍 1

mod_moder marked this conversation as resolved

Sean Kim requested changes 2024-08-05 19:43:05 +02:00

Dismissed

Sean Kim left a comment

Main area of concern I think is the comments in vert_id_get

Main area of concern I think is the comments in `vert_id_get`

source/blender/blenlib/BLI_atomic_disjoint_set.hh

						
				@ -144,2 +144,3 @@

				   * \return The total number of unique IDs.

				   */

				  void calc_reduced_ids(MutableSpan<int> result) const;

				  int calc_reduced_ids(MutableSpan<int> result) const;

Sean Kim commented

2024-08-05 19:28:36 +02:00

Being a bit nitpicky here, but maybe this BLI change should be a separate PR? I don't think it'll have odd side effects, but it seems better to isolate it.

Hans Goudey commented

2024-08-05 19:57:07 +02:00

I committed this separately.

HooglyBoogly marked this conversation as resolved

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

						
				@ -6484,2 +6485,2 @@

				  if (ss.attrs.topology_island_key) {

				    return *static_cast<uint8_t *>(SCULPT_vertex_attr_get(vertex, ss.attrs.topology_island_key));

				  BLI_assert(ss.topology_island_cache);

				  const SculptTopologyIslandCache &cache = *ss.topology_island_cache;

Sean Kim commented

2024-08-05 19:32:49 +02:00

With this change, the updated code does crash in non-debug builds if topology_island_cache hasn't been set. Probably safer to still exit early with -1 or change this signature to std::optional<int>

With this change, the updated code does crash in non-debug builds if `topology_island_cache` hasn't been set. Probably safer to still exit early with `-1` or change this signature to `std::optional<int>`

Hans Goudey commented

2024-08-05 19:53:32 +02:00

I think developers should always make sure the cache is available before using it. However it's reasonable to avoid giving users crashes in release builds.

HooglyBoogly marked this conversation as resolved

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

						
				@ -6486,3 +6489,2 @@

				  }

				  return -1;

				  return 0;

Sean Kim commented

2024-08-05 19:35:39 +02:00

Related to the above comment, I think if we keep this method as returning only integers, it makes more sense keep it returning -1 as a not found indicator instead of changing this.

Related to the above comment, I think if we keep this method as returning only integers, it makes more sense keep it returning `-1` as a not found indicator instead of changing this.

Hans Goudey commented

2024-08-05 19:54:46 +02:00

The array is now empty when there is just one island, I think it's a little clearer to return 0 in that case

Sean Kim commented

2024-08-05 20:06:01 +02:00

Ah, I completely missed your comment and the code that explained that. Might be worth a small comment in the ensure_cache function header or on the variable inside SculptSession

Ah, I completely missed your comment and the code that explained that. Might be worth a small comment in the `ensure_cache` function header or on the variable inside `SculptSession`

👍 1

HooglyBoogly marked this conversation as resolved

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

						
				@ -6543,0 +6553,4 @@

				      SubdivCCGNeighbors neighbors;

				      for (const short y : IndexRange(key.grid_size)) {

				        for (const short x : IndexRange(key.grid_size)) {

				          const SubdivCCGCoord coord{grid, x, y};

Sean Kim commented

2024-08-05 19:39:38 +02:00

Minor styling question - do we have a standardized way of writing struct initialization? For SubdivCCGCoord in general I think I've seen this, struct member assignment, and the named initializer list.

Minor styling question - do we have a standardized way of writing struct initialization? For `SubdivCCGCoord` in general I think I've seen this, struct member assignment, and the named initializer list.

Hans Goudey commented

2024-08-05 19:56:42 +02:00

Not really AFAIK. Others have expressed that they like the named initialization options, since this way it's easy to make a mistake when changing the struct. But I don't see SubdivCCGCoord changing without a larger refactor anyway. I hadn't done this before because I didn't use short in the grid iteration. Personally I like the single-line initialization and the ability to declare the variable const.

Not really AFAIK. Others have expressed that they like the named initialization options, since this way it's easy to make a mistake when changing the struct. But I don't see `SubdivCCGCoord` changing without a larger refactor anyway. I hadn't done this before because I didn't use `short` in the grid iteration. Personally I like the single-line initialization and the ability to declare the variable const.

👍 1

Sean-Kim marked this conversation as resolved

Hans Goudey added 2 commits 2024-08-05 19:57:16 +02:00

Merge branch 'main' into sculpt-topology-island-refactor 0507f4e02c

Avoid crashes from programming errors in release builds 00bbe729c4

Hans Goudey requested review from Sean Kim 2024-08-05 19:57:18 +02:00

Sean Kim approved these changes 2024-08-05 20:06:05 +02:00

Hans Goudey added 1 commit 2024-08-05 20:18:34 +02:00

Add comment about array emptiness 12bfbe631f

Hans Goudey merged commit 79ad6a3148 into main

2024-08-05 20:20:23 +02:00

Hans Goudey referenced this issue from a commit