Details

Reviewers

Bastien Montagne (mont29)
Brecht Van Lommel (brecht)
Jeroen Bakker (jbakker)

Maniphest Tasks

T74205: Crash in sculpt mode at trying to rotate after a cancelled rotate

Commits

rB1a69384e76e2: Fix T74205: crash cancelling transfrom operation in sculpt mode

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 6949
Build 6949: arc lint + arc unit

Event Timeline

Asad-ullah Khan (kh4n) created this revision.Mar 4 2020, 6:20 AM

Harbormaster completed remote builds in B6928: Diff 22356.Mar 4 2020, 6:20 AM

Asad-ullah Khan (kh4n) mentioned this in T74205: Crash in sculpt mode at trying to rotate after a cancelled rotate .Mar 4 2020, 6:22 AM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Mar 4 2020, 12:24 PM

Ankit Meel (ankitm) added a task: T74205: Crash in sculpt mode at trying to rotate after a cancelled rotate .

Ankit Meel (ankitm) added reviewers: Bastien Montagne (mont29), Jeroen Bakker (jbakker).

Ankit Meel (ankitm) added projects: BF Blender, Sculpt, Paint & Texture.

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?

https://wiki.blender.org/wiki/Tools/CodeReview#Ingredients_of_a_Patch

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

Fixed comment style

Harbormaster completed remote builds in B6948: Diff 22385.Mar 4 2020, 5:14 PM

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

Fixing bad diff

Harbormaster completed remote builds in B6949: Diff 22386.Mar 4 2020, 5:19 PM

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

Ankit Meel (ankitm) mentioned this in T74571: Crash when aborting sculpt face set creation with Esc.Mar 9 2020, 12:51 PM

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.Mar 9 2020, 6:58 PM

This revision now requires changes to proceed.Mar 9 2020, 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.