WIP: Sculpt: cleanup sculpt attribute API #106920

Closed
Joseph Eagar wants to merge 7 commits from temp-sculpt-attr-api into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
5 changed files with 21 additions and 23 deletions
Showing only changes of commit 2e8b7c832e - Show all commits

View File

@ -907,8 +907,8 @@ namespace blender::bke::paint {
/* 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>
static Tptr elem_attr_ptr(const ElemRef elem, const SculptAttribute *attr)
template<typename T, typename ElemRef = PBVHVertRef>

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.
static T *elem_attr_ptr(const ElemRef elem, const SculptAttribute *attr)
{
void *ptr = nullptr;
Review

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

Could avoid the temporary variable here and return the pointer inside the if statement.
@ -928,18 +928,18 @@ static Tptr elem_attr_ptr(const ElemRef elem, const SculptAttribute *attr)
ptr = BM_ELEM_CD_GET_VOID_P(v, attr->bmesh_cd_offset);
}
return static_cast<Tptr>(ptr);
return static_cast<T *>(ptr);
}
/*
* Get a pointer to attribute data at vertex.
*
* Example: float *persistent_co = vertex_attr_ptr<float*>(vertex, ss->attrs.persistent_co);
* 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)
template<typename T>
static T *vertex_attr_ptr(const PBVHVertRef vertex, const SculptAttribute *attr)

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.
{
return elem_attr_ptr<Tptr, PBVHVertRef>(vertex, attr);
return elem_attr_ptr<T, PBVHVertRef>(vertex, attr);
}
/*
@ -950,7 +950,7 @@ static Tptr vertex_attr_ptr(const PBVHVertRef vertex, const SculptAttribute *att
template<typename T>
static T vertex_attr_get(const PBVHVertRef vertex, const SculptAttribute *attr)
{
return *vertex_attr_ptr<T *>(vertex, attr);
return *vertex_attr_ptr<T>(vertex, attr);
}
/*
@ -961,24 +961,23 @@ static T vertex_attr_get(const PBVHVertRef vertex, const SculptAttribute *attr)
template<typename T>
static void vertex_attr_set(const PBVHVertRef vertex, const SculptAttribute *attr, T data)
{
*vertex_attr_ptr<T *>(vertex, attr) = data;
*vertex_attr_ptr<T>(vertex, attr) = data;
}
template<typename Tptr>
static Tptr face_attr_ptr(const PBVHVertRef face, const SculptAttribute *attr)
template<typename T> static T *face_attr_ptr(const PBVHFaceRef face, const SculptAttribute *attr)
{
return elem_attr_ptr<Tptr, PBVHFaceRef>(face, attr);
return elem_attr_ptr<T, PBVHFaceRef>(face, attr);
}
template<typename T> static T face_attr_get(const PBVHFaceRef face, const SculptAttribute *attr)
{
return *face_attr_ptr<T *>(face, attr);
return *face_attr_ptr<T>(face, attr);
}
template<typename T>
static void face_attr_set(const PBVHFaceRef face, const SculptAttribute *attr, T data)
{
*face_attr_ptr<T *>(face, attr) = data;
*face_attr_ptr<T>(face, attr) = data;
}
} // namespace blender::bke::paint
#endif

View File

@ -212,7 +212,7 @@ void SCULPT_vertex_normal_get(SculptSession *ss, PBVHVertRef vertex, float no[3]
const float *SCULPT_vertex_persistent_co_get(SculptSession *ss, PBVHVertRef vertex)
{
if (ss->attrs.persistent_co) {
return vertex_attr_ptr<const float *>(vertex, ss->attrs.persistent_co);
return vertex_attr_ptr<const float>(vertex, ss->attrs.persistent_co);
}
return SCULPT_vertex_co_get(ss, vertex);
@ -260,7 +260,7 @@ void SCULPT_vertex_limit_surface_get(SculptSession *ss, PBVHVertRef vertex, floa
void SCULPT_vertex_persistent_normal_get(SculptSession *ss, PBVHVertRef vertex, float no[3])
{
if (ss->attrs.persistent_no) {
copy_v3_v3(no, vertex_attr_ptr<float *>(vertex, ss->attrs.persistent_no));
copy_v3_v3(no, vertex_attr_ptr<float>(vertex, ss->attrs.persistent_no));
return;
}
SCULPT_vertex_normal_get(ss, vertex, no);

View File

@ -268,7 +268,7 @@ static float automasking_view_occlusion_factor(AutomaskingCache *automasking,
char f = vertex_attr_get<char>(vertex, ss->attrs.automasking_occlusion);
if (stroke_id != automasking->current_stroke_id) {
f = *vertex_attr_ptr<char *>(
f = *vertex_attr_ptr<char>(

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.
vertex,
ss->attrs.automasking_occlusion) = SCULPT_vertex_is_occluded(ss, vertex, true) ? 2 : 1;
}
@ -748,8 +748,8 @@ static void SCULPT_boundary_automasking_init(Object *ob,
const float p = 1.0f - (float(edge_distance[i]) / float(propagation_steps));
const float edge_boundary_automask = pow2f(p);
*vertex_attr_ptr<float *>(vertex,
ss->attrs.automasking_factor) *= (1.0f - edge_boundary_automask);
*vertex_attr_ptr<float>(vertex,
ss->attrs.automasking_factor) *= (1.0f - edge_boundary_automask);
}
MEM_SAFE_FREE(edge_distance);

View File

@ -1547,7 +1547,7 @@ static void do_layer_brush_task_cb_ex(void *__restrict userdata,
const int vi = vd.index;
float *disp_factor;
if (use_persistent_base) {
disp_factor = vertex_attr_ptr<float *>(vd.vertex, ss->attrs.persistent_disp);
disp_factor = vertex_attr_ptr<float>(vd.vertex, ss->attrs.persistent_disp);
}
else {
disp_factor = &ss->cache->layer_displacement_factor[vi];

View File

@ -11,9 +11,9 @@
#include "BLI_ghash.h"
#include "BLI_gsqueue.h"
#include "BLI_math.h"
#include "BLI_math_vector_types.hh"
#include "BLI_task.h"
#include "BLI_utildefines.h"
#include "BLI_math_vector_types.hh"
#include "BLT_translation.h"
@ -106,8 +106,7 @@ static int sculpt_set_persistent_base_exec(bContext *C, wmOperator * /*op*/)
PBVHVertRef vertex = BKE_pbvh_index_to_vertex(ss->pbvh, i);
vertex_attr_set<float3>(vertex, ss->attrs.persistent_co, SCULPT_vertex_co_get(ss, vertex));
SCULPT_vertex_normal_get(
ss, vertex, vertex_attr_ptr<float *>(vertex, ss->attrs.persistent_no));
SCULPT_vertex_normal_get(ss, vertex, vertex_attr_ptr<float>(vertex, ss->attrs.persistent_no));
vertex_attr_set<float>(vertex, ss->attrs.persistent_disp, 0.0f);
}