WIP: Sculpt: cleanup sculpt attribute API #106920

Joseph Eagar · 2023-04-13T21:20:37+02:00

Joseph Eagar commented

2023-04-13 21:20:37 +02:00

This PR cleans up the sculpt attribute API a bit to make greater use of C++ templates.

BKE_sculpt_vertex_attr_get (and it's alias SCULPT_vertex_attr_get)
is now blender::bke::paint::vertex_attr_ptr. Same for the face
versions of those functions. The SCULPT_XXX aliases are removed.
Removed code duplication; [vertex/face]_attr_ptr are now implemented via
a generic template (elem_attr_ptr).
Added [vertex/face]_attr_get and [vertex/face]_attr_set functions
to get/set attribute data without pointer wrangling.
Given the unwieldly length of blender::bke::paint:: it's recommended
to do a using blender::bke::paint; at the top of sculpt files.

Example: float w = vertex_attr_get<float>(vertex, ss->attrs.automasking_factor);

This PR cleans up the sculpt attribute API a bit to make greater use of C++ templates. * `BKE_sculpt_vertex_attr_get` (and it's alias `SCULPT_vertex_attr_get`) is now `blender::bke::paint::vertex_attr_ptr`. Same for the face versions of those functions. The SCULPT_XXX aliases are removed. * Removed code duplication; `[vertex/face]_attr_ptr` are now implemented via a generic template (`elem_attr_ptr`). * Added `[vertex/face]_attr_get` and `[vertex/face]_attr_set` functions to get/set attribute data without pointer wrangling. * Given the unwieldly length of `blender::bke::paint::` it's recommended to do a `using blender::bke::paint;` at the top of sculpt files. Example: `float w = vertex_attr_get<float>(vertex, ss->attrs.automasking_factor);`

Joseph Eagar added 1 commit 2023-04-13 21:20:49 +02:00

fbf091750c Sculpt: cleanup sculpt attribute API

* `BKE_sculpt_vertex_attr_get` (and it's alias `SCULPT_vertex_attr_get`)
  is now `blender::bke::paint::vertex_attr_ptr`.  Same for the face
  versions of those functions.  The SCULPT_XXX aliases are removed.
* Removed code duplication; `[vertex/face]_attr_ptr` now just calls
  a generic template (`elem_attr_ptr`).
* Added `[vertex/face]_attr_get` and `[vertex/face]_attr_set` functions
  to get/set attribute data without pointer wrangling.
* Given the unwieldly length of `blender::bke::paint::` it's recommended
  to do a `using blender::bke::paint;` at the top of sculpt files.

Example: `float w = vertex_attr_get<float>(vertex,
ss->attrs.automasking_factor);`

Joseph Eagar requested review from Hans Goudey 2023-04-13 21:22:42 +02:00

Hans Goudey requested changes 2023-04-19 15:52:25 +02:00

Hans Goudey left a comment

In isolation, this is a nice cleanup, so I think with a few changes this PR makes sense.

However, I'm generally skeptical about making an API that processes a single element at a time.
For example, these functions take a single PBVHVertRef, abstract away the PBVH type, and return a value.
Given enough templating, some of these branches can be removed, but at the cost of templates being required everywhere and generating a lot of code. A different API might be one that processes spans of indices/bmesh pointers, and values, only abstracting away the PBVH type at the node level. That makes it easier to specialize the implementation for certain types, aligns more generally with methods of removing constant checks from hot loops, and avoids the downsides of one PBVH implementation leaking into another.

That's a fairly general comment about the design of C++ usage in the sculpt/paint area though, so maybe it's better to have that conversion elsewhere.

Given the unwieldly length of blender::bke::paint:: it's recommended
to do a using blender::bke::paint; at the top of sculpt files.

Generally adding using namespace at the top of files is discouraged since it makes it more difficult to see where code is coming from and makes name conflict resolution harder. I think the best solution here is to move entire files to the proper namespace when that becomes possible. Then blender:: will be unnecessary everywhere, and bke:: and ::paint will often become unnecessary too.

In isolation, this is a nice cleanup, so I think with a few changes this PR makes sense. However, I'm generally skeptical about making an API that processes a single element at a time. For example, these functions take a single `PBVHVertRef`, abstract away the PBVH type, and return a value. Given enough templating, some of these branches can be removed, but at the cost of templates being required everywhere and generating a lot of code. A different API might be one that processes spans of indices/bmesh pointers, and values, only abstracting away the PBVH type at the node level. That makes it easier to specialize the implementation for certain types, aligns more generally with methods of removing constant checks from hot loops, and avoids the downsides of one PBVH implementation leaking into another. That's a fairly general comment about the design of C++ usage in the sculpt/paint area though, so maybe it's better to have that conversion elsewhere. > * Given the unwieldly length of `blender::bke::paint::` it's recommended > to do a `using blender::bke::paint;` at the top of sculpt files. Generally adding `using namespace` at the top of files is discouraged since it makes it more difficult to see where code is coming from and makes name conflict resolution harder. I think the best solution here is to move entire files to the proper namespace when that becomes possible. Then `blender::` will be unnecessary everywhere, and `bke::` and `::paint` will often become unnecessary too.

source/blender/blenkernel/BKE_paint.h Outdated

						
				@ -946,0 +907,4 @@

				/* Base implementation for vertex_attr_*** and face_attr_*** methods.

				 * Returns a pointer to the attribute data (as defined by attr) for elem.

				 */

				template<typename Tptr, typename ElemRef = PBVHVertRef>

Hans Goudey commented

2023-04-19 15:41:41 +02:00

I think it might be clearer to avoid Tptr and just use T * in the code below.

I think it might be clearer to avoid `Tptr` and just use `T *` in the code below.

source/blender/blenkernel/BKE_paint.h

						
				@ -946,0 +910,4 @@

				template<typename Tptr, typename ElemRef = PBVHVertRef>

				static Tptr elem_attr_ptr(const ElemRef elem, const SculptAttribute *attr)

				{

				  void *ptr = nullptr;

Hans Goudey commented

2023-04-19 15:42:17 +02:00

Could avoid the temporary variable here and return the pointer inside the if statement.

source/blender/blenkernel/BKE_paint.h Outdated

						
				@ -946,0 +937,4 @@

				 * Example: float *persistent_co = vertex_attr_ptr<float*>(vertex, ss->attrs.persistent_co);

				 */

				template<typename Tptr>

				static Tptr vertex_attr_ptr(const PBVHVertRef vertex, const SculptAttribute *attr)

Hans Goudey commented

2023-04-19 15:45:50 +02:00

I think exposing the pointer where a value is stored might not work well with different attribute storage methods in the future, and exposes the internal of the system a bit more than necessary. (For example, virtual arrays don't provide that ability, not that we would use them here exactly). The alternative is using a get/set combo when that's actually necessary.

I think exposing the pointer where a value is stored might not work well with different attribute storage methods in the future, and exposes the internal of the system a bit more than necessary. (For example, virtual arrays don't provide that ability, not that we would use them here exactly). The alternative is using a `get`/`set` combo when that's actually necessary.

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

						
				@ -268,3 +269,3 @@

				  if (stroke_id != automasking->current_stroke_id) {

				    f = *(char *)SCULPT_vertex_attr_get(

				    f = *vertex_attr_ptr<char *>(

Hans Goudey commented

2023-04-19 15:46:24 +02:00

Can this use vertex_attr_get instead of *vertex_attr_ptr?

Same with a few places below.

Can this use `vertex_attr_get` instead of `*vertex_attr_ptr`? Same with a few places below.

Joseph Eagar commented

2023-04-22 03:33:17 +02:00

In isolation, this is a nice cleanup, so I think with a few changes this PR makes sense.

However, I'm generally skeptical about making an API that processes a single element at a time.
For example, these functions take a single PBVHVertRef, abstract away the PBVH type, and return a value.
Given enough templating, some of these branches can be removed, but at the cost of templates being required everywhere and generating a lot of code. A different API might be one that processes spans of indices/bmesh pointers, and values, only abstracting away the PBVH type at the node level. That makes it easier to specialize the implementation for certain types, aligns more generally with methods of removing constant checks from hot loops, and avoids the downsides of one PBVH implementation leaking into another.

That's a fairly general comment about the design of C++ usage in the sculpt/paint area though, so maybe it's better to have that conversion elsewhere.

I do in fact plan to use templates pretty much everywhere in the sculpt code. The node span approach is basically how the PBVHVertIter works, but there are times when you really do need an element-wise API. I think it's hard to avoid templates if you want clearly written, polymorphic code that can be plugged into very different implementing back-ends.

If you think about it there's a trade off between code clarity, compiled code generation size and runtime performance. You can only optimize two of those three.

> In isolation, this is a nice cleanup, so I think with a few changes this PR makes sense. > > However, I'm generally skeptical about making an API that processes a single element at a time. > For example, these functions take a single `PBVHVertRef`, abstract away the PBVH type, and return a value. > Given enough templating, some of these branches can be removed, but at the cost of templates being required everywhere and generating a lot of code. A different API might be one that processes spans of indices/bmesh pointers, and values, only abstracting away the PBVH type at the node level. That makes it easier to specialize the implementation for certain types, aligns more generally with methods of removing constant checks from hot loops, and avoids the downsides of one PBVH implementation leaking into another. > > That's a fairly general comment about the design of C++ usage in the sculpt/paint area though, so maybe it's better to have that conversion elsewhere. I do in fact plan to use templates pretty much everywhere in the sculpt code. The node span approach is basically how the `PBVHVertIter` works, but there are times when you really do need an element-wise API. I think it's hard to avoid templates if you want clearly written, polymorphic code that can be plugged into very different implementing back-ends. If you think about it there's a trade off between code clarity, compiled code generation size and runtime performance. You can only optimize two of those three.

Joseph Eagar added 1 commit 2023-05-01 22:40:29 +02:00

798ae0295f Merge branch 'main' into temp-sculpt-attr-api

Joseph Eagar added 2 commits 2023-05-03 08:37:50 +02:00

2e8b7c832e temp-sculpt-attr-api: Make requested changes

9b5eadf62d Merge branch 'main' into temp-sculpt-attr-api

Joseph Eagar added 1 commit 2023-05-03 23:14:14 +02:00

0376d9b8c2 Merge branch 'main' into temp-sculpt-attr-api

Joseph Eagar added 2 commits 2023-05-09 21:59:00 +02:00

c20efffafe Merge branch 'main' into temp-sculpt-attr-api

ef8777d4e8 temp-sculpt-attr-api: Tweak API to better support short-use attribute handles

Sometimes you want to clear a SculptAttribute handle without
destroying the attribute itself.

Also removed the using namespace blender::bke::paint lines.

Joseph Eagar closed this pull request

2023-10-09 21:15:21 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

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

WIP: Sculpt: cleanup sculpt attribute API #106920

Pull request closed