Undo - Current Status and Important Fixes Needed #83806

New Issue

Bastien Montagne · 2020-12-15T17:35:01+01:00

Bastien Montagne commented

2020-12-15 17:35:01 +01:00

While investigating #82851 (Undo bug while sculpting) I realized that current general undo code has several issues, some being fairly critical and not trivial to solve (as is, too involved for a mere bugfix).

Here is current view of the situation (as I understand it), and some draft proposals to fix things and hopefully reach a better general state for undo code.

Current Status and Issues

1- Some undo steps will never be undone

Some undo modes (Sculpt and Image ones afaict) have a specific way to store and process their undo steps, which results in the fact that when undoing, they actually need to process the next step, and not the one that is passed to them (see sculpt_undosys_step_decode_undo e.g.).

This is all good as long as we stay in a same undo step type, but as soon as the step is from another type (like a global memfile one e.g.), its apply function is fully unaware of this, and therefore that first sculpt undo step never gets processed (undone).

2 - Some undo modes have useless redundant and critically buggy code

Again, Sculpt and Image undo code has loops over previous/next steps that are both useless, and critically buggy as they assume said previous/next steps are of the same undotype.

I'd assume this is inherited from the time where undo stacks where separated, but in a single common undo stack this is both useless (since main undo code like BKE_undosys_step_undo_with_data_ex already ensures that all previous/next steps have been processed in proper order), and obviously broken (due to lack of undo step type checking).

3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps

Definition:

A absolute step can be evaluated as its own and generate a valid data state, regardless of any other steps in-between it and the current data.
A relative step depends on the neighbor steps' results, so going back to a specific step requires to sequentially apply all intermediary steps in-between it and the current data.

Currently, general undo system (in undo_system.c) assumes all steps to be relative (see e.g. the first loop in BKE_undosys_step_undo_with_data_ex). There are comments and pieces of code that seem to hint that absolute steps were supported, or intended to be supported at some point.

In addition, while memfile steps used to be absolute, it's no longer the case with new system for them either.

4 - Apparently unused GPencil undo code

GPencil undo code is tagged in ed_undo_step_impl as needing update to be properly integrated in new general undo stack, however it seems to never be used effectively. See #84703 (Modernize (or Remove) GPencil Undo System) for details.

...And more?

There are probably more issues more or less related to issues mentioned above (did not check our reports related to edits/memfile undos mismatches e.g.), but I think addressing those three points would already bring things to a saner state.

Proposals

1 - Some undo steps will never be undone

Think main undo system should deal with those cases. Would do it that way:

All step_decode callbacks should only ever process the step that is given to them.
We need a tag for undo types that require the step-after-target-one to be processed in undo case, so that main undo system knows how to deal with them, including within a mixed type of steps situation.
** E.g. when you undo from a sculpt step to a memfile one, you actually need to process both the sculpt undo step (exception case) and the memfile one.

2 - Some undo modes have useless redundant and critically buggy code

Those extra looping pieces of code should simply be removed imho. Afaict they are dead code from earlier versions of undo system?
Tried to get them actually do something in sculpt_undosys_step_decode_undo() in debug session, and in all cases those actually never do anything, besides setting step-to-be-processed as the next one instead of the target one e.g.

3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps

Suggestion is to simply always assume all steps are relative ones. This will simplify logic and make code easier to follow.

This does imply some extra work when undoing a lot of steps at once (through the undo history menu e.g.), however I think this situation is not that common? And one could argue that on very heavy files like production ones, doing fifty relative undo steps affecting only a few data-blocks may very well be much quicker than re-reading the whole .blend file from memory, with all the implied depsgraph rebuild, updates, etc.

