Add per-scene UndoRedo

[KoBeWi]

This is just a proof-of-concept, but looks promising so far:
(it's finished, just needs bugfixing)

Right now implemented only in scene tree dock and everything is completely broken xd It needs quite a bit work to finish this PR. But the idea is that there are minimal changes to usage of UndoRedo, everything is magically performed behind the scenes. Most of the changes right now come from the fact that EditorData returns a reference to UndoRedo, so it was accessed with . operator in some places and I had to change it.

How does this work:

• editor has a single global UndoRedo instance that everything uses
• the most common operations are creating actions and adding methods/properties
• the idea is to replace this single UndoRedo object with another object with the same interface
• almost every UndoRedo operation works on a specific object
• based on that object, it's possible to infer a "context"
  • for nodes, it's the current scene. You can't operate on nodes from another scene
  • for internal resources, it's their owning scene's path
  • for external resources and everything else, there's a special "global" context

When you undo, the editor will again infer the current context (i.e. the current scene) and undo actions done on that scene. The global actions are mixed-in, I'm planning to add timestamps to actions, so that scenes and global operations can work together.

As I said, the PR is heavily in progress. The new system is hacked together with the old one. I added a method get_undo_redo2(), so that I can test parts of the code with the new system. I will remove the old UndoRedo once everything is finished.

Also, this does not change the old UndoRedo. It will still be used, but internally by the central EditorUndoRedoManager object. There is a chance that this all might fail at some point (i.e. there will be something that can't be easily replaced), but so far the work is smooth.

Closes godotengine/godot-proposals#1630
Fixes #45601

[ator-dev]

Good job, I definitely prefer your approach of using multiple UndoRedo objects; far more clean and object-oriented compared to multiple stacks in one UndoRedo. Would it be alright for me to integrate this into my system?

My PR #59544 uses my approach to multiple stacks in the background, but this can be replaced with your EditorUndoRedoManager approach while keeping the parts of my idea that work well (in my opinion).

[KoBeWi]

Would it be alright for me to integrate this into my system?

Sure, whatever. I'm going to finish this anyway, so then we can just pick the implementation that works better.

EDIT:
Pushed more changes. CanvasItemEditor should now undo properly. I also changed animation editor, but trying to key a property crashes. I'll check that later.

[ator-dev]

One way in which our PRs differ is that you use a string (the file path?) to identify stacks, whereas I use the relevant ObjectID. Do you think the path string will work reliably enough for this usage? I'm just a bit concerned that it might stop working if something is saved to a different location, but maybe that makes it a different resource/scene anyway.

[KoBeWi]

I might need to change that, right now it doesn't work properly with empty scenes. But using ObjectID would not really help either, I think.

[ator-dev]

No, mine doesn't work for empty scenes either - it uses the ObjectID of the root node. My reason for preference is that an ObjectID is guaranteed to be unique, stay unique throughout the session, and not change for any given object (whereas I'm thinking that the string file path would change if a resource is saved somewhere else). There should also be a memory improvement - being an integer instead of a string - but of course this is negligible.

[ator-dev]

Just something I'd like to bring up - there are some things that (so far as I can tell) cannot be made to work correctly without additional contributor effort. Both of our approaches detect context automatically where possible, but in cases such as renaming a node there is nothing passed to UndoRedo which can be sensibly used to detect the node being modified.

For my approach I'm thinking of having contributors add an extra line to pass in an object which can be used to detect context, such as the node being renamed, and you seem to use a similar approach with getting and passing in an UndoRedo object.

Are we happy with the effect of this on Godot development, and can it be mitigated? In most cases UndoRedo will work the same as before, but as soon as someone changes/adds an action so that context isn't easily detectable, it creates an opportunity for undo/redo to mysteriously fail in the future. This can't be tested for because undo/redo is all created on the fly.

[KoBeWi]

EditorUndoRedoManager can take an optional context object in create_action() and if not provided it will guess the context from the first action. The worst thing that can happen is some scene-related action registered in global context. This will need some testing.

btw inspector and most editors are finished (still need fixing ofc).

[ator-dev]

EditorUndoRedoManager can take an optional context object in create_action() and if not provided it will guess the context from the first action

I see, that's what I'm doing as well, except that don't have a global context (at least in my current approach). I have a fail condition for if no context could be detected and none was passed in, which I think might help eliminate some confusion.

inspector and most editors are finished

Well done, sounds like it's going well (;
Would you mind trying mine out as well once you've got the time, in order to compare them? I was thinking we could take the best ideas from both PRs, since there are aspects of yours that I think work better than mine, and vice-versa. I'll also do some more extensive testing on yours; I'm a bit caught up in work right now, but I've got a holiday in a week so will have plenty of time then.

[KoBeWi]

Almost finished with moving stuff to the new system. Note that I'm just replacing UndoRedo * with Ref. Surprisingly it works in many cases, but lots of stuff is likely broken.

Would you mind trying mine out as well once you've got the time, in order to compare them?

I can check when I finish this.

I'll also do some more extensive testing on yours;

I'd wait until I mark the PR as ready for review.

EDIT:
Pushed more changes. Context is now based on "edited scene id" instead of String (bruh).
EditorData has internal EditedScene structure. Every opened scene tab has associated EditedScene and it (hopefully!) persists if the tabs are moved around, root is deleted or file is moved, so I added id to EditedScene and now context is determined by this value.

I also added Action struct to EditorUndoRedoManager. It's created for every action and holds action name, context id and timestamp. It's not stored nor used yet, but this will be needed later.

[KoBeWi]

Ok timestamps and global/scene mixed contexts seem to work. So base functionality is roughly done and works as planned (although that's not surprise). Now onto fixing bugs and adding missing features... (like scene versioning)

[KoBeWi]

Oof. I just pushed a commit that removes get_undo_redo2() and changes the base method to return Ref<EditorUndoRedoManager>. The amount of changes I had to do in different editors is insane. I wanted to avoid including EditorUndoRedoManager in the headers, so I changed to forward declarations everywhere (except editor inspector; no idea why it won't allow me). It would be much easier if the undoredo wasn't uselessly assigned to variables instead of just being used from the singleton :/

There was some code I couldn't port, so for now it's marked with /// TODO. The most difficult part probably starts now.

[KoBeWi]

I think I'm close to finishing this. After my latest changes it's possible to revert a scene without destroying whole undo history:

[akien-mga]

That's a matter for a different PR, but I think we should harmonize our usage of "ID" and "id" in the classref.

$ rg -g'*.xml' " ID " | wc -l
122
$ rg -g'*.xml' " id " | wc -l
25

[akien-mga]

Left some non-blocking documentation comments, should be good to merge as is or once amended IMO.

[akien-mga]

Thanks!

[atirut-w]

Is this coming to 3.x by any chance?

[Calinou]

Is this coming to 3.x by any chance?

This relies on backwards-incompatible core refactoring, so I'm afraid not.

[KoBeWi]

Compatibility breaking would only affect editor plugins. While EditorUndoRedoManager has almost the same interface as UndoRedo, it requires doing extra steps in some cases and obviously it would break if someone uses typing. But if such (rather minor IMO) breakage is acceptable, I guess it could be backported. But someone would have to do it and the PR is quite big, soo...