Decoupled scene undo implementation [contextual scenes+resources]

[ator-dev]

Describe the project you are working on

• The Godot editor.
• Any Godot project, especially those with many complex scenes.

Describe the problem or limitation you are having in your project

I believe the single UndoRedo history stack used by the main Godot editor is a major limitation of the engine, since it violates its separate/modular design, discourages editing of multiple/many scenes despite this being one of its strengths, and causes frequent confusion and irritation with users (based on my research, but only my opinion of course). I've discussed undo with the community on many occasions at this point, and every time this issue is brought up without any mention of it by myself. See this poll for example; I made it to gather opinions on whether the system is unstable, and most commentors focused on this problem and its impact on general usability.

This is closely linked with #1630 and #2109, but I think a separate proposal for discussion about this type of implementation is beneficial, since the changes in my upcoming PR are very significant (e.g. large, and potentially breaking plugins). API changes are unavoidable regardless of the approach, but this could be done in a more object-oriented style by having code get an UndoRedoContext object and calling the same methods on that. Even more object-oriented is using one UndoRedo object per stack/context, instead of having one-or-many stacks in each UndoRedo object, and getting these UndoRedo objects directly for the methods to be called on.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

My implementation uses one stack per resource or scene. One special-case stack is the project settings editor [NOTE: currently non-functional because I haven't found a method to get the focused viewport yet], but the system is highly extendible (with very little new code per context type) so infinitely many 'contexts' can be flexibly created. A 'context' will only stop working if a major API change takes place, or if a huge change in the internals happens that changes the nature of how that class operates; this breakage will be immediately obvious and require a one-line fix.

In my opinion changes to resources should be undoable separately from the scenes they are in, and these changes should then apply everywhere as they will not affect scene history. This works well with the main concept of shared resources, allowing different data areas/types (e.g. structure vs visuals) to be properly separated.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

See this PR for my current implementation. This is very liable to change depending on what people think of it however; I'm willing to change how bits of it work or even re-do the entire thing depending on other people's arguments.

EditorNode:

• A helper method get_edited_object_id has been created, which returns the object ID of the object considered selected/edited. Contexts are as follows:
  • The ID of the resource open and focused in the Inspector.
  • The ID of the resource open in the focused dock, such as AnimationTrackEditor; every dock this can applied to must be separately 'registered' for querying its API.
  • The ID of the open and focused editor window such as ProjectSettings, if any; these must similarly be registered.
  • If selection is elsewhere, the ID of the root node of the currently open scene.
• In the constructor, a one-line call per special-case context is added, enabling specific open docks and windows to be recognised and considered.

UndoRedo:

• Any functions now have a parameter for the undo context (an object ID relevant to the context), as detailed above, although I've temporarily disabled some functions and will implement them later if this is considered a good approach.

EditorInspectorDock:

• Modified to be focusable with a click and to draw a blue 'selected' border, as do similar docks. I have also submitted a PR specifically for this, but it's particularly important that the change is present here. I predict that if my approach to contextual undo is taken, larger UI additions would make it clear what is currently being 'worked on' (thus what undo/redo would apply to), and in which situations a separate stack is used.

Other classes/files:

• Minor changes in a few files; these will mainly be necessary where the appropriate method for getting the edited object isn't bound, so ClassDB::bind_method() must be called.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This is an inherent flaw of the editor's undo-related systems. I hope that my implementation can work for Godot or be developed into something that will be accepted, but if nothing else this should prompt more useful discussion about the issue. I genuinely believe this is the right approach for the editor, but keep in mind I am completely open to other suggestions or viewpoints; I am not emotionally invested in this system and don't mind discarding it for something more fitting, which I would also be willing to develop/PR.

Is there a reason why this should be core and not an add-on in the asset library?

UndoRedo's behaviour and API are significantly changed by contextual undo (as they will be by any implementation of per-scene undo).

[Mickeon]

It's a lot of technical jargon, but I'd take anything better than the current undo/redo implementation. I've lost progress as a result of minor, but cumolative flaws and inconsistencies enough times.

[KoBeWi]

I'm actually also working on UndoRedo rework right now, but my approach won't involve much (or any) modifications to UndoRedo, but rather introduce a new EditorUndoRedoManager class that will keep multiple UndoRedo objects and pick them based on context. I don't know yet if it will even work though.

[ator-dev]

@KoBeWi Thanks for letting me know - I was also wondering if that approach would work after you informed me of it earlier. Could you message me if you get that to work or make significant progress on it please?

As mentioned in the PR, it's the overall concept that it focuses on, not how stacks are accessed; so if one UndoRedo per stack works well, I could make a new PR using the same system except with that method used in the backend instead.

[KoBeWi]

Here it is: godotengine/godot#59564
Completely broken, but there is a part that somewhat works.

[lyuma]

Honestly my biggest issue with the proposal is it does not address how edits to resources (from the inspector or panels such as animation tracks etc) are mixed with edits to the scene, and how the user can undo changes they make e.g. to materials, themes and other resources.

There was a survey, but I am not convinced the issue is a design problem. In my experience, the biggest problem isn't the architecture, it's bugs. When I hit undo, it is likely to skip some resources I edited and not undo them; or if I go too far, it will undo something I changed way back.

I don't see how making it per-scene is going to help solve that. Unless I can get a good explanation how Godot will behave if I do:

• Go to Scene A,
• Change an object's position,
• Change a material property (Albedo color) on a cube
• Go to Scene B,
• Add a sphere
• Assign that same material on a Sphere
• modify that material's texture
• Go back to scene A
• change albedo color again.
• Go to scene B
• Undo undo undo undo

And given that each scene gets its own history, it gets even more funky if you think about nested sub-scenes, especially if you save in the middle and the patch to auto-update referenced scenes is applied:

• Embed scene C into Scene A
• Switch to Scene C,
• add a new node to the scene
• Go to scene A
• reference that node in a NodePath
• change the position of that new node.
• now change its rotation
• Undo once.
• Go to scene C
• Undo
• Go to scene A
• Redo

I can imagine all sorts of other pathological cases which. Mixing scene edits with resource edits, and combining these with undo and redo in the middle. I can't figure out in my head what any of the above sequences would do if this proposal is implemented, because the separate scenes are not actually entirely isolated in practice.

In the ideal status quo, containing a single linear timeline, it is fairly easy for me to explain what should happen if I made 5 changes over a 3 minutes and then undo 5 times: it will revert the entire project to a state from 3 minutes ago. There is no corruption or confusing states. This is what Godot's present state should be. However, it is not, because there are bugs.

The issue I see today, is if I make a change to an animation or resource, there's a 50% chance that hitting undo will switch tabs and not revert the change I made. If my experience is uncommon, I will try to do better to record what I change so that I can reproduce these and file bugs. I believe that I am not the only Godot user with this experience, and my guess is that is why users replied the way they did on the survey.

[ator-dev]

does not address how edits to resources (from the inspector or panels such as animation tracks etc) are mixed with edits to the scene

The current idea in my proposal (and what I have implemented in the PR) is that an action context is detected for every action logged to undo/redo.

• For example, if add_do_method is called with an Animation resource, the context for the action being created is determined to be that resource, and so a stack is created/found for this resource and the action added to that.
• The current context is what the user is editing at the time of e.g. pressing Ctrl+Z or Ctrl+Y[/Shift+Z]. If the AnimationTrackEditor dock has focus (is selected/clicked), the current context is detected to be the Animation open in this dock, and so the action is taken from the stack associated with that particular resource.
• Similarly, if the Inspector is focused and a resource (instead of a node) is open in that, this is detected as the current context. The default current context is the open scene.

I understand this is not the main point of your comment, however. I'll address that now, but wanted to make sure the idea behind the system is clear first.

the biggest problem isn't the architecture, it's bugs. When I hit undo, it is likely to skip some resources I edited

That's interesting, thanks for letting me know. Though I was a frequent user of Godot about a year ago, it's changed a lot in that time and it does seem like undo/redo has become significantly worse than my memory of it. I have actually been concerned with the way undo/redo is handled and the amount of effort it takes to maintain it (/issues/3714) but my PR which suggested one way of ensuring consistency [as long the object being edited is known], godotengine/godot/pull/56621, was rejected due to the decision that it had minimal gains.

I would appreciate if you could give me some examples of the cases in which these problems occur, but I imagine they are very intermittent and probably occur due to a combination of factors involving state, and possibly this isn't the best way to investigate the underlying issues. Perhaps a test suite for undo/redo that automatically creates and tests complex scenes would be the best way forwards? There wouldn't be a disadvantage as far as I can see; either it proves that undo/redo is stable (unlikely), shows where problems occur so that they can be identified and fixed continuously, or shows that undo/redo is too unstable to continue with the current approach, in which case a better new approach could be proposed.

Unless I can get a good explanation how Godot will behave if I do: [use case]

This is the kind of case my PR is intended for. Each scene and resource has its own stack, so while you have the material open and focused in the Inspector you are undoing/redoing changes to that (so e.g. setting the colour of everything using the material to red), and while the scene is focused you are changing the positions of nodes and assigning materials to them. A disadvantage of this approach is it could make undo/redo a bit more confusing, unless very clear context clues are provided - I can imagine a user not realising the material has to be focused, and wondering why they are undoing translations instead of colour changes. A scene at the same point in the stack could have red or white nodes without any undos/redos being applied specifically to the scene, but in my opinion this fits in with Godot's decoupled resource philosophy.

it gets even more funky if you think about nested sub-scenes

Thank you for bringing this up, this is something I hadn't considered; I forgot that sub-scenes can have editable children. I think the expected behaviour would be that there is some memory of the state of nodes/properties etc in these scenes, so that an undo will still apply but to an invisible object (maybe with a notification that this is happening). If the same object is later brought back, its state is restored. This would need some specific coding to make it work though, and in the current state of the PR it would probably crash. As for the node path, I think that would just be temporarily invalidated, but I should make sure it isn't doing anything weird.

separate scenes are not actually entirely isolated in practice

True; unfortunately if some form of per-scene undo is implemented, there will always have to be workarounds for this. I think I have a fairly good idea of what changes are needed, but it can never be perfect of course.

it is not, because there are bugs

Also a real problem, and one which sadly requires a more extensive test suite, such as specific tests for undo/redo. Godot's design with very granular interactions between modules makes bugs almost impossible to eliminate; unit testing is thus the best way to ensure congruency and consistency, particularly where a sensitive system like this is involved.

[ator-dev]

@KoBeWi I imagine you've probably got a lot to work on, but if you get the chance would you mind giving your own take on this please (and possibly stating how your PR addresses the main issues above)? Don't feel you need to give a really long answer of course - I'd just be interested to better understand the differences (e.g. benefits) with your approach, because the way I see it both our PRs will suffer from these problems.

[Zireael07]

A disadvantage of this approach is it could make undo/redo a bit more confusing, unless very clear context clues are provided - I can imagine a user not realising the material has to be focused, and wondering why they are undoing translations instead of colour changes. A scene at the same point in the stack could have red or white nodes without any undos/redos being applied specifically to the scene, but in my opinion this fits in with Godot's decoupled resource philosophy.

One way of solving this problem (that I bet I would run into, my eyes aren't the best and inspector focus hasn't mattered much so far unless you're a keyboard user) would be to display a list of recently done operations somewhere (e.g. upon clicking Undo in the menu) so that you see what's gonna get undoed.

[ator-dev]

display a list of recently done operations

Good idea, that could help - it could be a bit too cumbersome for many users though, undo for me is a spur-of-the-moment thing. Maybe there could be some kind of context clue that goes beside a focused element with an associated undo stack? (Potential to add some kind of text, e.g. on hover, that states the next and previous action)

[KoBeWi]

My implementation is going to work like I explained once in a comment: #2109 (comment)
It will also be rather easy to display the history, so the list from my concept image could be visible to user in some way.

[ator-dev]

My implementation is going to work like I explained once in a comment

Thanks, but looking back at this the explanation doesn't cover nested sub-scenes (e.g. with editable children), and it also needs to be considered how bugs in the overall system will be handled (i.e. so there isn't a crash or too much work lost). Do you have any thoughts on this?

[KoBeWi]

nested sub-scenes

The context would be still the current scene, because the sub-scenes themselves are not being modified (data is stored externally).

how bugs in the overall system will be handled [...] Do you have any thoughts on this?

Not yet.

[ator-dev]

the sub-scenes themselves are not being modified

That's true - but as mentioned by @lyuma, behaviour when undoing an edit (such as a translation) to a node in a nested scene that was since deleted from within that scene (with the scene then being saved) seems unclear and could cause a crash, since the node being changed no longer exists. I'm concerned that things which modify other scenes/resources could cause problems as well; for example, what about the AnimationPlayer which changes the state of nodes without causing any undo actions to be logged?
I'm really sorry to be a bother by the way, I just think this is a question that needs to be fully considered in order to make an implementation that makes sense for Godot...

[me2beats]

I think it would be fine to provide API to write editor custom undo/redo lists (like Blender default undo history list or even undo-redo slider like in Zbrush).
I tried to make it as a plugin (for Godot 3) but I faced with

• lacking of methods (get_version_count(), get_action_name())
• in case of undo slider: inability to undo actions per scene (an idea was to make an undo slider for level design).

but I also think that in some cases undoing in order of time still would be useful, for example I want to revert to the project stage that was 5min ago.

[ator-dev]

I think it would be fine to provide API to write editor custom undo/redo lists

Definitely it would be useful to have API methods that can support this. Maybe a next step after the main rework, or in its final stages? At the moment focus is on only the contextual side of the functionality, at least for me.

[Calinou]

This was implemented by godotengine/godot#59564 for 4.0, closing.