Sculpt: Start data-oriented refactor for draw brush #121835

Merged
Hans Goudey merged 98 commits from Sergey/blender:sculpt_brush_refactor into main 2024-06-05 14:09:28 +02:00
9 changed files with 1063 additions and 104 deletions

View File

@ -3,6 +3,8 @@
# SPDX-License-Identifier: GPL-2.0-or-later
set(INC
../..
../include
../uvedit
../../blenkernel
@ -108,9 +110,15 @@ set(SRC
curves_sculpt_intern.hh
grease_pencil_intern.hh
mesh_brush_common.hh
grease_pencil_weight_paint.hh
paint_intern.hh
sculpt_intern.hh
brushes/draw.cc
brushes/draw_vector_displacement.cc
brushes/types.hh
)
set(LIB

View File

@ -0,0 +1,241 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors
HooglyBoogly marked this conversation as resolved Outdated

Replace 2023 by 2024

Replace `2023` by `2024`
*
* SPDX-License-Identifier: GPL-2.0-or-later */
#include "editors/sculpt_paint/brushes/types.hh"
#include "DNA_brush_types.h"
#include "DNA_mesh_types.h"
#include "DNA_object_types.h"
#include "DNA_scene_types.h"
#include "BKE_key.hh"
#include "BKE_mesh.hh"
#include "BKE_paint.hh"
#include "BKE_pbvh.hh"
#include "BKE_subdiv_ccg.hh"
#include "BLI_array.hh"
#include "BLI_enumerable_thread_specific.hh"
#include "BLI_math_matrix.hh"
#include "BLI_math_vector.hh"
#include "BLI_task.h"
#include "BLI_task.hh"
#include "editors/sculpt_paint/mesh_brush_common.hh"
#include "editors/sculpt_paint/sculpt_intern.hh"
namespace blender::ed::sculpt_paint {
inline namespace draw_cc {
struct LocalData {
Vector<float> factors;
Vector<float> distances;
Vector<float3> translations;
};
static void calc_faces(const Sculpt &sd,
const Brush &brush,
const float3 &offset,
const Span<float3> positions_eval,
const Span<float3> vert_normals,
const PBVHNode &node,
HooglyBoogly marked this conversation as resolved Outdated

r_ prefixes in this file for return args

`r_` prefixes in this file for return args
Object &object,
LocalData &tls,
const MutableSpan<float3> positions_sculpt,
const MutableSpan<float3> positions_mesh)
{
SculptSession &ss = *object.sculpt;
StrokeCache &cache = *ss.cache;
Mesh &mesh = *static_cast<Mesh *>(object.data);
const Span<int> verts = bke::pbvh::node_unique_verts(node);
tls.factors.reinitialize(verts.size());
const MutableSpan<float> factors = tls.factors;
fill_factor_from_hide_and_mask(mesh, verts, factors);
if (brush.flag & BRUSH_FRONTFACE) {
calc_front_face(cache.view_normal, vert_normals, verts, factors);
}
tls.distances.reinitialize(verts.size());
const MutableSpan<float> distances = tls.distances;
calc_distance_falloff(
ss, positions_eval, verts, eBrushFalloffShape(brush.falloff_shape), distances, factors);
calc_brush_strength_factors(ss, brush, verts, distances, factors);
if (ss.cache->automasking) {
auto_mask::calc_vert_factors(object, *ss.cache->automasking, node, verts, factors);
}
calc_brush_texture_factors(ss, brush, positions_eval, verts, factors);
tls.translations.reinitialize(verts.size());
const MutableSpan<float3> translations = tls.translations;
for (const int i : verts.index_range()) {
translations[i] = offset * factors[i];
}
clip_and_lock_translations(sd, ss, positions_eval, verts, translations);
if (!ss.deform_imats.is_empty()) {
apply_crazyspace_to_translations(ss.deform_imats, verts, translations);
}
apply_translations(translations, verts, positions_sculpt);
flush_positions_to_shape_keys(object, verts, positions_sculpt, positions_mesh);
}
static void calc_grids(Object &object, const Brush &brush, const float3 &offset, PBVHNode &node)
{
SculptSession &ss = *object.sculpt;
SculptBrushTest test;
SculptBrushTestFn sculpt_brush_test_sq_fn = SCULPT_brush_test_init_with_falloff_shape(
ss, test, brush.falloff_shape);
const int thread_id = BLI_task_parallel_thread_id(nullptr);
auto_mask::NodeData automask_data = auto_mask::node_begin(
object, ss.cache->automasking.get(), node);
SubdivCCG &subdiv_ccg = *ss.subdiv_ccg;
const CCGKey key = *BKE_pbvh_get_grid_key(*ss.pbvh);
const Span<CCGElem *> grids = subdiv_ccg.grids;
const BitGroupVector<> &grid_hidden = subdiv_ccg.grid_hidden;
/* TODO: Remove usage of proxies. */
const MutableSpan<float3> proxy = BKE_pbvh_node_add_proxy(*ss.pbvh, node).co;
int i = 0;
for (const int grid : bke::pbvh::node_grid_indices(node)) {
const int grid_verts_start = grid * key.grid_area;
CCGElem *elem = grids[grid];
for (const int j : IndexRange(key.grid_area)) {
if (!grid_hidden.is_empty() && grid_hidden[grid][j]) {
i++;
continue;
}
if (!sculpt_brush_test_sq_fn(test, CCG_elem_offset_co(key, elem, j))) {
i++;
continue;
}
auto_mask::node_update(automask_data, i);
const float fade = SCULPT_brush_strength_factor(
ss,
brush,
CCG_elem_offset_co(key, elem, j),
math::sqrt(test.dist),
CCG_elem_offset_no(key, elem, j),
nullptr,
key.has_mask ? CCG_elem_offset_mask(key, elem, j) : 0.0f,
BKE_pbvh_make_vref(grid_verts_start + j),
thread_id,
&automask_data);
proxy[i] = offset * fade;
i++;
}
}
}
static void calc_bmesh(Object &object, const Brush &brush, const float3 &offset, PBVHNode &node)
{
SculptSession &ss = *object.sculpt;
SculptBrushTest test;
SculptBrushTestFn sculpt_brush_test_sq_fn = SCULPT_brush_test_init_with_falloff_shape(
ss, test, brush.falloff_shape);
const int thread_id = BLI_task_parallel_thread_id(nullptr);
auto_mask::NodeData automask_data = auto_mask::node_begin(
object, ss.cache->automasking.get(), node);
const int mask_offset = CustomData_get_offset_named(
&ss.bm->vdata, CD_PROP_FLOAT, ".sculpt_mask");
/* TODO: Remove usage of proxies. */
const MutableSpan<float3> proxy = BKE_pbvh_node_add_proxy(*ss.pbvh, node).co;
int i = 0;
for (BMVert *vert : BKE_pbvh_bmesh_node_unique_verts(&node)) {
if (BM_elem_flag_test(vert, BM_ELEM_HIDDEN)) {
i++;
continue;
}
if (!sculpt_brush_test_sq_fn(test, vert->co)) {
i++;
continue;
}
auto_mask::node_update(automask_data, *vert);
const float mask = mask_offset == -1 ? 0.0f : BM_ELEM_CD_GET_FLOAT(vert, mask_offset);
const float fade = SCULPT_brush_strength_factor(ss,
brush,
vert->co,
math::sqrt(test.dist),
vert->no,
nullptr,
mask,
BKE_pbvh_make_vref(intptr_t(vert)),
Sean-Kim marked this conversation as resolved Outdated

We're going to have this switch statement repeated a fair amount across the brushes as we migrate them, do you think there's a common abstract class we could extract for the different PBVH types and their respective calc_ methods?

We're going to have this switch statement repeated a fair amount across the brushes as we migrate them, do you think there's a common abstract class we could extract for the different PBVH types and their respective `calc_` methods?

Yeah, I think we might end up with something like that. Maybe even with some storage data during a brush stroke. But I would rather keep this simple for now, and only add abstractions a bit later when it's clearer.

Yeah, I think we might end up with something like that. Maybe even with some storage data during a brush stroke. But I would rather keep this simple for now, and only add abstractions a bit later when it's clearer.
thread_id,
&automask_data);
proxy[i] = offset * fade;
i++;
}
}
} // namespace draw_cc
void do_draw_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes)
{
const SculptSession &ss = *object.sculpt;
const Brush &brush = *BKE_paint_brush_for_read(&sd.paint);
const float bstrength = ss.cache->bstrength;
/* Offset with as much as possible factored in already. */
float3 effective_normal;
SCULPT_tilt_effective_normal_get(ss, brush, effective_normal);
const float3 offset = effective_normal * ss.cache->radius * ss.cache->scale * bstrength;
switch (BKE_pbvh_type(*object.sculpt->pbvh)) {
case PBVH_FACES: {
threading::EnumerableThreadSpecific<LocalData> all_tls;
Mesh &mesh = *static_cast<Mesh *>(object.data);
const PBVH &pbvh = *ss.pbvh;
const Span<float3> positions_eval = BKE_pbvh_get_vert_positions(pbvh);
const Span<float3> vert_normals = BKE_pbvh_get_vert_normals(pbvh);
MutableSpan<float3> positions_sculpt = mesh_brush_positions_for_write(*object.sculpt, mesh);
MutableSpan<float3> positions_mesh = mesh.vert_positions_for_write();
threading::parallel_for(nodes.index_range(), 1, [&](const IndexRange range) {
LocalData &tls = all_tls.local();
for (const int i : range) {
calc_faces(sd,
brush,
offset,
positions_eval,
vert_normals,
*nodes[i],
object,
tls,
positions_sculpt,
positions_mesh);
BKE_pbvh_node_mark_positions_update(nodes[i]);
}
});
break;
}
case PBVH_GRIDS:
threading::parallel_for(nodes.index_range(), 1, [&](const IndexRange range) {
for (const int i : range) {
calc_grids(object, brush, offset, *nodes[i]);
}
});
break;
case PBVH_BMESH:
threading::parallel_for(nodes.index_range(), 1, [&](const IndexRange range) {
for (const int i : range) {
calc_bmesh(object, brush, offset, *nodes[i]);
}
});
break;
}
}
} // namespace blender::ed::sculpt_paint

View File

@ -0,0 +1,263 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors
HooglyBoogly marked this conversation as resolved Outdated

Replace 2023 by 2024

Replace `2023` by `2024`
*
* SPDX-License-Identifier: GPL-2.0-or-later */
#include "editors/sculpt_paint/brushes/types.hh"
#include "DNA_brush_types.h"
#include "DNA_mesh_types.h"
#include "DNA_object_types.h"
#include "DNA_scene_types.h"
#include "BKE_key.hh"
#include "BKE_mesh.hh"
#include "BKE_paint.hh"
#include "BKE_pbvh.hh"
#include "BKE_subdiv_ccg.hh"
#include "BLI_array.hh"
#include "BLI_enumerable_thread_specific.hh"
#include "BLI_math_vector.hh"
#include "BLI_task.h"
#include "BLI_task.hh"
#include "editors/sculpt_paint/mesh_brush_common.hh"
#include "editors/sculpt_paint/sculpt_intern.hh"
namespace blender::ed::sculpt_paint {
inline namespace draw_vector_displacement_cc {
struct LocalData {
Vector<float> factors;
Vector<float> distances;
Vector<float4> colors;
Vector<float3> translations;
};
static void calc_brush_texture_colors(SculptSession &ss,
const Brush &brush,
const Span<float3> vert_positions,
const Span<int> verts,
const Span<float> factors,
const MutableSpan<float4> r_colors)
{
BLI_assert(verts.size() == r_colors.size());
const int thread_id = BLI_task_parallel_thread_id(nullptr);
for (const int i : verts.index_range()) {
float texture_value;
float4 texture_rgba;
/* NOTE: This is not a thread-safe call. */
sculpt_apply_texture(
ss, brush, vert_positions[verts[i]], thread_id, &texture_value, texture_rgba);
r_colors[i] = texture_rgba * factors[i];
}
}
static void calc_faces(const Sculpt &sd,
const Brush &brush,
const Span<float3> positions_eval,
const Span<float3> vert_normals,
const PBVHNode &node,
Object &object,
LocalData &tls,
MutableSpan<float3> positions_orig,
MutableSpan<float3> mesh_positions)
{
SculptSession &ss = *object.sculpt;
StrokeCache &cache = *ss.cache;
Mesh &mesh = *static_cast<Mesh *>(object.data);
const Span<int> verts = bke::pbvh::node_unique_verts(node);
tls.factors.reinitialize(verts.size());
const MutableSpan<float> factors = tls.factors;
fill_factor_from_hide_and_mask(mesh, verts, factors);
if (brush.flag & BRUSH_FRONTFACE) {
calc_front_face(cache.view_normal, vert_normals, verts, factors);
}
tls.distances.reinitialize(verts.size());
const MutableSpan<float> distances = tls.distances;
calc_distance_falloff(
ss, positions_eval, verts, eBrushFalloffShape(brush.falloff_shape), distances, factors);
calc_brush_strength_factors(ss, brush, verts, distances, factors);
if (ss.cache->automasking) {
auto_mask::calc_vert_factors(object, *ss.cache->automasking, node, verts, factors);
}
tls.colors.reinitialize(verts.size());
const MutableSpan<float4> colors = tls.colors;
calc_brush_texture_colors(ss, brush, positions_eval, verts, factors, colors);
tls.translations.reinitialize(verts.size());
const MutableSpan<float3> translations = tls.translations;
for (const int i : verts.index_range()) {
SCULPT_calc_vertex_displacement(ss, brush, colors[i], translations[i]);
}
clip_and_lock_translations(sd, ss, positions_eval, verts, translations);
if (!ss.deform_imats.is_empty()) {
apply_crazyspace_to_translations(ss.deform_imats, verts, translations);
}
apply_translations(translations, verts, positions_orig);
flush_positions_to_shape_keys(object, verts, positions_orig, mesh_positions);
}
static void calc_grids(Object &object, const Brush &brush, PBVHNode &node)
{
SculptSession &ss = *object.sculpt;
SubdivCCG &subdiv_ccg = *ss.subdiv_ccg;
const CCGKey key = *BKE_pbvh_get_grid_key(*ss.pbvh);
const Span<CCGElem *> grids = subdiv_ccg.grids;
const BitGroupVector<> &grid_hidden = subdiv_ccg.grid_hidden;
SculptBrushTest test;
SculptBrushTestFn sculpt_brush_test_sq_fn = SCULPT_brush_test_init_with_falloff_shape(
ss, test, brush.falloff_shape);
const int thread_id = BLI_task_parallel_thread_id(nullptr);
auto_mask::NodeData automask_data = auto_mask::node_begin(
object, ss.cache->automasking.get(), node);
/* TODO: Remove usage of proxies. */
const MutableSpan<float3> proxy = BKE_pbvh_node_add_proxy(*ss.pbvh, node).co;
int i = 0;
for (const int grid : bke::pbvh::node_grid_indices(node)) {
const int grid_verts_start = grid * key.grid_area;
CCGElem *elem = grids[grid];
for (const int j : IndexRange(key.grid_area)) {
if (!grid_hidden.is_empty() && grid_hidden[grid][j]) {
i++;
continue;
}
if (!sculpt_brush_test_sq_fn(test, CCG_elem_offset_co(key, elem, j))) {
i++;
continue;
}
auto_mask::node_update(automask_data, i);
float r_rgba[4];
SCULPT_brush_strength_color(ss,
brush,
CCG_elem_offset_co(key, elem, j),
math::sqrt(test.dist),
CCG_elem_offset_no(key, elem, j),
nullptr,
key.has_mask ? CCG_elem_offset_mask(key, elem, j) : 0.0f,
BKE_pbvh_make_vref(grid_verts_start + j),
thread_id,
&automask_data,
r_rgba);
SCULPT_calc_vertex_displacement(ss, brush, r_rgba, proxy[i]);
i++;
}
}
}
static void calc_bmesh(Object &object, const Brush &brush, PBVHNode &node)
{
SculptSession &ss = *object.sculpt;
SculptBrushTest test;
SculptBrushTestFn sculpt_brush_test_sq_fn = SCULPT_brush_test_init_with_falloff_shape(
ss, test, brush.falloff_shape);
const int thread_id = BLI_task_parallel_thread_id(nullptr);
auto_mask::NodeData automask_data = auto_mask::node_begin(
object, ss.cache->automasking.get(), node);
const int mask_offset = CustomData_get_offset_named(
&ss.bm->vdata, CD_PROP_FLOAT, ".sculpt_mask");
/* TODO: Remove usage of proxies. */
const MutableSpan<float3> proxy = BKE_pbvh_node_add_proxy(*ss.pbvh, node).co;
int i = 0;
for (BMVert *vert : BKE_pbvh_bmesh_node_unique_verts(&node)) {
if (BM_elem_flag_test(vert, BM_ELEM_HIDDEN)) {
i++;
continue;
}
if (!sculpt_brush_test_sq_fn(test, vert->co)) {
i++;
continue;
}
auto_mask::node_update(automask_data, *vert);
const float mask = mask_offset == -1 ? 0.0f : BM_ELEM_CD_GET_FLOAT(vert, mask_offset);
float r_rgba[4];
SCULPT_brush_strength_color(ss,
brush,
vert->co,
math::sqrt(test.dist),
vert->no,
nullptr,
mask,
BKE_pbvh_make_vref(intptr_t(vert)),
thread_id,
&automask_data,
r_rgba);
SCULPT_calc_vertex_displacement(ss, brush, r_rgba, proxy[i]);
i++;
}
}
} // namespace draw_vector_displacement_cc
void do_draw_vector_displacement_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes)
{
const SculptSession &ss = *object.sculpt;
const Brush &brush = *BKE_paint_brush_for_read(&sd.paint);
switch (BKE_pbvh_type(*object.sculpt->pbvh)) {
case PBVH_FACES: {
threading::EnumerableThreadSpecific<LocalData> all_tls;
Mesh &mesh = *static_cast<Mesh *>(object.data);
const PBVH &pbvh = *ss.pbvh;
const Span<float3> positions_eval = BKE_pbvh_get_vert_positions(pbvh);
const Span<float3> vert_normals = BKE_pbvh_get_vert_normals(pbvh);
MutableSpan<float3> positions_orig = mesh_brush_positions_for_write(*object.sculpt, mesh);
MutableSpan<float3> mesh_positions = mesh.vert_positions_for_write();
threading::parallel_for(nodes.index_range(), 1, [&](const IndexRange range) {
LocalData &tls = all_tls.local();
for (const int i : range) {
calc_faces(sd,
brush,
positions_eval,
vert_normals,
*nodes[i],
object,
tls,
positions_orig,
mesh_positions);
BKE_pbvh_node_mark_positions_update(nodes[i]);
}
});
break;
}
case PBVH_GRIDS:
threading::parallel_for(nodes.index_range(), 1, [&](const IndexRange range) {
for (const int i : range) {
calc_grids(object, brush, *nodes[i]);
}
});
break;
case PBVH_BMESH:
threading::parallel_for(nodes.index_range(), 1, [&](const IndexRange range) {
for (const int i : range) {
calc_bmesh(object, brush, *nodes[i]);
}
});
break;
}
}
} // namespace blender::ed::sculpt_paint

