Implement unique undo option for operators
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Nov 4 2016, 5:19 PM.

Details

Summary

This option makes an operator not be stored in the undo stack if the previous stored elemen is the same operator.

The main usage is for animation, so you can change frames to inspect the
poses, and revert the previous pose without having to roll back tons of
"change frame" operator, or even see the undo stack full.

This complements rB13ee9b8e
Design by Sergey Sharybin.

Diff Detail

Repository
rB Blender
Dalai Felinto (dfelinto) retitled this revision from to Implement unique undo option for operators.Nov 4 2016, 5:19 PM
Dalai Felinto (dfelinto) updated this object.

Technical seems ok,

Not sure about the naming 'unique'. Other software like osx, qt try to solve this by grouping. But introducing such a mechanism is more than the ticket you are trying to solve. But in naming would prefer 'group together' as that is the functionality it does. I can imagine that that will be more future proof.

But in naming would prefer 'group together'

Right, OPTYPE_UNDO_GROUPED may be more descriptive.

For reference, mypaint uses a kind of action (stroke) grouping, defined by the time elapsed between two strokes, could be something to investigate.

This seems like a step in the right direction.

However, unless I'm mistaken, there are a few potential issues still:

  1. You only do an undo push for the first in a sequence. This means though that you may still have the annoying "frame reset problem" if you spend ages scrubbing back and forth to find the perfect frame to set a key on, try to set a pose, make a mistake, and hit undo. Because the last stored undo step was the one at the start of that sequence and not the final result of that scrubbing sequence, the current frame may end up changing after undoing.
  1. Different operators may be used as part of the same conceptual operation. For instance, the animator's previewing sequence may look something like: "Jump to closest keyframe, scrub forward, scrub back, scrub back and forth around around another keyframe, jump to next keyframe". In the animator's eyes, this sequence is probably a "single operation/step" but because there are several different operators involved, it will end up being 3-4 different operators instead.
  1. Also, how well would something like this work well for the text editor and console?

As @Hadrien Brissaud (hadrien) suggested, an approach that takes some temporal considerations into account may work better in this case. So, some options I was thinking about back when this issue last came up were:
i) Make the frame step/etc. operators modal. So basically, you have this single "super" operator that can do any kind of frame changing behaviour when invoked from the keyboard (using one of the existing keys). After each keypress, it waits for a "time-delay" period to see if the next keypress is one that it recognises. If so, it executes the relevant frame-change and keeps listening for the next one. Otherwise, if it's an unrecognised key or the timer times out first, it exits immediately and lets that key get handled. This way, there will be a single undo push for each sequence of scrubbing actions.

ii) Have a way of tagging "related operators" that will get chunked together with that operator if they appear together in the undo stack. This is more like a hybrid of the previous idea and what you have in this patch. It would be a more general solution that may also apply to things like the Text Editor and Console.

Is this planed to work in all blender areas or exclusively in pose mode?

If this concerns all blender operators I don't think this is good idea. Thing is blender incorporates multiple workflows and majority of them consist tweaking and repeatable operations. They will be internally considered as group but users understand them as individual steps and always want to handle them separately, i.e revert to last good step. Except this timeline scrubbing.

This change will completely break workflow in all painting modes and edit mode. Also cause major problems all areas that incorporate values tweaking (Properties, Node editor etc.). Some examples:

Edit mode. I translate vertex with big proportional falloff, then invoke operator again, reduce falloff size and tweak tip of the shape. Same operator, distinct actions. Witch this patch undo will revert me to flat plane.

Painting (all modes, 3D or 2D). Last stroke goes wrong. Doing undo with this change will rewind all of my work to undefined moment. Might be 2 strokes or 100.

Either this grouping will be disabled in all blender areas except Pose mode or we find different solution.

@Bartosz Moniewski (monio): From the looks of things, this is something we will enable on specific operators on an "as-needed" basis. So, for most operations (e.g. standard transforms and stuff like that), this is grouping behaviour isn't likely to be enabled.

@Joshua Leung (aligorith) That's a relief. ;)

I case of your second point. We can manually define group keyword in particular operators. For example "timeline_scrub" for change_frame, frame_offset etc. Then undo all subsequent operators within a specific group.

To solve "frame reset problem" I propose to add new shortcut to separate Individual Undo and Group Undo. SHIFT+CTRL+ALT+Z is unoccupied. Question is which behavior should use it, maybe Group Undo? It this case we can expand this idea to other blender areas (even painting modes or mesh edit talked before).

Seems like a step in the right direction, but agree we should ideally merge more than just operators with the same name.

  1. You only do an undo push for the first in a sequence. This means though that you may still have the annoying "frame reset problem" if you spend ages scrubbing back and forth to find the perfect frame to set a key on, try to set a pose, make a mistake, and hit undo. Because the last stored undo step was the one at the start of that sequence and not the final result of that scrubbing sequence, the current frame may end up changing after undoing.