While investigating #82851 (Undo bug while sculpting) I realized that current general undo code has several issues, some being fairly critical and not trivial to solve (as is, too involved for a mere bugfix). Here is current view of the situation (*as I understand it*), and some draft proposals to fix things and hopefully reach a better general state for undo code. ## Current Status and Issues ### 1- Some undo steps will never be undone Some undo modes (Sculpt and Image ones afaict) have a specific way to store and process their undo steps, which results in the fact that when undoing, they actually need to process the `next` step, and not the one that is passed to them (see `sculpt_undosys_step_decode_undo` e.g.). This is all good as long as we stay in a same undo step type, but as soon as the step is from another type (like a global memfile one e.g.), its apply function is fully unaware of this, and therefore that first sculpt undo step never gets processed (undone). ### 2 - Some undo modes have useless redundant and critically buggy code Again, Sculpt and Image undo code has loops over previous/next steps that are both useless, and critically buggy as they assume said previous/next steps are of the same undotype. I'd assume this is inherited from the time where undo stacks where separated, but in a single common undo stack this is both useless (since main undo code like `BKE_undosys_step_undo_with_data_ex` already ensures that all previous/next steps have been processed in proper order), and obviously broken (due to lack of undo step type checking). ### 3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps Definition: * A *absolute* step can be evaluated as its own and generate a valid data state, regardless of any other steps in-between it and the current data. * A *relative* step depends on the neighbor steps' results, so going back to a specific step requires to sequentially apply all intermediary steps in-between it and the current data. Currently, general undo system (in `undo_system.c`) assumes all steps to be relative (see e.g. the first loop in `BKE_undosys_step_undo_with_data_ex`). There are comments and pieces of code that seem to hint that absolute steps were supported, or intended to be supported at some point. In addition, while memfile steps used to be absolute, it's no longer the case with new system for them either. ### 4 - Apparently unused GPencil undo code GPencil undo code is tagged in `ed_undo_step_impl` as needing update to be properly integrated in new general undo stack, however it seems to never be used effectively. See #84703 (Modernize (or Remove) GPencil Undo System) for details. ### ...And more? There are probably more issues more or less related to issues mentioned above (did not check our reports related to edits/memfile undos mismatches e.g.), but I think addressing those three points would already bring things to a saner state. ## Proposals ### 1 - Some undo steps will never be undone Think main undo system should deal with those cases. Would do it that way: * All `step_decode` callbacks should only ever process the step that is given to them. * We need a tag for undo types that require the step-after-target-one to be processed in undo case, so that main undo system knows how to deal with them, including within a mixed type of steps situation. ** *E.g. when you undo from a sculpt step to a memfile one, you actually need to process both the sculpt undo step (exception case) and the memfile one.* ### 2 - Some undo modes have useless redundant and critically buggy code Those extra looping pieces of code should simply be removed imho. Afaict they are dead code from earlier versions of undo system? *Tried to get them actually do something in `sculpt_undosys_step_decode_undo()` in debug session, and in all cases those actually never do anything, besides setting step-to-be-processed as the next one instead of the target one e.g.* ### 3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps Suggestion is to simply always assume all steps are relative ones. This will simplify logic and make code easier to follow. This does imply some extra work when undoing a lot of steps at once (through the undo history menu e.g.), however I think this situation is not that common? And one could argue that on very heavy files like production ones, doing fifty relative undo steps affecting only a few data-blocks may very well be much quicker than re-reading the whole .blend file from memory, with all the implied depsgraph rebuild, updates, etc.

Bastien Montagne commented

2020-12-15 17:35:01 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Bastien Montagne commented

2020-12-15 17:35:01 +01:00

Added subscribers: @mont29, @brecht, @ideasman42, @dfelinto

Bastien Montagne commented

2020-12-15 17:42:32 +01:00

This task is mainly to see if other devs having some good knowledge of undo area see missing issues, or invalid analysis of those, and/or have other ideas about how to address them, before I start spending time in actual code refactor...

@ideasman42 am especially eager to have your feedback here, since iirc you were the last one to work on general undo system (among other things, merging all stacks into a single one?). ;)

This task is mainly to see if other devs having some good knowledge of undo area see missing issues, or invalid analysis of those, and/or have other ideas about how to address them, before I start spending time in actual code refactor... @ideasman42 am especially eager to have your feedback here, since iirc you were the last one to work on general undo system (among other things, merging all stacks into a single one?). ;)

TheRedWaxPolice commented

2020-12-15 19:44:22 +01:00

Added subscriber: @TheRedWaxPolice

Campbell Barton commented

2020-12-19 02:03:27 +01:00

Clarifications

Fairly sure image undo is no longer an issue, 151cc02b6f should have resolved the kinds of problems that still exist in sculpt mode.

If there are open bugs with image undo, could you link to them?

... Otherwise we can focus on sculpt undo.

re: Some undo modes have useless redundant and critically buggy code and Unclear half-not-implemented fully-not-working support of 'absolute' undo steps.

