Page MenuHome

Fix T74205: manually adjust op_undo_depth to account for sculpt mode undo differences
Needs ReviewPublic

Authored by Asad-ullah Khan (kh4n) on Wed, Mar 4, 6:20 AM.

Details

Summary

Fix T74205. Intended to to fix the assertion error caused by having step_init != NULL by modifying the undo depth to match what sculpt mode undo expects. Typically, this value (wm->op_undo_depth) is 1 while doing a cancel (canceling operation with right click/escape), however sculpt mode (which handles its own undos) needs this value to be zero. The reason it is not zero is because of line 1988 of blender/source/blender/windowmanager/intern/wm_event_system.c (wm_handler_operator_call) which is:

if (ot->flag & OPTYPE_UNDO) {
        wm->op_undo_depth++;
}

Alternative solution, which also "works" but has a worse user experience (have to hit ctrl+z multiple times, and probably other problems as well):

Essentially sculpt mode does all its ops differently (which is fine because it handles it own stuff) but for rotates etc. it borrows the other ones, which all set OPTYPE_UNDO, which it explicitly doesn't:

/* Flags (sculpt does own undo? (ton)). */
ot->flag = OPTYPE_BLOCKING;

line 8355 blender/source/blender/editors/sculpt_paint/sculpt.c SCULPT_OT_brush_stroke
Adding OPTYPE_UNDO and removing condition from sculpt_undo_push_end(void) is another solution, but then you have to hit Ctrl+Z a few times before it actually undoes anything :/

Diff Detail

Repository
rB Blender
Branch
master
Build Status
Buildable 7167
Build 7167: arc lint + arc unit

Event Timeline

Ankit (ankitm) added a subscriber: Ankit (ankitm).EditedWed, Mar 4, 2:28 PM

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments what is there in the comments, right now suits the description here. Also, take some notes your previous comments from the task. see my next comment

I will fix the comment format, but I don't follow that last part 😅 do you mean I should add some of my previous notes into the comments?

Asad-ullah Khan (kh4n) edited the summary of this revision. (Show Details)Wed, Mar 4, 5:13 PM

Fixed comment style

OH no I have made a mistake....fixing it shortly

Asad-ullah Khan (kh4n) edited the summary of this revision. (Show Details)Thu, Mar 5, 4:16 AM

With this patch, does it do two undo pushes rather than one? Or is the undo push that is automatically done by the transform operator somehow skipped in this case?

If it does only one undo push, then we could do the equivalent of this, but without incrementing/decrementing the undo depth I think. Instead ED_sculpt_end_transform should pass a parameter to SCULPT_undo_push_end to skip the wm->op_undo_depth test.

But maybe the simpler solution is to add a transform operator for sculpt tools specifically (in transform_ops.c), without the OPTYPE_UNDO flag?

Brecht Van Lommel (brecht) requested changes to this revision.Mon, Mar 9, 6:58 PM
This revision now requires changes to proceed.Mon, Mar 9, 6:58 PM

Hmm I don't believe this skips or does any additional undo pushes. All it does is set op_undo_depth to a value that sculpt_undo_push_end expects. By doing this, the stack_init variable is cleared, preventing an assertion failure down the road

Passing an additional parameter to sculpt_undo_push_end will also work (to skip the undo depth check).
I also believe that adding an additional set of transform ops for sculpt will work, but I have not tested it.
Yet another solution is to make sculpt mode use OPTYPE_UNDO somehow, but in my experience this resulted in odd behavior, and I am assuming there is a reason sculpt mode does its things differently.

These are the options I see, aside from this patch which has the advantage of being the least obtrusive, but not much else 😅

You're right, there is no double undo. I'm not sure how that works exactly, but ok.

I suggest to pass a parameter explicitly to sculpt_undo_push_end then.

You might want to make a SCULPT_undo_push_end_ex(const bool skip_nested_undo), which is then called by SCULPT_undo_push_end() with true, just so the many calls to SCULPT_undo_push_end() don't need to be changed.

Now using sculpt_undo_push_end_ex

Sigh, I have mucked it up...give me a sec

Jeroen Bakker (jbakker) resigned from this revision.Thu, Mar 26, 9:45 AM

I don't know the details of the undo system yet, so best that Bastien and Brecht handle this patch