View File

@ -0,0 +1,20 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors
HooglyBoogly marked this conversation as resolved Outdated

Replace 2023 by 2024

Replace `2023` by `2024`
*
* SPDX-License-Identifier: GPL-2.0-or-later */
#pragma once
#include "BLI_span.hh"
struct Sculpt;
struct Object;
struct PBVHNode;
namespace blender::ed::sculpt_paint {
Sean-Kim marked this conversation as resolved
Review

How about we add a new namespace here for brushes?

How about we add a new namespace here for brushes?
Review

I tried this but didn't really like it. It conflicted a bit with more specific namespaces like cloth,pose, and smooth. Such a large portion of the code is brushes that maybe it's not necessary? It also sort of relates to how the sculpt_paint module is currently shared between meshes, curves, and grease pencil, and vertex painting, etc. We could split those two separate modules that all depend on some smaller module that defines the brush system.

I tried this but didn't really like it. It conflicted a bit with more specific namespaces like `cloth`,`pose`, and `smooth`. Such a large portion of the code is brushes that maybe it's not necessary? It also sort of relates to how the `sculpt_paint` module is currently shared between meshes, curves, and grease pencil, and vertex painting, etc. We could split those two separate modules that all depend on some smaller module that defines the brush system.
/** A simple normal-direction displacement. */
HooglyBoogly marked this conversation as resolved Outdated

Documentation for both of these methods? Maybe just something about the difference between the regular draw_brush and draw_vector_displacement_brush?

Documentation for both of these methods? Maybe just something about the difference between the regular `draw_brush` and `draw_vector_displacement_brush`?
void do_draw_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes);
/** A simple normal-direction displacement based on image texture RGB/XYZ values. */
void do_draw_vector_displacement_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes);
farsthary marked this conversation as resolved