I would like to see more examples of this... mainly because it's unclear to me where the issues are.

Global undo used to be absolute In that you could read an undo step and it would load most data (except for images, handled separately, maybe some other rare exceptions).

Regarding Proposal

1 - Some undo steps will never be undone

This is fuzzy to me, if we need to handle special cases, OK, but I'm not sure which steps are not undone and the kinds of bugs this is causing currently.

2 - Some undo modes have useless redundant and critically buggy code

Can we confirm that this is only sculpt mode? - if that's the case. Then we're on the same page. Getting sculpt modes undo working at all was quite difficult given how it handles undo steps internally.

3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps

+1 to remove absolute undo steps. There are times speed things up, but my sense is that undo-history is not a common way to access undo. And we can accept some performance loss to keep the internal logic stable + working.

Note that we could always support ways of skipping some undo steps as an optimization later (if we want). For example - loading undo steps into the same edit-mesh is not very useful. However I dont see this as a priority... having working predictable undo is so much more important.

Comments...

Generally the proposal seems fine, although I'd like to be aware of bugs. In general my impression is undo is working well in that it's not crashing and users are not regularly running into problems. That's not to say there are no problems, just that overall it's reliable. OTOH, it's possible I miss some issues and am unaware of some bugs/problems.
I'd highly recommend using tests for any development in this area. It's far too easy to accidentally break things and not realize, especially when this is interactions with different undo systems.

Some breakages only show up in quite obscure cases... undo/redo between editors, switching modes... etc.

See: `../lib/tests/ui_simulate/run.py`, for the current undo tests.

Suggest to:

Double check the current tests cover the areas you intend to change/develop.
Create tests that expose existing bugs these changes intend to fix (handy for TDD too)

I can help with these tests if needed, if you can point me to bugs in undo, I don't mind to turn them into tests we can use to resolve issues, or if you want a hand in getting them setup, that's fine too.

### Clarifications - Fairly sure image undo is no longer an issue, 151cc02b6f should have resolved the kinds of problems that still exist in sculpt mode. ``` If there are open bugs with image undo, could you link to them? ``` ``` ... Otherwise we can focus on sculpt undo. ``` - re: **Some undo modes have useless redundant and critically buggy code** and **Unclear half-not-implemented fully-not-working support of 'absolute' undo steps**. ``` I would like to see more examples of this... mainly because it's unclear to me where the issues are. ``` ``` Global undo used to be absolute In that you could read an undo step and it would load most data (except for images, handled separately, maybe some other rare exceptions). ``` ---- ### Regarding Proposal > **1 - Some undo steps will never be undone** This is fuzzy to me, if we need to handle special cases, OK, but I'm not sure which steps are not undone and the kinds of bugs this is causing currently. > **2 - Some undo modes have useless redundant and critically buggy code** Can we confirm that this is only sculpt mode? - if that's the case. Then we're on the same page. Getting sculpt modes undo working at all was quite difficult given how it handles undo steps internally. > **3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps** +1 to remove absolute undo steps. There are times speed things up, but my sense is that undo-history is not a common way to access undo. And we can accept some performance loss to keep the internal logic stable + working. Note that we _could_ always support ways of skipping some undo steps as an optimization later (if we want). For example - loading undo steps into the same edit-mesh is not very useful. However I dont see this as a priority... having working predictable undo is so much more important. ---- ### Comments... - Generally the proposal seems fine, although I'd like to be aware of bugs. In general my impression is undo is working well in that it's not crashing and users are not regularly running into problems. That's not to say there are no problems, just that overall it's reliable. OTOH, it's possible I miss some issues and am unaware of some bugs/problems. - I'd highly recommend using tests for any development in this area. It's far too easy to accidentally break things and not realize, especially when this is interactions with different undo systems. ``` Some breakages only show up in quite obscure cases... undo/redo between editors, switching modes... etc. ``` ``` See: `../lib/tests/ui_simulate/run.py`, for the current undo tests. ``` ``` Suggest to: ``` - Double check the current tests cover the areas you intend to change/develop. - Create tests that expose existing bugs these changes intend to fix (handy for TDD too) I can help with these tests if needed, if you can point me to bugs in undo, I don't mind to turn them into tests we can use to resolve issues, or if you want a hand in getting them setup, that's fine too.

Bastien Montagne commented