We could do the undo push every time, and then optionally merge it with the previous one. BLO_memfile_merge() might work to merge the UndoElems at least, but there's more undo stacks.

then you could make color picker use 1 undo instead of 2... or prevent image slot switching to fill the undo stack

I can see why this can be useful. I also agree about the raised issues though. IMHO it's fine to commit this now (or at least I don't have a strong opinion), a "real" solution would likely be a bigger project.

  • Merge remote-tracking branch 'origin/master' into arcpatch-D2330
  • From review: _UNIQUE > _GROUPED
  • Final touch ups: group all the frame changes operators together

Hi, I've read it all, thanks for the feedback. It's hard to draw a line on when to hold on to a feature in order to have a more complete (though time consuming) solution. Anyways, for some specific replies:

@Joshua Leung (aligorith) :

  1. *You only do an undo push for the first in a sequence.* This means though that you may still have the annoying "frame reset problem" if you spend ages scrubbing back and forth to find the perfect frame to set a key on, try to set a pose, make a mistake, and hit undo. Because the last stored undo step was the one at the start of that sequence and not the final result of that scrubbing sequence, the current frame may end up changing after undoing.

You are right, this is what would happen. That said, I don't believe "finding the perfect frame to set a key on" is that critical. And if it's the case, you can easily redo take a look at the frame number, and undo again. @Hjalti Hjálmarsson (hjalti) any thoughts on that drawback?

We could also add the operator twice to prevent that (so always the first and the last). How about that?

  1. *Different operators may be used as part of the same conceptual operation*.

It is thinking about your specific example, that I did the latest changes for the patch. Basically I unified all the frame change operators into a single UNDO call.

  1. *Also, how well would something like this work well for the text editor and console?*

As in, how could we use this to make UNDO remove an entire word/line as oppose to one letter at a time? I think only the modal approach you suggest would address that.

@Bartosz Moniewski (monio) :

Is this planed to work in all blender areas or exclusively in pose mode?

It works in handy-picked areas where we think this would make sense. The patch as it is only affects time changing operators.

@lower case (lowercase) :

then you could make color picker use 1 undo instead of 2... or prevent image slot switching to fill the undo stack

Good points. Yes I think those cases would fit well within this feature.

  • Merge remote-tracking branch 'origin/master' into arcpatch-D2330
  • From review: fix prototype

Thinking loud: would it be more clear to do following:

  • When we want operator undo to be "grouped" or "merged" we tag it with OPTYPE_UNDO_GROUPED flag.
  • By default we use operator's identifier for the group name
  • If we want to group more than one operator type together, we can specify operator group in operator template.

This way we'll have:

static void SCREEN_OT_keyframe_jump(wmOperatorType *ot)
{
  /* ... */
  ot->flag = OPTYPE_UNDO_GROUPED;
  ot->undo_group = "FRAME_CHANGE";
  /* ... */

}

Benefit is that:

  • We always know what's gonna to be de-duplicated/grouped/merged.
  • We don't have to do manual undo pushes.

Also, either i'm missing something or the patch does not implement "merge" policy discussed above? Is it something left for the future or did we decide not to do it?

@Sergey Sharybin (sergey) I really like your suggestion (op->undo_group).

Also, either i'm missing something or the patch does not implement "merge" policy discussed above? Is it something left for the future or did we decide not to do it?

The time condition for grouping as suggested by @Joshua Leung (aligorith) was indeed not implemented. That would broad the scope of the proposed changes further than I originally planned. So not sure if it would be something I would pursuit/implement myself.

  • From review: use ot->undo_group
Bastien Montagne (mont29) requested changes to this revision.Nov 15 2016, 10:48 AM

Besides minor points noted in comments below, patch LGTM from quick look.

Except for one crucial missing point imho: this new undo_group feature is not implemented in python (see rna_wm.c)!

source/blender/blenkernel/intern/blender_undo.c
322–328

BKE_undo_get_name() returns NULL if no undo element is found, a bit inconsistent not to have same behavior here?

source/blender/editors/util/undo.c
222

picky not only operator, non-op code may also trigger undo - and it’s not necessarily same op, can be different op from same undo group ;)

This revision now requires changes to proceed.Nov 15 2016, 10:48 AM
  • From review: change BKE_undo_get_last() for consistency
  • From review: expose functionality for rna

I added the "undo_group" to both MacroOperator and Operator

All of @Bastien Montagne (mont29) 's concerns are now addressed.

This revision is now accepted and ready to land.Nov 15 2016, 2:09 PM
This revision was automatically updated to reflect the committed changes.