In the future, new/ported brushes should be added here?

In the future, new/ported brushes should be added here?
Review

Yep, we've opted out from having per-brush header.

Yep, we've opted out from having per-brush header.
} // namespace blender::ed::sculpt_paint

View File

@ -0,0 +1,167 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors
*
* SPDX-License-Identifier: GPL-2.0-or-later */
#pragma once
#include "BLI_math_matrix_types.hh"
#include "BLI_math_vector_types.hh"
#include "BLI_span.hh"
#include "DNA_brush_enums.h"
/**
* This file contains common operations useful for the implementation of various different brush
* tools. The design goals of the API are to always operate on more than one data element at a
* time, to avoid unnecessary branching for constants, favor cache-friendly access patterns, enable
* use of SIMD, and provide opportunities to avoid work where possible.
*
* API function arguments should favor passing raw data references rather than general catch-all
* storage structs in order to clarify the scope of each function, structure the work around the
* required data, and limit redundant data storage.
*
* Many functions calculate "factors" which describe how strong the brush influence should be
* between 0 and 1. Most functions multiply with the existing factor value rather than assigning a
* new value from scratch.
*/
struct Brush;
struct Mesh;
struct Object;
struct PBVHNode;
struct Sculpt;
struct SculptSession;
namespace blender::ed::sculpt_paint {
namespace auto_mask {
struct Cache;
};
/**
* Note on the various positions arrays:
* - positions_sculpt: The positions affected by brush strokes (maybe indirectly). Owned by the
* PBVH or mesh.
* - positions_mesh: Positions owned by the original mesh. Not the same as `positions_sculpt` if
* there are deform modifiers.
* - positions_eval: Positions after procedural deformation, used to build the PBVH. Translations
* are built for these values, then applied to `positions_sculpt`.
*
* Only two of these arrays are actually necessary. The third comes from the fact that the PBVH
* currently stores its own copy of positions when there are deformations. If that was removed, the
* situation would be clearer.
*
* \todo Get rid of one of the arrays mentioned above to avoid the situation with evaluated
* positions, original positions, and then a third copy that's just there because of historical
* reasons. This would involve removing access to positions and normals from the PBVH structure,
* which should only be concerned with splitting geometry into spacially contiguous chunks.
*/
/**
* Calculate initial influence factors based on vertex visibility and masking.
*/
void fill_factor_from_hide_and_mask(const Mesh &mesh,
Span<int> vert_indices,
MutableSpan<float> r_factors);
/**
* Disable brush influence when vertex normals point away from the view.
*/
void calc_front_face(const float3 &view_normal,
Span<float3> vert_normals,
Span<int> vert_indices,
MutableSpan<float> factors);
/**
* Modify influence factors based on the distance from the brush cursor and various other settings.
* Also fill an array of distances from the brush cursor for "in bounds" vertices.
*/
void calc_distance_falloff(SculptSession &ss,
Span<float3> vert_positions,
Span<int> vert_indices,
eBrushFalloffShape falloff_shape,
MutableSpan<float> r_distances,
MutableSpan<float> factors);
/**
* Modify the factors based on distances to the brush cursor, using various brush settings.
*/
void calc_brush_strength_factors(const SculptSession &ss,
farsthary marked this conversation as resolved

If both calc_distance_falloff() and calc_brush_strength_factors() modify influence factors based on the distance from the brush cursor and various other settings, what's the main functional difference?

If both calc_distance_falloff() and calc_brush_strength_factors() modify influence factors based on the distance from the brush cursor and various other settings, what's the main functional difference?
Review

It is indeed a bit tricky difference. The calc_distance_falloff only depends on the brush position. The calc_brush_strength_factors does more things like hardness, curve etc.

Splitting those into two functions basically:

  • Allows each of them to be simpler, likely benefiting from cache coherencies and such much better in comparison with if they were more of a mega-kernel.
  • Allows to have an early outputs in the latter function, without affecting branching and such in the comparison to the mega-kernel.
It is indeed a bit tricky difference. The `calc_distance_falloff ` only depends on the brush position. The `calc_brush_strength_factors` does more things like hardness, curve etc. Splitting those into two functions basically: - Allows each of them to be simpler, likely benefiting from cache coherencies and such much better in comparison with if they were more of a mega-kernel. - Allows to have an early outputs in the latter function, without affecting branching and such in the comparison to the mega-kernel.
const Brush &brush,
Span<int> vert_indices,
Span<float> distances,
MutableSpan<float> factors);
/**
* Modify brush influence factors to include sampled texture values.
*/
void calc_brush_texture_factors(SculptSession &ss,
const Brush &brush,
Span<float3> vert_positions,
Span<int> vert_indices,
MutableSpan<float> factors);
namespace auto_mask {
/**
* Calculate all auto-masking influence on each vertex.
*
* \todo Remove call to `undo::push_node` deep inside this function so the `object` argument can be
* const. That may (hopefully) require pulling out the undo node push into the code for each brush.
* That should help clarify the code path for brushes, and various optimizations will depend on
* brush implementations doing their own undo pushes.
*/
void calc_vert_factors(Object &object,
const Cache &cache,
const PBVHNode &node,
Span<int> verts,
MutableSpan<float> factors);
} // namespace auto_mask
/**
* Many brushes end up calculating translations from the original positions. Instead of applying
* these directly to the modified values, it's helpful to process them separately to easily
* calculate various effects like clipping. After they are processed, this function can be used to
* simply add them to the final vertex positions.
*/
void apply_translations(Span<float3> translations, Span<int> verts, MutableSpan<float3> positions);
/**
* Rotate translations to account for rotations from procedural deformation.
*
* \todo Don't invert `deform_imats` on object evaluation. Instead just invert them on-demand in
* brush implementations. This would be better because only the inversions required for affected
* vertices would be necessary.
*/
void apply_crazyspace_to_translations(Span<float3x3> deform_imats,
Span<int> verts,
MutableSpan<float3> translations);
/**
* Modify translations based on sculpt mode axis locking and mirroring clipping.
*/
void clip_and_lock_translations(const Sculpt &sd,
const SculptSession &ss,
Span<float3> positions,
Span<int> verts,
MutableSpan<float3> translations);
/**
* Retrieve the final mutable positions array to be modified.
*
* \note See the comment at the top of this file for context.
*/
MutableSpan<float3> mesh_brush_positions_for_write(SculptSession &ss, Mesh &mesh);
/**
* Applying final positions to shape keys is non-trivial because the mesh positions and the active
* shape key positions must be kept in sync, and shape keys dependent on the active key must also
* be modified.
*/
void flush_positions_to_shape_keys(Object &object,
Span<int> verts,
Span<float3> positions,
MutableSpan<float3> positions_mesh);
} // namespace blender::ed::sculpt_paint