2020-12-21 12:26:37 +01:00

I can mostly only repeat why I already say in task itself...

In #83806#1078454, @ideasman42 wrote:

Clarifications

Fairly sure image undo is no longer an issue, 151cc02b6f should have resolved the kinds of problems that still exist in sculpt mode.

If there are open bugs with image undo, could you link to them?

... Otherwise we can focus on sculpt undo.

No open bug afaik, but image undo code seems to use the exact same 'use next step instead of given one' process as sculpt mode, so would expect exact same issue of missing first undo step...

re: Some undo modes have useless redundant and critically buggy code and Unclear half-not-implemented fully-not-working support of 'absolute' undo steps.

I would like to see more examples of this... mainly because it's unclear to me where the issues are.

Again, why are internal undo/redo code of image and sculpt (sculpt_undosys_step_decode_undo etc.) looping over undo steps, when main system (BKE_undosys_step_undo_with_data_ex) already does it? Looks like dead code from before merge of the different undo stacks to me?

Regarding Proposal

1 - Some undo steps will never be undone

This is fuzzy to me, if we need to handle special cases, OK, but I'm not sure which steps are not undone and the kinds of bugs this is causing currently.

See #82851: Undo bug while sculpting. First sculpt step stored after a Memfile one will never be undone. This is not visible from user most of the time since switching to Sculpt mode generates it's own, initial 'do nothing' sculpt step, but if there are memfile undo steps within a sculpt session e.g., then you'll have that bug.

That's why I want to make main BKE_undosys_step_undo_with_data_ex aware of this requirement instead, so that it can deal properly with this case, and mode-specific undo code can just blindly always process the step passed to it, and nothing else. This is the only safe way to handle an heterogeneous undo stack imho. mode-specific callbacks should never have to access any other step than the one they are given.

2 - Some undo modes have useless redundant and critically buggy code

Can we confirm that this is only sculpt mode? - if that's the case. Then we're on the same page. Getting sculpt modes undo working at all was quite difficult given how it handles undo steps internally.

Again, think this is at least sculpt and image. At the very least, they both share the same process in sculpt_undosys_step_decode_undo and image_undosys_step_decode_undo, iterating over undo steps blindly assuming those are of their own type, without any checks (and afaict without any valid reason to do so anymore).

Comments...

Generally the proposal seems fine, although I'd like to be aware of bugs. In general my impression is undo is working well in that it's not crashing and users are not regularly running into problems. That's not to say there are no problems, just that overall it's reliable. OTOH, it's possible I miss some issues and am unaware of some bugs/problems.

I'd highly recommend using tests for any development in this area. It's far too easy to accidentally break things and not realize, especially when this is interactions with different undo systems.

Some breakages only show up in quite obscure cases... undo/redo between editors, switching modes... etc.

See: ../lib/tests/ui_simulate/run.py, for the current undo tests.

Suggest to:

Double check the current tests cover the areas you intend to change/develop.

Create tests that expose existing bugs these changes intend to fix (handy for TDD too)

I can help with these tests if needed, if you can point me to bugs in undo, I don't mind to turn them into tests we can use to resolve issues, or if you want a hand in getting them setup, that's fine too.

Indeed, starting with new tests seems the best thing to do here, will do that first.

