I don't know why we were doing this in previous Blender versions, were
geometry changes in sculpt mode were not supported. This is producing
major performance issues in several sculpt mode tools when working with
high poly meshes.
Maybe this change is too risky for 2.81
Details
- Reviewers
Jeroen Bakker (jbakker) Sergey Sharybin (sergey) - Maniphest Tasks
- T71434: Sculpt Performance Regression in 2.81
Diff Detail
- Repository
- rB Blender
- Branch
- sculpt-pbvh-update-objects (branched from master)
- Build Status
Buildable 5622 Build 5622: arc lint + arc unit
Event Timeline
Please dig deeper to understand what is going on, this seems like a hack that will break things.
The bug report says this is a recent regression, so it seems logical to start by identifying which change caused it.
The PBVH rebuilds after changing the brush size were introduced in 79b703bb635e
These rebuilds were also causing problems with the voxel size property control and when saving files, and that is not related to the commit.
What is the intended purpose of the BKE_sculpt_update_object_before_eval function? The only reasons to do rebuild are running a SCULPT_UNDO_GEOMETRY operation, applying a modifier or changing the frame, otherwise you just need to update the bounding boxes and the deformMats.
Are you sure they are not related? They sound like the kind of problems that this commit can cause.
What is the intended purpose of the BKE_sculpt_update_object_before_eval function?
BKE_sculpt_update_object_before_eval is called when the mesh object is (re-)evaluated by the dependency graph. Avoiding the PBVH rebuild would only solve part of the performance problem, there are other costly parts to re-evaluation. Changing brush properties should not cause a mesh object to be re-evaluated at all.
The only reasons to do rebuild are running a SCULPT_UNDO_GEOMETRY operation, applying a modifier or changing the frame, otherwise you just need to update the bounding boxes and the deformMats.
That's not a complete list of operators that can modify meshes in object/sculpt mode, and not taking into account arbitrary add-ons that may change mesh topology.
There are certainly cases where a PBVH rebuild can be avoided, and the correct solution may be in more fine grained depsgraph tagging. But clearly there is a regression here that should be solved first.
Are you sure they are not related? They sound like the kind of problems that this commit can cause.
Yes, that is happening since the first implementation of the voxel remesher in the sculpt branch. This is also why all sculpt tools need ss->pbvh checks to avoid crashing.
Ok, you'll have to describe what those problems are in more detail then. Flushing the PBVH could be improved to not require a PBVH rebuild, since really the PBVH can stay the same, just in some cases it might need to updated some pointers perhaps.
But for this specific regression, flushing the PBVH and updating the node bounds is not cheap either on high poly meshes. In principle there is no need to do it for changing a brush size.
| source/blender/editors/sculpt_paint/sculpt_undo.c | ||
|---|---|---|
| 1295 | This part isn't clear to me. When the undo geometry is created the sculpt session is requested to rebuild the pbvh. I would not have expected that when calling this function. If this is always needed we should rename the function if this is not always needed we should split the function. | |