griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix: Sculpt draw brush doesn't apply positions with deformation

Browse Source 
Only the evaluated positions were changed when deform or subsurf
modifiers were active. This was caused by only writing to the
evaluated positions owned by the PBVH and assuming these were
aliased with the mesh positions.

There were also quite a few TODOs in the area related to an extra
positions array that was passed around that was conceptually
unnecessary. Brush deformations should be based on the evaluated
positions, but only the original positions should be written to.

This PR resolves those TODOs while resolving sculpting with deform
modifiers and shape keys. We do this by only writing to the original
mesh positions. For shape keys we use the existing translations from
the brush rather than recomputing them, and only copy the mesh
original positions to the active shape key.

This also makes it easier to remove the PBVH's copy of vertex
positions in the future.

Pull Request: https://projects.blender.org/blender/blender/pulls/122842
This commit is contained in:
Hans Goudey
2024-06-06 23:05:41 +02:00
committed by Hans Goudey
parent 44a1e6a219
commit e4ab6dd8de
4 changed files with 48 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
source/blender/editors/sculpt_paint/brushes/draw.cc
View File

@@ -43,8 +43,7 @@ static void calc_faces(const Sculpt &sd,
const PBVHNode &node,
Object &object,
LocalData &tls,
const MutableSpan<float3> positions_sculpt,
const MutableSpan<float3> positions_mesh)
const MutableSpan<float3> positions_orig)
{
SculptSession &ss = *object.sculpt;
StrokeCache &cache = *ss.cache;
@@ -84,8 +83,9 @@ static void calc_faces(const Sculpt &sd,
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);
apply_translations(translations, verts, positions_orig);
apply_translations_to_shape_keys(object, verts, translations, positions_orig);
apply_translations_to_pbvh(*ss.pbvh, verts, translations);
}
static void calc_grids(Object &object, const Brush &brush, const float3 &offset, PBVHNode &node)
@@ -201,8 +201,7 @@ void do_draw_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes)
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();
MutableSpan<float3> positions_orig = 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) {
@@ -214,8 +213,7 @@ void do_draw_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes)
*nodes[i],
object,
tls,
positions_sculpt,
positions_mesh);
positions_orig);
BKE_pbvh_node_mark_positions_update(nodes[i]);
}
});

20
source/blender/editors/sculpt_paint/brushes/draw_vector_displacement.cc
View File

@@ -64,8 +64,7 @@ static void calc_faces(const Sculpt &sd,
const PBVHNode &node,
Object &object,
LocalData &tls,
MutableSpan<float3> positions_orig,
MutableSpan<float3> mesh_positions)
MutableSpan<float3> positions_orig)
{
SculptSession &ss = *object.sculpt;
StrokeCache &cache = *ss.cache;
@@ -108,7 +107,8 @@ static void calc_faces(const Sculpt &sd,
}
apply_translations(translations, verts, positions_orig);
flush_positions_to_shape_keys(object, verts, positions_orig, mesh_positions);
apply_translations_to_shape_keys(object, verts, translations, positions_orig);
apply_translations_to_pbvh(*ss.pbvh, verts, translations);
}
static void calc_grids(Object &object, const Brush &brush, PBVHNode &node)
@@ -224,20 +224,12 @@ void do_draw_vector_displacement_brush(const Sculpt &sd, Object &object, Span<PB
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();
MutableSpan<float3> positions_orig = 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);
calc_faces(
sd, brush, positions_eval, vert_normals, *nodes[i], object, tls, positions_orig);
BKE_pbvh_node_mark_positions_update(nodes[i]);
}
});

37
source/blender/editors/sculpt_paint/mesh_brush_common.hh
View File

@@ -40,21 +40,10 @@ 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
* - positions_orig: Positions owned by the original mesh. Not the same as `positions_eval` 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 spatially contiguous chunks.
* are built for these values, then applied to `positions_orig`.
*/
/**
@@ -147,21 +136,21 @@ void clip_and_lock_translations(const Sculpt &sd,
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);
void apply_translations_to_shape_keys(Object &object,
Span<int> verts,
Span<float3> translations,
MutableSpan<float3> positions_mesh);
/**
* Currently the PBVH owns its own copy of deformed positions that needs to be updated to stay in
* sync with brush deformations.
* \todo This should be removed one the PBVH no longer stores this copy of deformed positions.
*/
void apply_translations_to_pbvh(PBVH &pbvh, Span<int> verts, Span<float3> positions_orig);
} // namespace blender::ed::sculpt_paint

55
source/blender/editors/sculpt_paint/sculpt.cc
View File

@@ -6686,58 +6686,49 @@ void clip_and_lock_translations(const Sculpt &sd,
}
}
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)
void apply_translations_to_shape_keys(Object &object,
const Span<int> verts,
const Span<float3> translations,
const MutableSpan<float3> positions_orig)
{
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);
if (active_key == mesh.key->refkey) {
for (const int vert : verts) {
active_key_data[vert] = positions_orig[vert];
}
}
else {
apply_translations(translations, verts, active_key_data);
}
/* 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);
apply_translations(translations, verts, data);
}
}
MEM_freeN(dependent);
}
}
/* 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);
void apply_translations_to_pbvh(PBVH &pbvh, Span<int> verts, const Span<float3> translations)
{
if (!BKE_pbvh_is_deformed(pbvh)) {
return;
}
/* Apply new positions to active shape key. */
for (const int vert : verts) {
active_key_data[vert] = positions[vert];
MutableSpan<float3> pbvh_positions = BKE_pbvh_get_vert_positions(pbvh);
for (const int i : verts.index_range()) {
const int vert = verts[i];
pbvh_positions[vert] += translations[i];
}
}