I can mostly only repeat why I already say in task itself... > In #83806#1078454, @ideasman42 wrote: > ### Clarifications > > - Fairly sure image undo is no longer an issue, 151cc02b6f should have resolved the kinds of problems that still exist in sculpt mode. > > If there are open bugs with image undo, could you link to them? > > ... Otherwise we can focus on sculpt undo. No open bug afaik, but image undo code seems to use the exact same 'use next step instead of given one' process as sculpt mode, so would expect exact same issue of missing first undo step... > - re: **Some undo modes have useless redundant and critically buggy code** and **Unclear half-not-implemented fully-not-working support of 'absolute' undo steps**. > > I would like to see more examples of this... mainly because it's unclear to me where the issues are. Again, why are internal undo/redo code of image and sculpt (`sculpt_undosys_step_decode_undo` etc.) looping over undo steps, when main system (`BKE_undosys_step_undo_with_data_ex`) already does it? Looks like dead code from before merge of the different undo stacks to me? > ### Regarding Proposal > >> **1 - Some undo steps will never be undone** > > This is fuzzy to me, if we need to handle special cases, OK, but I'm not sure which steps are not undone and the kinds of bugs this is causing currently. See #82851: Undo bug while sculpting. First sculpt step stored after a Memfile one will **never** be undone. This is not visible from user most of the time since switching to Sculpt mode generates it's own, initial 'do nothing' sculpt step, but if there are memfile undo steps within a sculpt session e.g., then you'll have that bug. That's why I want to make main `BKE_undosys_step_undo_with_data_ex` aware of this requirement instead, so that it can deal properly with this case, and mode-specific undo code can just blindly always process the step passed to it, and nothing else. This is the only safe way to handle an heterogeneous undo stack imho. mode-specific callbacks should never have to access any other step than the one they are given. >> **2 - Some undo modes have useless redundant and critically buggy code** > > Can we confirm that this is only sculpt mode? - if that's the case. Then we're on the same page. Getting sculpt modes undo working at all was quite difficult given how it handles undo steps internally. Again, think this is at least sculpt and image. At the very least, they both share the same process in `sculpt_undosys_step_decode_undo` and `image_undosys_step_decode_undo`, iterating over undo steps blindly assuming those are of their own type, without any checks (and afaict without any valid reason to do so anymore). > ### Comments... > > - Generally the proposal seems fine, although I'd like to be aware of bugs. In general my impression is undo is working well in that it's not crashing and users are not regularly running into problems. That's not to say there are no problems, just that overall it's reliable. OTOH, it's possible I miss some issues and am unaware of some bugs/problems. > > - I'd highly recommend using tests for any development in this area. It's far too easy to accidentally break things and not realize, especially when this is interactions with different undo systems. > > Some breakages only show up in quite obscure cases... undo/redo between editors, switching modes... etc. > > See: `../lib/tests/ui_simulate/run.py`, for the current undo tests. > > Suggest to: > > - Double check the current tests cover the areas you intend to change/develop. > > - Create tests that expose existing bugs these changes intend to fix (handy for TDD too) > > I can help with these tests if needed, if you can point me to bugs in undo, I don't mind to turn them into tests we can use to resolve issues, or if you want a hand in getting them setup, that's fine too. Indeed, starting with new tests seems the best thing to do here, will do that first.

Bastien Montagne commented

2020-12-22 11:28:02 +01:00

Added new view3d_sculpt_with_memfile_step test demonstrating issue from #82532/T82851 in rBL62534

Added new `view3d_sculpt_with_memfile_step` test demonstrating issue from #82532/T82851 in rBL62534

StrictlyIncreasing commented

2020-12-26 11:45:23 +01:00

Added subscriber: @NoBlahBlah

StrictlyIncreasing commented

2020-12-26 11:45:23 +01:00

I can assure you guys that using undo history menu to undo multiple steps at once is an extremely common use case. Why? Because of 2 reasons

1). Undoing and then redoing between the same steps to check whether a change you made is a good one is pretty much a universal workflow, no one teaches you to work like this you learn this yourself, given just how universal this is you can almost call it human nature.

2). Sometimes or rather most of the times comparing the state of something (be it a mesh or scene etc etc...) immediately before and after a change is just not a fair comparison to make, it's like comparing apples to oranges , to make it a more fair comparison you need to make more than one change possibly even 10 to 20 changes before you can finally compare the current state with what it was many, many undo steps ago.

I can assure you guys that using undo history menu to undo multiple steps at once is an extremely common use case. Why? Because of 2 reasons 1). Undoing and then redoing between the same steps to check whether a change you made is a good one is pretty much a universal workflow, no one teaches you to work like this you learn this yourself, given just how universal this is you can almost call it human nature. 2). Sometimes or rather most of the times comparing the state of something (be it a mesh or scene etc etc...) immediately before and after a change is just not a fair comparison to make, it's like comparing apples to oranges , to make it a more fair comparison you need to make more than one change possibly even 10 to 20 changes before you can finally compare the current state with what it was many, many undo steps ago.

Chris Kohl commented

2020-12-28 03:24:22 +01:00

Added subscriber: @ckohl_art

Oliver Weissbarth commented

2020-12-29 10:47:25 +01:00

Added subscriber: @oweissbarth

Bastien Montagne commented

2020-12-29 11:46:46 +01:00

