Page MenuHome

Regression: Sculpt mode undo: Broken when undoing to a Global/memfile undo step after rB4c7b1766a7f1
Closed, ResolvedPublicBUG

Description

System Information
Operating system: Win 10

Blender Version
Broken: blender-2.92.0-564f3be20a6f-windows64
Caused by rB4c7b1766a7f1: Fix undo steps not allowing re-using old BMain in non-global undo.

Short description of error
In the sculpting template, the Undo (ctrl+z) function doesn't undo the first stroke.

However, if you then click "Redo" (shift+ctrl+z), it will actually undo the first stroke... This is a separated issue, see T82532: Sculpt Redo does not always properly apply the first stroke of the sculpt session

Same issue cause a slightly different bug when one switch from Object mode to Sculpt mode, draws a stroke, then undo twice: the object does not go back to Object mode as expected.

Exact steps for others to reproduce the error

  1. File/New/Sculpting
  2. Perform a brush stroke
  3. Undo (ctrl+z) - It should undo the brush stroke, but nothing happens.

Redo (shift+ctrl+z) - Now it will undo the brush stroke.

OR

  1. Default startup file, switch to sculpt mode.
  2. Draw a stroke.
  3. Undo twice.

Even not using the sculpting template, you can still replicate this erratic behavior in sculpt mode, if you load a texture/perform a brush stroke and try to undo... the undo won't work, but the Redo will undo the brush stroke...

Event Timeline

Philipp Oeser (lichtwerk) renamed this task from Sculpt mode: Unexpected undo behavior to Sculpt mode: Unexpected undo behavior in Sculpting Template.Nov 4 2020, 9:23 AM
Philipp Oeser (lichtwerk) changed the task status from Needs Triage to Confirmed.
Philipp Oeser (lichtwerk) changed the subtype of this task from "Report" to "Bug".

Ah thanks for the report, actually discovered that yesterday, and have been trying to understand what exactly is going on with it (part of the issue also comes from rB1cb07530a956, but reverting this does not fully fix it either).

Note that another symptom of the same issue is that from startup file, if you switch to Sculpt mode, undo will stay in sculpt mode (instead of going back to Object mode).

This is actually fairly involved to fix... Summoning some more brains here.

Root of the issue: Since initial memfile undo step is the only one existing, none of its IDs are flagged as 'changed', so when undoing we just fully re-use current IDs, hence the lack of undoing of the first sculpt action. Other modes like Paint ones are not affected because they do store a memfile undo step on entering, but for sculpt this was changed to a sculpt undo step in rB1cb07530a956.

Only quick solution I can see is to revert rB4c7b1766a7f1, but this will break most of the work done for 'preserving tool settings across undos' (since it relies on re-using existing data-blocks). So I'm not sure which issue would be considered the more critical right now?

For the more long term solution, we can try to be smart, and e.g 'remove' modes that are not supported by memfile undo when reading one of those steps. We already do it for Edit mode, this snippets adds Sculpt to the party. It solves part of the issue, but not all (especially when already starting in Sculpt mode, we still have a memfile undo step at the beginning in sculpt mode, so this is not working here).

1diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
2index c2a18033b44..09e953b6ec6 100644
3--- a/source/blender/blenloader/intern/readfile.c
4+++ b/source/blender/blenloader/intern/readfile.c
5@@ -3673,7 +3673,7 @@ static void direct_link_object(BlendDataReader *reader, Object *ob)
6 /* For undo we want to stay in object mode during undo presses, so keep some edit modes
7 * disabled.
8 * TODO: Check if we should not disable more edit modes here? */
9- ob->mode &= ~(OB_MODE_EDIT | OB_MODE_PARTICLE_EDIT);
10+ ob->mode &= ~(OB_MODE_EDIT | OB_MODE_PARTICLE_EDIT | OB_MODE_ALL_SCULPT);
11 }
12
13 BLO_read_data_address(reader, &ob->adt);
14@@ -5655,7 +5655,7 @@ static void read_libblock_undo_restore_identical(
15 }
16 /* For undo we stay in object mode during undo presses, so keep editmode disabled for re-used
17 * data-blocks too. */
18- ob->mode &= ~OB_MODE_EDIT;
19+ ob->mode &= ~(OB_MODE_EDIT | OB_MODE_PARTICLE_EDIT | OB_MODE_ALL_SCULPT);
20 }
21 }
22
23diff --git a/source/blender/editors/undo/memfile_undo.c b/source/blender/editors/undo/memfile_undo.c
24index 2df26abe8b3..03c9b8272c6 100644
25--- a/source/blender/editors/undo/memfile_undo.c
26+++ b/source/blender/editors/undo/memfile_undo.c
27@@ -36,6 +36,7 @@
28 #include "BKE_lib_query.h"
29 #include "BKE_main.h"
30 #include "BKE_node.h"
31+#include "BKE_paint.h"
32 #include "BKE_scene.h"
33 #include "BKE_undo_system.h"
34
35@@ -219,6 +220,17 @@ static void memfile_undosys_step_decode(struct bContext *C,
36 bmain, id, memfile_undosys_step_id_reused_cb, NULL, IDWALK_READONLY);
37 }
38
39+ /* Sculpt mode has its own undo system, so when we get back from it into object mode,
40+ * clearing `ob->mode` flag is not enough, we also need to clean up the sculpt session data,
41+ * otherwise the object does not go back properly in Object mode. */
42+ if (GS(id->name) == ID_OB) {
43+ Object *ob = (Object *)id;
44+ if (ob->sculpt != NULL) {
45+ BKE_sculptsession_free(ob);
46+ DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE);
47+ }
48+ }
49+
50 /* Tag depsgraph to update data-block for changes that happened between the
51 * current and the target state, see direct_link_id_restore_recalc(). */
52 if (id->recalc) {

We can also make all those 'editing' modes to actually go into last memfile undo step, find the memchunks for the data-block(s) they will be editing, and set their is_identical_future to false. I think this is the 'proper' solution, but not sure how involved it will be to implement it.

renamed this task from Sculpt mode: Unexpected undo behavior to Sculpt mode: Unexpected undo behavior in Sculpting Template.

Actually, even not using the sculpting template, you can still replicate this erratic behavior in sculpt mode, if you load a texture/perform a brush stroke and try to undo... the undo won't work, but the Redo will undo the brush stroke...

Philipp Oeser (lichtwerk) renamed this task from Sculpt mode: Unexpected undo behavior in Sculpting Template to Sculpt mode: Unexpected undo behavior.Nov 4 2020, 6:05 PM
Philipp Oeser (lichtwerk) updated the task description. (Show Details)

@Bastien Montagne (mont29), I don't really understand how the bug report and your description relate. Why would undoing the first stroke be affected by memfile undo or mode switching undo at all? Why doesn't it just undo the stroke with sculpt mode undo and nothing else?

I can imagine that if switching sculpt mode bypasses memfile undo for performance, some additional code would be needed to ensure the sculpt session is initialized properly. But I guess the steps to reproduce that issue are different than what is in the bug report description.

When a file is opened in sculpt mode as in the sculpting template, I would not expect there to be an undo step to go from sculpt to object mode that the user can actually do.

rB1cb07530a956: Fix T71564: Undo stroke lags after entering sculpt mode is also strange. It calls SCULPT_undo_push_begin, but that is supposed to go along with a call to SCULPT_undo_push_end once the operator is done. I don't know what it means to call just begin, maybe that indirectly does something useful, but it's not how this is supposed to be used.

@Bastien Montagne (mont29), I don't really understand how the bug report and your description relate. Why would undoing the first stroke be affected by memfile undo or mode switching undo at all? Why doesn't it just undo the stroke with sculpt mode undo and nothing else?

I can imagine that if switching sculpt mode bypasses memfile undo for performance, some additional code would be needed to ensure the sculpt session is initialized properly. But I guess the steps to reproduce that issue are different than what is in the bug report description.

When a file is opened in sculpt mode as in the sculpting template, I would not expect there to be an undo step to go from sculpt to object mode that the user can actually do.

Those both have the same cause, symptoms are different because when doing new -> sculpting, the initial memfile undo step stored is already in sculpt mode, so we do not have an initial, unchanged sculpt mode undo step, unlike when starting default blender and then switching to sculpt mode.

Here is the undo stack after opening default blender and switching to sculpt and doing one stroke:

Undo 3 Steps (*: active, #=applied, M=memfile-active, S=skip)
[*#M ]   0 {0x60c000319c08} type='Global Undo', name='Original'
[    ]   1 {0x60d000636218} type='Sculpt', name='Sculpt Mode'
[    ]   2 {0x60d00066e588} type='Sculpt', name='Draw Brush'

And here is the undo stack after doing new -> sculpting and doing one stroke:

Undo 2 Steps (*: active, #=applied, M=memfile-active, S=skip)
[*#M ]   0 {0x60c000468288} type='Global Undo', name='Original'
[    ]   1 {0x60d0002c71b8} type='Sculpt', name='Draw Brush'

rB1cb07530a956: Fix T71564: Undo stroke lags after entering sculpt mode is also strange. It calls SCULPT_undo_push_begin, but that is supposed to go along with a call to SCULPT_undo_push_end once the operator is done. I don't know what it means to call just begin, maybe that indirectly does something useful, but it's not how this is supposed to be used.

Yes, it is strange, and I'm not sure I understand it properly either, but think that that SCULPT_undo_push_begin simply ensures that the undo step stored at the end of this sculpt_mode_toggle_exec operator (by generic code in WM area) is actually a SCULPT undo step, and not a global, memfile one?

Those both have the same cause, symptoms are different because when doing new -> sculpting, the initial memfile undo step stored is already in sculpt mode, so we do not have an initial, unchanged sculpt mode undo step, unlike when starting default blender and then switching to sculpt mode.

Right, but I'm not sure why such an initial, unchanged sculpt mode undo step would be needed. As far as I know the 'Draw Brush' undo step should contain the old vertex coordinates to be restored, and that should be sufficient to perform undo.

Those both have the same cause, symptoms are different because when doing new -> sculpting, the initial memfile undo step stored is already in sculpt mode, so we do not have an initial, unchanged sculpt mode undo step, unlike when starting default blender and then switching to sculpt mode.

Right, but I'm not sure why such an initial, unchanged sculpt mode undo step would be needed. As far as I know the 'Draw Brush' undo step should contain the old vertex coordinates to be restored, and that should be sufficient to perform undo.

That would be the case, if the mesh ID's memchunks were properly tagged with is_identical_future being set to True.

But currently, that flag is only set when writing the next memfile undo step, and a difference is detected. So we'd need the first Sculpt undo step to do that as well I think.

It seems we are talking past each other. I don't know why you are talking about memchunks when my point was that for this bug report, no memfile or memchunks should be relevant. If they are relevant, please explain why.

If there is some other sequence of steps that you are doing a bugfix for, please explain what those steps are.

As already said, memfile/memchunks are the issue, since in both cases they are the first initial undo step, the ones that fail to apply properly:

Those both have the same cause, symptoms are different because when doing new -> sculpting, the initial memfile undo step stored is already in sculpt mode, so we do not have an initial, unchanged sculpt mode undo step, unlike when starting default blender and then switching to sculpt mode.

Here is the undo stack after opening default blender and switching to sculpt and doing one stroke:

Undo 3 Steps (*: active, #=applied, M=memfile-active, S=skip)
[*#M ]   0 {0x60c000319c08} type='Global Undo', name='Original'
[    ]   1 {0x60d000636218} type='Sculpt', name='Sculpt Mode'
[    ]   2 {0x60d00066e588} type='Sculpt', name='Draw Brush'

And here is the undo stack after doing new -> sculpting and doing one stroke:

Undo 2 Steps (*: active, #=applied, M=memfile-active, S=skip)
[*#M ]   0 {0x60c000468288} type='Global Undo', name='Original'
[    ]   1 {0x60d0002c71b8} type='Sculpt', name='Draw Brush'

Anyway, will be at the office tomorrow, easier to sort this in person (and also hope to have a working proper fix by then to review)

Bastien Montagne (mont29) renamed this task from Sculpt mode: Unexpected undo behavior to Regression: Sculpt mode undo: Broken when undoing to a Global/memfile undo step after rB4c7b1766a7f1.Nov 9 2020, 2:42 PM
Bastien Montagne (mont29) updated the task description. (Show Details)
Bastien Montagne (mont29) updated the task description. (Show Details)

Hi @Bastien Montagne (mont29) not sure where to post this, but I just found another situation where this undo issue is still happening , this time involving the multires.... But it's easy to reproduce....

  • File/New/Sculpting
  • Add a multires modifier
  • Subdivide once
  • Perform a brush stroke
  • Try to undo (nothing happens)
  • Now try to redo (it will undo the brush stroke)

(blender-2.92.0-3e325c35f870-windows64)