Sculpt: Start data-oriented refactor for draw brush #121835
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#121835
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Sergey/blender:sculpt_brush_refactor"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR establishes the beginning of the transition to data-oriented code
for sculpt brushes described in #118145. The final brush "API" design is
still in progress, and further iteration will be required as more brushes are
refactored and other areas can be cleaned up.
Currently the main goal is making the code paths more obvious and easing
future development, but this change itself may also give a performance
improvement. In a simple test with a large mesh, that was about 14%.
calc_fron_face
earlier in sequence ca627494d9@blender-bot build
@ -0,0 +1,207 @@
/* SPDX-FileCopyrightText: 2023 Blender Authors
Replace
2023
by2024
@ -0,0 +1,229 @@
/* SPDX-FileCopyrightText: 2023 Blender Authors
Replace
2023
by2024
@ -0,0 +1,18 @@
/* SPDX-FileCopyrightText: 2023 Blender Authors
Replace
2023
by2024
Just a first pass on the code so far for high level / stylistic stuff, haven't gone super in depth into either of the brush algorithms to compare to the existing code.
As a side note, once this is ready, should this wait until we have the v4.2 branch?
@ -0,0 +40,4 @@
const PBVHNode &node,
Object &object,
LocalData &tls,
const MutableSpan<float3> positions_sculpt,
r_
prefixes in this file for return args@ -0,0 +172,4 @@
const float3 offset = effective_normal * ss.cache->radius * ss.cache->scale * bstrength;
switch (BKE_pbvh_type(*object.sculpt->pbvh)) {
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.
@ -0,0 +10,4 @@
struct Object;
struct PBVHNode;
namespace blender::ed::sculpt_paint {
How about we add a new namespace here for brushes?
I tried this but didn't really like it. It conflicted a bit with more specific namespaces like
cloth
,pose
, andsmooth
. Such a large portion of the code is brushes that maybe it's not necessary? It also sort of relates to how thesculpt_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.@ -0,0 +12,4 @@
namespace blender::ed::sculpt_paint {
void do_draw_brush(const Sculpt &sd, Object &object, Span<PBVHNode *> nodes);
Documentation for both of these methods? Maybe just something about the difference between the regular
draw_brush
anddraw_vector_displacement_brush
?@ -0,0 +11,4 @@
#include "DNA_brush_enums.h"
/**
* This file contains common operations useful for the implementation of various different brush
Minor nit, I'm personally not a fan of
utils
as a suffix, it tends to lead to the file / namespace / class being a dumping ground of loosely related concepts instead of something more cohesive. That being said I don't think I have a good name for this right now - maybe something to do withfactor
anddisplacement
, since that seems to be the main type of output of these functions?Agreed, I didn't really like it either. It's meant to be a place for common methods shared between brushes, so I renamed it to
mesh_brush_common.hh
, which is a little better anyway. I don't think this organization is set in stone, I expect things to continue moving around as we figure out how to share these methods between brushes. We'll also find common themes too, which may give better ways to organize.@ -0,0 +58,4 @@
*/
void calc_mesh_hide_and_mask(const Mesh &mesh,
Span<int> vert_indices,
MutableSpan<float> r_factors);
Since
factors
comes up a fair bit in this file, I think it's worth defining what it is somewhere in the main header.@ -0,0 +66,4 @@
void calc_front_face(const float3 &view_normal,
Span<float3> vert_normals,
Span<int> vert_indices,
MutableSpan<float> factors);
factors
->r_factors
, this and elsewhere in the other methods.@ -0,0 +117,4 @@
* 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);
positions
->r_positions
here and elsewhere in the file@ -0,0 +128,4 @@
*/
void apply_crazyspace_to_translations(Span<float3x3> deform_imats,
Span<int> verts,
MutableSpan<float3> translations);
translations
->r_translations
here and elsewhere in the file@ -6282,0 +6310,4 @@
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 - mask[verts[i]];
Should this be
span[verts[i]]
instead? I don't see what's the point of line 6311 otherwise.@ -6282,0 +6325,4 @@
}
}
void calc_front_face(const float3 &view_normal,
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.
Just my 2 cents - I think them being redundant is fine, to me the asserts at this level provide two benefits:
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
@ -6282,0 +6418,4 @@
void apply_translations(const Span<float3> translations,
const Span<int> verts,
const MutableSpan<float3> positions)
{
BLI_assert(translations.size() == verts.size())
here?@ -6282,0 +6429,4 @@
const Span<int> verts,
const MutableSpan<float3> translations)
{
for (const int i : verts.index_range()) {
Same assert as above
@ -6282,0 +6440,4 @@
const Span<int> verts,
const MutableSpan<float3> translations)
{
const StrokeCache *cache = ss.cache;
Same assert as above
@ -1125,6 +1125,13 @@ float SCULPT_brush_strength_factor(
int thread_id,
const blender::ed::sculpt_paint::auto_mask::NodeData *automask_data);
void sculpt_apply_texture(const SculptSession *ss,
Documentation
@ -1127,1 +1127,4 @@
void sculpt_apply_texture(const SculptSession *ss,
const Brush *brush,
const float brush_point[3],
Is it worth changing this to
float3
andfloat r_rgba[4]
tofloat4
in this PR?I don't think so, that's a separate cleanup. I'm just exposing this function but this really shouldn't be the final state of things.
Sculpt: Initial data-oriented brush refactor for draw brushto Sculpt: Initial data-oriented refactor for draw brushThanks for the review!
For the
r_
prefixes, I don't actually think they apply here. I've always viewed those as a way to express that the values were initialized or otherwise "completely filled from scratch" by the function. For these functions, the existing values matter since we multiply with them for the final factors (factors[i] *= new_factor;
). So I don't really see it as a "return argument" but a reference to mutable data instead. I guess the difference is subtle. But I also think our much stronger const correctness makes the prefix less necessary too.Sculpt: Initial data-oriented refactor for draw brushto Sculpt: Start data-oriented refactor for draw brushI get your intent now, I think the thing that tripped me up on first reading was that it just looked like a missed formatting on the first instance in the
calc_mesh_hide_and_mask
call. I do agree that the const correctness makes the naming less necessary.Some further thoughts after the last round of review.
@ -6282,0 +6308,4 @@
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]];
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.
@ -6282,0 +6312,4 @@
}
}
else {
r_factors.fill(1.0f);
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 ther_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
tofill_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.
@ -6282,0 +6368,4 @@
const StrokeCache &cache = *ss.cache;
for (const int i : verts.index_range()) {
if (factors[i] == 0.0f) {
Minor nit - the comment below makes me think that we should compare
distances[i]
toFLT_MAX
instead@ -0,0 +16,4 @@
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);
In the future, new/ported brushes should be added here?
Yep, we've opted out from having per-brush header.
@ -0,0 +86,4 @@
/**
* Modify the factors based on distances to the brush cursor, using various brush settings.
*/
void calc_brush_strength_factors(const SculptSession &ss,
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?
It is indeed a bit tricky difference. The
calc_distance_falloff
only depends on the brush position. Thecalc_brush_strength_factors
does more things like hardness, curve etc.Splitting those into two functions basically:
@blender-bot build
@ -6270,0 +6512,4 @@
/* Modifying of basis key should update mesh. */
if (active_key == mesh.key->refkey) {
/* XXX: There are too many positions arrays getting passed around. We should have a better
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 regularTODO
?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.