Page MenuHome

Fix T77328: Crash on undo Draw Face Sets stroke with dyntopo active

Authored by Pablo Dobarro (pablodp606) on Jun 4 2020, 12:45 AM.



Draw Face Sets does not work in Dyntopo and the sculpt API should be
responsible for that without needing to add checks all over the code,
but it was doing an undo push of type SCULPT_UNDO_FACE_SETS which is not
supported, causing the crash.

Diff Detail

rB Blender

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Jun 4 2020, 12:45 AM
Pablo Dobarro (pablodp606) created this revision.

Dyntopo and the sculpt API should be responsible for that without needing to add checks all over the code

Exactly my thoughts. So intuitively this belongs to sculpt_undo.c which takes care of all mode-specific logic and does proper thing.

So why is this solved on the "tool" level rather than on sculpt undo level? You can have a no-op placeholder there with a TODO.


Usually comment answers "why", not :"what". Surely, in some cases of non-trivial optimized code having answer for "what" question adds a lot of value, and sometimes it also helps to split up code visually.

Here it definitely deserved to be a "why" answer, since it's clear that you skip undo for dyntopo, but it is not clear why.

But also read non-inlined comment.

Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Review update

I clarified the comment about why the check is done there. I also tried to put the check inside the SCULPT_undo_push_node function and it seems to work fine, but this seems safer for adding the fix to the LTS. When vertex colors are added they are going to have the same problem, so probably the undo code needs to be refactored to check for unsupported types and multiple types in the same node

If it's aimed for LTS, then it indeed sounds safer. And indeed undo needs some love.

Now, since you're mentioning LTS, @Jeroen Bakker (jbakker) coordinates that. Not sure if he wants to do extra checks now. At a very least he gets a gentle poke now :)

This revision is now accepted and ready to land.Jun 10 2020, 9:40 AM