Page MenuHome

Fix T71434: Rebuild PBVH only on sculpt geometry changes
Needs ReviewPublic

Authored by Pablo Dobarro (pablodp606) on Sat, Nov 9, 5:18 PM.
Tokens
"Like" token, awarded by threedslider."Like" token, awarded by ebarranco."Love" token, awarded by roman13."Like" token, awarded by Frozen_Death_Knight."Like" token, awarded by xrg."Love" token, awarded by ghfujianbin."Mountain of Wealth" token, awarded by Brandon777."Like" token, awarded by amonpaike.

Details

Summary

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

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.

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.

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.

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.