@NoBlahBlah it's not about removing ability to undo multiple steps at once, it's about removing half-finished, not-really-working that tries to optimize that specific scenario, but is not really working anyway currently.

@NoBlahBlah it's not about *removing* ability to undo multiple steps at once, it's about removing half-finished, not-really-working that tries to optimize that specific scenario, but is not really working anyway currently.

Bastien Montagne commented

2020-12-29 11:48:41 +01:00

FYI, quick-slapped a (horrible!) investigating patch to address the sculpt-step-not-properly-undone case, P1858: (An Untitled Masterwork)

diff --git a/source/blender/blenkernel/intern/undo_system.c b/source/blender/blenkernel/intern/undo_system.c
index 078c93532d9..fb2e80fccf2 100644
--- a/source/blender/blenkernel/intern/undo_system.c
+++ b/source/blender/blenkernel/intern/undo_system.c
@@ -207,7 +207,24 @@ static void undosys_step_decode(
   }
 
   UNDO_NESTED_CHECK_BEGIN;
-  us->type->step_decode(C, bmain, us, dir, is_final);
+  if (dir < 0) {
+    if (us->type == BKE_UNDOSYS_TYPE_SCULPT) {
+      if (us->next && us->next->type == BKE_UNDOSYS_TYPE_SCULPT) {
+        us->next->type->step_decode(C, bmain, us->next, dir, is_final);
+      }
+    }
+    else if (us->type != BKE_UNDOSYS_TYPE_SCULPT && us->next &&
+             us->next->type == BKE_UNDOSYS_TYPE_SCULPT) {
+      us->next->type->step_decode(C, bmain, us->next, dir, is_final);
+      us->type->step_decode(C, bmain, us, dir, is_final);
+    }
+    else {
+      us->type->step_decode(C, bmain, us, dir, is_final);
+    }
+  }
+  else {
+    us->type->step_decode(C, bmain, us, dir, is_final);
+  }
   UNDO_NESTED_CHECK_END;
 
 #ifdef WITH_GLOBAL_UNDO_CORRECT_ORDER