View File

@ -23,6 +23,7 @@
#include "BLI_ghash.h"
#include "BLI_math_geom.h"
#include "BLI_math_matrix.h"
#include "BLI_math_matrix.hh"
#include "BLI_math_rotation.h"
#include "BLI_set.hh"
#include "BLI_span.hh"
@ -85,6 +86,9 @@
#include "bmesh.hh"
#include "editors/sculpt_paint/brushes/types.hh"
#include "mesh_brush_common.hh"
using blender::float3;
using blender::MutableSpan;
using blender::Set;
@ -1238,6 +1242,33 @@ void SCULPT_orig_vert_data_update(SculptOrigVertData &orig_data, const PBVHVerte
}
}
void SCULPT_orig_vert_data_update(SculptOrigVertData &orig_data, const BMVert &vert)
{
using namespace blender::ed::sculpt_paint;
if (orig_data.unode->type == undo::Type::Position) {
BM_log_original_vert_data(
orig_data.bm_log, &const_cast<BMVert &>(vert), &orig_data.co, &orig_data.no);
}
else if (orig_data.unode->type == undo::Type::Mask) {
orig_data.mask = BM_log_original_mask(orig_data.bm_log, &const_cast<BMVert &>(vert));
}
}
void SCULPT_orig_vert_data_update(SculptOrigVertData &orig_data, const int i)
{
using namespace blender::ed::sculpt_paint;
if (orig_data.unode->type == undo::Type::Position) {
orig_data.co = orig_data.coords[i];
orig_data.no = orig_data.normals[i];
}
else if (orig_data.unode->type == undo::Type::Color) {
orig_data.col = orig_data.colors[i];
}
else if (orig_data.unode->type == undo::Type::Mask) {
orig_data.mask = orig_data.vmasks[i];
}
}
namespace blender::ed::sculpt_paint {
static void sculpt_rake_data_update(SculptRakeData *srd, const float co[3])
@ -2389,12 +2420,12 @@ static float sculpt_apply_hardness(const SculptSession &ss, const float input_le
return final_len;
}
static void sculpt_apply_texture(SculptSession &ss,
const Brush &brush,
const float brush_point[3],
const int thread_id,
float *r_value,
float r_rgba[4])
void sculpt_apply_texture(const SculptSession &ss,
const Brush &brush,
const float brush_point[3],
const int thread_id,
float *r_value,
float r_rgba[4])
{
const blender::ed::sculpt_paint::StrokeCache &cache = *ss.cache;
const Scene *scene = cache.vc->scene;
@ -3516,9 +3547,17 @@ static void do_brush_action(const Sculpt &sd,
/* Apply one type of brush action. */
switch (brush.sculpt_tool) {
case SCULPT_TOOL_DRAW:
SCULPT_do_draw_brush(sd, ob, nodes);
case SCULPT_TOOL_DRAW: {
const bool use_vector_displacement = (brush.flag2 & BRUSH_USE_COLOR_AS_DISPLACEMENT &&
(brush.mtex.brush_map_mode == MTEX_MAP_MODE_AREA));
if (use_vector_displacement) {
ed::sculpt_paint::do_draw_vector_displacement_brush(sd, ob, nodes);
}
else {
ed::sculpt_paint::do_draw_brush(sd, ob, nodes);
}
break;
}
case SCULPT_TOOL_SMOOTH:
if (brush.smooth_deform_type == BRUSH_SMOOTH_DEFORM_LAPLACIAN) {
smooth::do_smooth_brush(sd, ob, nodes);
@ -5622,12 +5661,16 @@ static void sculpt_stroke_update_step(bContext *C,
*
* Same applies to the DEG_id_tag_update() invoked from
* sculpt_flush_update_step().
*
* For some brushes, flushing is done in the brush code itself.
*/
if (ss.deform_modifiers_active) {
SCULPT_flush_stroke_deform(sd, ob, sculpt_tool_is_proxy_used(brush.sculpt_tool));
}
else if (ss.shapekey_active) {
sculpt_update_keyblock(ob);
if (!(ELEM(brush.sculpt_tool, SCULPT_TOOL_DRAW) && BKE_pbvh_type(*ss.pbvh) == PBVH_FACES)) {
if (ss.deform_modifiers_active) {
SCULPT_flush_stroke_deform(sd, ob, sculpt_tool_is_proxy_used(brush.sculpt_tool));
}
else if (ss.shapekey_active) {
sculpt_update_keyblock(ob);
}
}
ss.cache->first_time = false;
@ -6244,3 +6287,245 @@ void SCULPT_cube_tip_init(const Sculpt & /*sd*/,
invert_m4_m4(mat, tmat);
}
/** \} */
namespace blender::ed::sculpt_paint {
void fill_factor_from_hide_and_mask(const Mesh &mesh,
const Span<int> verts,
const MutableSpan<float> r_factors)
{
BLI_assert(verts.size() == r_factors.size());
/* TODO: Avoid overhead of accessing attributes for every PBVH node. */
const bke::AttributeAccessor attributes = mesh.attributes();
if (const VArray mask = *attributes.lookup<float>(".sculpt_mask", bke::AttrDomain::Point)) {
const VArraySpan span(mask);
for (const int i : verts.index_range()) {
r_factors[i] = 1.0f - span[verts[i]];
}
}
else {
r_factors.fill(1.0f);
}
if (const VArray hide_vert = *attributes.lookup<bool>(".hide_vert", bke::AttrDomain::Point)) {
Sean-Kim marked this conversation as resolved Outdated

Is adding a clamp to [0.0, 1.0] here worthwhile?

Is adding a clamp to [0.0, 1.0] here worthwhile?

I don't really think so, I think it's fine to rely on the mask values being between 0 and 1, optimizing for that common case. Worst case there is some strange deformation.

I don't really think so, I think it's fine to rely on the mask values being between 0 and 1, optimizing for that common case. Worst case there is some strange deformation.
const VArraySpan span(hide_vert);
for (const int i : verts.index_range()) {
HooglyBoogly marked this conversation as resolved Outdated

Should this be span[verts[i]] instead? I don't see what's the point of line 6311 otherwise.

Should this be `span[verts[i]]` instead? I don't see what's the point of line 6311 otherwise.
r_factors[i] = span[verts[i]] ? 0.0f : r_factors[i];
}
Sean-Kim marked this conversation as resolved Outdated

Something that I thought of based on your comment about the initialize vs modify comment on these spans - what do you think about moving the initialization of the r_factors Vector here and the r_distances Vector later to somewhere outside of these functions?

My goal is to remove some of the implicit ordering that we have for the methods, since in theory aside from the mask step, any of the multiplicative ones could go in whatever order (outside of performance reasons to do more broad strokes first)

Something that I thought of based on your comment about the initialize vs modify comment on these spans - what do you think about moving the initialization of the `r_factors` `Vector` here and the `r_distances` `Vector` later to somewhere outside of these functions? My goal is to remove some of the implicit ordering that we have for the methods, since in theory aside from the mask step, any of the multiplicative ones could go in whatever order (outside of performance reasons to do more broad strokes first)

I think you have a good point. But I wanted to avoid the overhead of initializing the values only to set them later. The "best" way to do that would be to track whether the factors have been set yet for each function and have an assignment and multiplication code path for every new loop. But that doesn't seem worth it, not yet at least. I thought a reasonable compromise was having one common function that goes first. Maybe once we have a better way to do performance testing we can change this.

For now I renamed calc_mesh_hide_and_mask to fill_factor_from_hide_and_mask, hopefully that clarifies things.

Also, I think eventually this order won't exist in too many places, I'm guessing it will just be called from intermediate function.

I think you have a good point. But I wanted to avoid the overhead of initializing the values only to set them later. The "best" way to do that would be to track whether the factors have been set yet for each function and have an assignment and multiplication code path for every new loop. But that doesn't seem worth it, not yet at least. I thought a reasonable compromise was having one common function that goes first. Maybe once we have a better way to do performance testing we can change this. For now I renamed `calc_mesh_hide_and_mask` to `fill_factor_from_hide_and_mask`, hopefully that clarifies things. Also, I think eventually this order won't exist in too many places, I'm guessing it will just be called from intermediate function.
}
}
void calc_front_face(const float3 &view_normal,
const Span<float3> vert_normals,
const Span<int> verts,
const MutableSpan<float> factors)
{
BLI_assert(verts.size() == factors.size());
for (const int i : verts.index_range()) {
const float dot = math::dot(view_normal, vert_normals[verts[i]]);
factors[i] *= dot > 0.0f ? dot : 0.0f;
HooglyBoogly marked this conversation as resolved Outdated

Apply the same BLI_assert as above in this function?

Apply the same `BLI_assert as above` in this function?

I tried removing the asserts. They're redundant since each span access has an assert too. And they're a bit verbose.

I tried removing the asserts. They're redundant since each span access has an assert too. And they're a bit verbose.

Just my 2 cents - I think them being redundant is fine, to me the asserts at this level provide two benefits:

  1. Gives more contextual information on this line with the sizes being unequal than later when it's just a span index out of range in cases where we're debugging an issue
  2. Documents the preconditions that we expect to be true for these methods

Of the two, I think the latter is more valuable as with the different sized spans that we're dealing with in these different methods, having it laid out makes reading this code in the future more clear

Just my 2 cents - I think them being redundant is fine, to me the asserts at this level provide two benefits: 1. Gives more contextual information on this line with the sizes being unequal than later when it's just a span index out of range in cases where we're debugging an issue 2. Documents the preconditions that we expect to be true for these methods Of the two, I think the latter is more valuable as with the different sized spans that we're dealing with in these different methods, having it laid out makes reading this code in the future more clear
}
}
void calc_distance_falloff(SculptSession &ss,
const Span<float3> positions,
const Span<int> verts,
const eBrushFalloffShape falloff_shape,
const MutableSpan<float> r_distances,
const MutableSpan<float> factors)
{
BLI_assert(verts.size() == factors.size());
BLI_assert(verts.size() == r_distances.size());
SculptBrushTest test;
const SculptBrushTestFn sculpt_brush_test_sq_fn = SCULPT_brush_test_init_with_falloff_shape(
ss, test, falloff_shape);
for (const int i : verts.index_range()) {
if (factors[i] == 0.0f) {
r_distances[i] = FLT_MAX;
continue;
}
if (!sculpt_brush_test_sq_fn(test, positions[verts[i]])) {
factors[i] = 0.0f;
r_distances[i] = FLT_MAX;
continue;
}
r_distances[i] = math::sqrt(test.dist);
}
}
void calc_brush_strength_factors(const SculptSession &ss,
const Brush &brush,
const Span<int> verts,
const Span<float> distances,
const MutableSpan<float> factors)
{
BLI_assert(verts.size() == distances.size());
BLI_assert(verts.size() == factors.size());
const StrokeCache &cache = *ss.cache;
for (const int i : verts.index_range()) {
HooglyBoogly marked this conversation as resolved Outdated

Minor nit - the comment below makes me think that we should compare distances[i]to FLT_MAX instead

Minor nit - the comment below makes me think that we should compare `distances[i]`to `FLT_MAX` instead
if (factors[i] == 0.0f) {
/* Skip already masked-out points, as they might be outside of the brush radius and be
* unaffected anyway. Having such large values in the calculations below might lead to
* non-finite values, leading to undesired results. */
continue;
}
const float hardness = sculpt_apply_hardness(ss, distances[i]);
const float strength = BKE_brush_curve_strength(&brush, hardness, cache.radius);
factors[i] *= strength;
}
}
void calc_brush_texture_factors(SculptSession &ss,
const Brush &brush,
const Span<float3> vert_positions,
const Span<int> verts,
const MutableSpan<float> factors)
{
BLI_assert(verts.size() == factors.size());
const int thread_id = BLI_task_parallel_thread_id(nullptr);
const MTex *mtex = BKE_brush_mask_texture_get(&brush, OB_MODE_SCULPT);
if (!mtex->tex) {
return;
}
for (const int i : verts.index_range()) {
float texture_value;
float4 texture_rgba;
/* NOTE: This is not a thread-safe call. */
sculpt_apply_texture(
ss, brush, vert_positions[verts[i]], thread_id, &texture_value, texture_rgba);
factors[i] *= texture_value;
}
}
void apply_translations(const Span<float3> translations,
const Span<int> verts,
const MutableSpan<float3> positions)
{
BLI_assert(verts.size() == translations.size());
for (const int i : verts.index_range()) {
const int vert = verts[i];
positions[vert] += translations[i];
}
}
HooglyBoogly marked this conversation as resolved Outdated

BLI_assert(translations.size() == verts.size()) here?

`BLI_assert(translations.size() == verts.size())` here?
void apply_crazyspace_to_translations(const Span<float3x3> deform_imats,
const Span<int> verts,
const MutableSpan<float3> translations)
{
BLI_assert(verts.size() == translations.size());
for (const int i : verts.index_range()) {
translations[i] = math::transform_point(deform_imats[verts[i]], translations[i]);
}
}
HooglyBoogly marked this conversation as resolved
Review

Same assert as above

Same assert as above
void clip_and_lock_translations(const Sculpt &sd,
const SculptSession &ss,
const Span<float3> positions,
const Span<int> verts,
const MutableSpan<float3> translations)
{
BLI_assert(verts.size() == translations.size());
const StrokeCache *cache = ss.cache;
if (!cache) {
HooglyBoogly marked this conversation as resolved
Review

Same assert as above

Same assert as above
return;
}
for (const int axis : IndexRange(3)) {
if (sd.flags & (SCULPT_LOCK_X << axis)) {
for (float3 &translation : translations) {
translation[axis] = 0.0f;
}
continue;
}
if (!(cache->flag & (CLIP_X << axis))) {
continue;
}
const float4x4 mirror(cache->clip_mirror_mtx);
const float4x4 mirror_inverse = math::invert(mirror);
for (const int i : verts.index_range()) {
const int vert = verts[i];
/* Transform into the space of the mirror plane, check translations, then transform back. */
float3 co_mirror = math::transform_point(mirror, positions[vert]);
if (math::abs(co_mirror[axis]) > cache->clip_tolerance[axis]) {
continue;
}
/* Clear the translation in the local space of the mirror object. */
co_mirror[axis] = 0.0f;
const float3 co_local = math::transform_point(mirror_inverse, co_mirror);
translations[i][axis] = co_local[axis] - positions[vert][axis];
}
}
}
MutableSpan<float3> mesh_brush_positions_for_write(SculptSession &ss, Mesh & /*mesh*/)
{
/* TODO: Eventually this should retrieve mutable positions directly from the mesh or the active
* shape key, instead of keeping a mutable reference to an array stored in the PBVH. That will
* help avoid copies when user edits don't affect positions, will help to make code safer because
* there will be less potentially-stale state, and will make it less confusing by avoiding
* redundant data storage. */
return BKE_pbvh_get_vert_positions(*ss.pbvh);
}
void flush_positions_to_shape_keys(Object &object,
const Span<int> verts,
const Span<float3> positions,
const MutableSpan<float3> positions_mesh)
{
Mesh &mesh = *static_cast<Mesh *>(object.data);
KeyBlock *active_key = BKE_keyblock_from_object(&object);
if (!active_key) {
return;
}
MutableSpan active_key_data(static_cast<float3 *>(active_key->data), active_key->totelem);
/* For relative keys editing of base should update other keys. */
if (bool *dependent = BKE_keyblock_get_dependent_keys(mesh.key, object.shapenr - 1)) {
/* TODO: Avoid allocation by using translations already calculated by brush. */
Array<float3> offsets(verts.size());
for (const int i : verts.index_range()) {
offsets[i] = positions[verts[i]] - active_key_data[verts[i]];
}
int i;
LISTBASE_FOREACH_INDEX (KeyBlock *, other_key, &mesh.key->block, i) {
if ((other_key != active_key) && dependent[i]) {
MutableSpan<float3> data(static_cast<float3 *>(other_key->data), other_key->totelem);
apply_translations(offsets, verts, data);
}
}
MEM_freeN(dependent);
}

XXX is typically used to indicate something that needs to be solved ASAP, possibly even prior to anding a patch. Is it the case here? Or is it more of a regular TODO?

`XXX` is typically used to indicate something that needs to be solved ASAP, possibly even prior to anding a patch. Is it the case here? Or is it more of a regular `TODO`?

There are currently 318 cases of XXX: in Blender's code, haha, but it's good to know what it means anyway. This is a todo that will get easier to resolve as we refactor the remainder of the brushes.

There are currently 318 cases of `XXX:` in Blender's code, haha, but it's good to know what it means anyway. This is a todo that will get easier to resolve as we refactor the remainder of the brushes.
/* Modifying of basis key should update mesh. */
if (active_key == mesh.key->refkey) {
/* TODO: There are too many positions arrays getting passed around. We should have a better
* naming system or not have to constantly update both the base shape key and original
* positions. OTOH, maybe it's just a consequence of the bad design of shape keys. */
apply_translations(positions, verts, positions_mesh);
}
/* Apply new positions to active shape key. */
for (const int vert : verts) {
active_key_data[vert] = positions[vert];
}
}
} // namespace blender::ed::sculpt_paint

View File

@ -23,6 +23,7 @@
#include "BKE_paint.hh"
#include "BKE_pbvh_api.hh"
#include "mesh_brush_common.hh"
#include "paint_intern.hh"
#include "sculpt_intern.hh"
@ -587,6 +588,38 @@ float factor_get(const Cache *automasking,
return automasking_factor_end(ss, automasking, vert, mask);
}
static void mesh_orig_vert_data_update(SculptOrigVertData &orig_data, const int vert)
{
if (orig_data.unode->type == undo::Type::Position) {
orig_data.co = orig_data.coords[vert];
orig_data.no = orig_data.normals[vert];
}
else if (orig_data.unode->type == undo::Type::Color) {
orig_data.col = orig_data.colors[vert];