godot
godot copied to clipboard
godotengine
Add per-scene UndoRedo
~~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 https://github.com/godotengine/godot-proposals/issues/1630 Fixes #45601
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).
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.
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.
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.
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.
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.
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).
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.
Almost finished with moving stuff to the new system. Note that I'm just replacing UndoRedo * with Ref<EditorUndoRedoManager>. 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.
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)
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.
I think I'm close to finishing this. After my latest changes it's possible to revert a scene without destroying whole undo history:
I think I'm done. This ended up being bigger than planned (what a surprise...), but I covered all usages of UndoRedo, hopefully. Obviously all needs testing now. What can break is some undo usages not being able to provide proper context (most of the time it's guessed from the object that is modified in the first added method/property, but sometimes the first object is unrelated. In such cases, the context has to be set manually).
What certainly broke is user editor plugins. Seems like I'd have to expose EditorUndoRedoManager to handle them properly. I also forgot to use timestamps in the end. The system works anyway, but the order of actions is more confusing in some cases.
Any special notes on EditorUndoRedoManager for people porting to it in the future?
Do you want me to test for a few days using it?
That would be helpful. I'm also going to do so.
Any special notes on EditorUndoRedoManager for people porting to it in the future?
The usage is the same as UndoRedo (you create action, add do/undo methods/properties and commit). It does most of the stuff behind the scenes, but sometimes when creating action you need to provide an object, to make sure the action is tied to the correct scene.
Also this is not available in GDScript for now. get_undo_redo() method returns UndoRedo for a custom plugin action history, separate from editor.
Status update on this PR: I think this is mostly finished now.
I have one unresolved problem, but it's not a blocker. Basically I can't forward-declare EditorUndoRedoManager in editor_inspector.h. It throws compilation errors that don't make sense (typical C++). Would be nice to do this properly.
Other than that, there's a small concern regarding EditorPlugins. I'd need to expose EditorUndoRedoManager to GDScript to have proper UndoRedo support for plugins. Right now they can't properly interact with scene history. idk if there are better solutions 🤔
I've been testing this a bit and found some undo issues, but some of them turned out to happen in vanilla alpha too, so it's not my problem lol. But the PR needs much testing overall. What can break is that some actions might not be able to determine proper undo context. The context is guessed based on the first method used for undo action (it's deduced from the used object), but if the object is e.g. EditorSelection, the action won't be recognized as belonging to the specific scene. I don't know how to test this properly other than going over every single UndoRedo usage and checking if it's used correctly.
I just pushed a commit that finally implements proper timestamps (so the history behaves in more predictable way), but for some reason undo will stop working randomly. Need to fix that.
Any ideas why this doesn't compile? 0001-fd.patc.txt For whatever reason I can't forward-declare EditorUndoRedoManager in Editor Inspector. It results in "use of undefined type" bug, but I'm not using it; same code works everywhere else.
Rebased again.
I've been using this for the past few weeks. I got 2 crashes at the beginning, but then they never happened again and the undo is reliable. So idk, it would be nice to get this reviewed and merged. I think there is still some (rare?) crash when exiting, but the system is quite stable overall.
The issue from sanitizers is probably the one responsible for the crash on exit. Not sure what is causing it though.
Rebased again. I fixed the plugin issue I mentioned before by exposing EditorUndoRedoManager for scripting.
The sanitizer issue still remains though.
Is this coming to 3.x by any chance?
This relies on backwards-incompatible core refactoring, so I'm afraid not.
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...