@@ -696,8 +713,8 @@ bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack,
 
     /* Handle accumulate steps. */
     if (ustack->step_active) {
-      UndoStep *us_iter = ustack->step_active;
-      while (us_iter != us) {
+      UndoStep *us_iter = ustack->step_active->prev;
+      while (us_iter && us_iter != us) {
         /* TODO:
          * - skip successive steps that store the same data, eg: memfile steps.
          * - or steps that include another steps data, eg: a memfile step includes text undo data.
diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.c b/source/blender/editors/sculpt_paint/sculpt_undo.c
index 9677152cf7e..d837e7c5c5d 100644
--- a/source/blender/editors/sculpt_paint/sculpt_undo.c
+++ b/source/blender/editors/sculpt_paint/sculpt_undo.c
@@ -1467,6 +1467,8 @@ static void sculpt_undosys_step_decode_undo(struct bContext *C,
                                             SculptUndoStep *us)
 {
   SculptUndoStep *us_iter = us;
+  sculpt_undosys_step_decode_undo_impl(C, depsgraph, us_iter);
+  return;
   while (us_iter->step.next && (us_iter->step.next->type == us_iter->step.type)) {
     if (us_iter->step.next->is_applied == false) {
       break;

.

It does fix the sculpt issue, but is obviously not a proper clean solution at all, and also break some other tests on the road... But think it demonstrate the problem on a code side of things.

FYI, quick-slapped a (horrible!) investigating patch to address the sculpt-step-not-properly-undone case, [P1858: (An Untitled Masterwork)](https://archive.blender.org/developer/P1858.txt) ``` diff --git a/source/blender/blenkernel/intern/undo_system.c b/source/blender/blenkernel/intern/undo_system.c index 078c93532d9..fb2e80fccf2 100644 --- a/source/blender/blenkernel/intern/undo_system.c +++ b/source/blender/blenkernel/intern/undo_system.c @@ -207,7 +207,24 @@ static void undosys_step_decode( } UNDO_NESTED_CHECK_BEGIN; - us->type->step_decode(C, bmain, us, dir, is_final); + if (dir < 0) { + if (us->type == BKE_UNDOSYS_TYPE_SCULPT) { + if (us->next && us->next->type == BKE_UNDOSYS_TYPE_SCULPT) { + us->next->type->step_decode(C, bmain, us->next, dir, is_final); + } + } + else if (us->type != BKE_UNDOSYS_TYPE_SCULPT && us->next && + us->next->type == BKE_UNDOSYS_TYPE_SCULPT) { + us->next->type->step_decode(C, bmain, us->next, dir, is_final); + us->type->step_decode(C, bmain, us, dir, is_final); + } + else { + us->type->step_decode(C, bmain, us, dir, is_final); + } + } + else { + us->type->step_decode(C, bmain, us, dir, is_final); + } UNDO_NESTED_CHECK_END; #ifdef WITH_GLOBAL_UNDO_CORRECT_ORDER @@ -696,8 +713,8 @@ bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack, /* Handle accumulate steps. */ if (ustack->step_active) { - UndoStep *us_iter = ustack->step_active; - while (us_iter != us) { + UndoStep *us_iter = ustack->step_active->prev; + while (us_iter && us_iter != us) { /* TODO: * - skip successive steps that store the same data, eg: memfile steps. * - or steps that include another steps data, eg: a memfile step includes text undo data. diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.c b/source/blender/editors/sculpt_paint/sculpt_undo.c index 9677152cf7e..d837e7c5c5d 100644 --- a/source/blender/editors/sculpt_paint/sculpt_undo.c +++ b/source/blender/editors/sculpt_paint/sculpt_undo.c @@ -1467,6 +1467,8 @@ static void sculpt_undosys_step_decode_undo(struct bContext *C, SculptUndoStep *us) { SculptUndoStep *us_iter = us; + sculpt_undosys_step_decode_undo_impl(C, depsgraph, us_iter); + return; while (us_iter->step.next && (us_iter->step.next->type == us_iter->step.type)) { if (us_iter->step.next->is_applied == false) { break; ``` . It does fix the sculpt issue, but is obviously not a proper clean solution at all, and also break some other tests on the road... But think it demonstrate the problem on a code side of things.

kursad k commented

2021-01-12 00:03:53 +01:00

Added subscriber: @kursadk

kursad k commented

2021-01-12 00:03:53 +01:00

I have had a case with undoing text edits inside the text editor also causes massive wait times with heavy scenes. I am wondering if it is possible to decouple the text editor changes from the actual scenes. This kind of slow down is extra unproductive.

Thanks

I have had a case with undoing text edits inside the text editor also causes massive wait times with heavy scenes. I am wondering if it is possible to decouple the text editor changes from the actual scenes. This kind of slow down is extra unproductive. Thanks

Pipeliner commented

2021-03-12 20:47:16 +01:00

Added subscriber: @Pipeliner

Campbell Barton commented

2021-04-09 01:36:44 +02:00

Again, Sculpt and Image undo code has loops over previous/next steps that are both useless, and critically buggy as they assume said previous/next steps are of the same undotype.

This comment isn't valid, added comment + assertion that this never happens, see: ab6e67767e

``` Again, Sculpt and Image undo code has loops over previous/next steps that are both useless, and critically buggy as they assume said previous/next steps are of the same undotype. ``` This comment isn't valid, added comment + assertion that this never happens, see: ab6e67767e

Jenny Brown commented

2021-12-18 19:38:48 +01:00

Added subscriber: @Jenny-Brown

Gilberto Rodrigues commented

2022-01-05 07:48:31 +01:00

Added subscriber: @Gilberto.R

RedMser commented

2022-07-01 15:46:44 +02:00

Added subscriber: @RedMser

Bastien Montagne commented

2022-07-13 16:13:00 +02:00

Changed status from 'Confirmed' to: 'Archived'

Bastien Montagne closed this issue

2022-07-13 16:13:00 +02:00

Bastien Montagne commented

2022-07-13 16:13:00 +02:00

Archiving this for now, it is no longer really relevant and have no time to update it to current status of undo code.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Undo - Current Status and Important Fixes Needed #83806

Current Status and Issues

1- Some undo steps will never be undone

2 - Some undo modes have useless redundant and critically buggy code

3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps

4 - Apparently unused GPencil undo code

...And more?

Proposals

1 - Some undo steps will never be undone

2 - Some undo modes have useless redundant and critically buggy code

3 - Unclear half-not-implemented fully-not-working support of 'absolute' undo steps

Clarifications

Regarding Proposal

Comments...

Clarifications

Regarding Proposal

Comments...