Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign how Undo/Redo works to make it more predictable #2109

Closed
reduz opened this issue Jan 12, 2021 · 19 comments
Closed

Redesign how Undo/Redo works to make it more predictable #2109

reduz opened this issue Jan 12, 2021 · 19 comments

Comments

@reduz
Copy link
Member

reduz commented Jan 12, 2021

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

UndoRedo is rather unfriendly, as it keeps all the history across edited scenes. This forces going back and forth scenes in a confusing way, also clears the history for everything when a scene is closed.

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

The main idea is to implement a per-scene UndoRedo. This has not been done before because some objects in Godot are shared across al scenes (such as resources), so editing them individually can break consistency. It's a difficult problem to solve.

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

Each scene will have its own UndoRedo stack. To be able to do this, the following changes need to take part:

  • When editing a shared resource (resource saved on disk), an edition of this specific resource in a scene will trigger other scenes open to lose their undo-redo history up to the point they edit this object (if they didn't edit it, which is most likely the case, nothing is lost). This avoids consistency problems and is friendly enough that most users will not notice (as editing a shared resource from multiple scenes is not a common use case anyway). Additionally If it is known that a certain resource is harmless, we can add a hint to it so the editor knows it needs not to remove history.
    If toggling resources between built-in and shared, you lose undo-redo history (or you lose it in other scenes I suppose).
  • Editing a built-in resource from a foreign scene can definitely break history consistency, so this should be blocked. As this can be useful many times (Specially for animation), we can introduce the concept of resource inheritance. This is similar to scene inheritance but for resources. The editor would just make a copy of this resource in the local scene.
  • Project settings or anything that edits the global settings, or editor settings, needs its own undo-redo separate from scenes.

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

No

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

This is editor core.

@norlowski
Copy link

Love the idea. I use JetBrains products in my dayjob and their undo-redo works extremely well. I especially like how it changes the tab focus as you progress back up the undo stack. Sometimes I just put a whitespace change in and delete it so that I can keep that section of the code in my mental stack because I know I'm coming back to it. Just a thought. Sounds like a complex but very welcome feature

@me2beats
Copy link

me2beats commented Jan 12, 2021

Having undo for every scene would be very helpful.
In this case, a scene becomes a more self-sufficient environment.
It would also allow making convenient undo/redo tools like undo history, undo sliders etc.

see also #1630

Is it less friendly to have a separate undo/redo for resources tho?

@eon-s
Copy link

eon-s commented Jan 12, 2021

It would be nice to be able to see the current scope of undo/redo in the editor (like: Editing scene X, Editing shader graph, Editing gdscript), on a status bar or something like that, because it is easy to mess up things with other parts of the editor.

@igor84
Copy link

igor84 commented Jan 12, 2021

If changing the same shared resource across two or more scenes is really so rare one more option could maybe be this:

  1. Shared resource R is changed in scene S1 and not saved
  2. User attempts to change the same resource R in scene S2 and gets a message: "R is modified in S1. Do you want to save and reload those changes or discard them?"
  3. just aborts the new change. discards the changes from S1 and removes all R editing actions from S1 undo history and then just proceeds with a new change. saves the changes made in S1, reloads the resource in S2 and makes a new change.

I'm not that familiar with Godot so my idea might not make sense but hopefully it will at least inspire some more thinking 😊

@golddotasksquestions
Copy link

golddotasksquestions commented Jan 17, 2021

Undoing parameters of shared resources in other scenes which are currently not open makes me nervous. The way Godot currently deals with shared resources hardly feels transparent or consistent(godotengine/godot#24950, godotengine/godot#6922, #317). I hope this won't make shared resources even more confusing than they already are. At the very least I would hope for a pop up warning be about the consequences.

Maybe this would also be a good time to think about the feasibility an optional Undo panel? #953

@KoBeWi
Copy link
Member

KoBeWi commented Jan 18, 2021

btw remote tree also needs a separate undo, because currently it fake-modifies current scene.

@elvisish
Copy link

I've had trouble with undo if I use a player scene and an enemy scene tab, if I undo and it switches the tab, it loses undo and redo, and switching back to the different tab doesn't bring it back.

@ator-dev
Copy link

ator-dev commented Dec 10, 2021

I've started a club which would greatly benefit from live multi-person editing (as in #851), and is difficult to run properly without it due to the nature of the computers and systems being used. I think a plugin is a good approach for this which I would be happy to experiment with making, but in my opinion it's impossible to implement it well without separate undo/redo stacks for scenes. There are a lot of use cases for a modularised undo system and it feels like a major limitation of the editor at the moment.

Juan's original proposal (and some of the other suggestions) seem interesting, although I don't fully agree with automatically discarding some history up to a point as soon as a shared resource is edited. In many cases, scenes don't actually rely on resources being entirely 'correct' at any given time; often users just do something like change the colour or scale of something so it looks different across the places it's used. I think the situation of a resource being used across multiple open scenes, the user changing something that causes incompatibility, then this later being undone is relatively uncommon; editing shared resources inherently comes with this risk anyway, and usually the problems caused are trivial to fix.

To accommodate this - if I'm correct in my above statement - perhaps editing a resource could result in changes being logged in its own stack (or tagged as its property, etc) and a pointer to that position added to the scene's stack. The same would apply when it's modified in other scenes. If a scene's history is undone up to or past one of these points, the resource's changes would be undone as usual; but if a different pointer is reached in another scene while navigating the history, its state would be automatically forwarded to the point it was at when the pointer was added.

I'm aware there are certain resources that may be more sensitive to changes, but these could be looked into to be better integrated or for allowances to be made.

Does this seem like a viable option in any way?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2021

I also don't like the idea of randomly discarding history. Some time ago I made my own proposal for this: #1630. I think the option 2 I described would be the best, especially coupled with history log #953, which would allow to have overview of global and scene history and their timestamps.

@ator-dev
Copy link

Looks like a good proposal, but I think we should avoid adding additional undo shortcuts and focus-based history (though it's similar to a solution I also considered). Option 2 seems like a similar idea to my suggestion; however, I was wondering if using a separate history stack also for each resource would be beneficial? That way there's little possibility of losing much if the history gets interfered with in an undesirable way (which is inevitable whatever kind of system you implement); the separate stacks would simply be forwarded to different states depending on the history navigation of the current scene.

@ator-dev
Copy link

ator-dev commented Dec 11, 2021

As @me2beats mentioned in #1630, it does seem like bad practice anyway to have scenes dependant on resources being in a particular state, and I think the engine should avoid this kind of design in general. In my experience this doesn't seem to happen much luckily which could make an undo system like some of the ones mentioned (resources having separate histories / a history) significantly less risky and confusing.

As for the history log #953, this seems like a good idea as well, but probably it's worth sorting out an undo system that works well for different scenes/resources before properly implementing it.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2021

This is what I have in mind:
image
The actions would be managed by UndoRedoManager, with each scene having its own UndoRedo + there would be a global one. Every undo action would get a timestamp (given by the manager). When you undo, the editor would look up actions in the current scene + global actions and undo the action that has the highest timestamp.

E.g. in the above image if you are in Scene1, using undo twice would undo Action2/Scene1 and Action3/Global. This is possible to do without modifying the current UndoRedo system or touching any shortcuts. The history would just be managed on a higher level.

My only concern is that having 2 intertwining history lines could be confusing, but undo log would alleviate that. Not sure if there is better way, without using hacks like random history clearing in the OP.

@ator-dev
Copy link

ator-dev commented Dec 11, 2021

Ah, right; I think that could potentially work well. Random history clearing or forwarding would probably have to be necessary though, as you say.

Still, I am concerned that undoing something in one scene (such as Scene2) could interfere with the global history in unintended ways, say if Global Action1-3 were all made in Scene1. If instead it ignores irrelevant changes, then history is suddenly rewound to a Global action made much earlier, many global actions could end up being lost.

That's why I think it could be useful to have the global history separated into actions made on separate resources; if it has to be rewound a long way, only one resource is affected at a time. Changes might also be restored if you go to the scene the last one was made in, then redo up to that point.

(Edit: my wifi will be turning off soon so I might have to continue the discussion later, sorry)

@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2021

Still, I am concerned that undoing something in one scene (such as Scene2) could interfere with the global history in unintended ways, say if Global Action1-3 were all made in Scene1. If instead it ignores irrelevant changes, then history is suddenly rewound to a Global action made much earlier, many global actions could end up being lost.

This shouldn't actually happen with my system. Undo would undo the latest action on stack, redo would redo the earliest action. Considering the image above again:

  • when you undo 3 times while on Scene2, you undo Action3/Scene2, Action2/Scene2, Action3/Global
  • then you undo 3 times on Scene1, so you unde: Action2/Scene1, Action1/Scene1, Action2/Global
  • then you redo 3 times on Scene1, so you redo: Action2/Global, Action1/Scene1, Action3/Global

Sounds tricky, but it's actually rather simple. The Global history and Scene history are independent. It doesn't matter if you edit a global resource on scene 1 or scene 2, because the undo history is considered based on the time of modification. So your undo has different effect depending if you do it on scene 1 or scene 2, but it's actually predictable and doesn't lead to random resets.

@ator-dev
Copy link

I think I see what you mean, but I'm still not convinced it will be easier or work better than a more modularised system. I'm open to suggestions though - for now I'll work on building an experimental implementation in a test branch to get a better idea of how these kinds of undo systems might feel to use.

@ator-dev
Copy link

ator-dev commented Feb 9, 2022

Potentially of interest is this poll I conducted: https://www.reddit.com/r/godot/comments/slwtkx/poll_how_do_you_use_godots_undo_system/?utm_source=share&utm_medium=web2x&context=3

Though the validity of the results are arguable as are any statistics, I think it clearly shows that Godot users are largely dissatisfied with the undo system (excluding people who have never tried it, the proportion is roughly half-and-half). The discussion below the poll is rather more interesting - several people expressed their opinions (some negative, many less negative but frustrated), and the point that came up extremely often was the limitations of the global undo system.

Contextual undo is one way in which this could be done differently, and would resolve pretty much all of the mentioned issues with the stack being lost when certain docks/views are closed (necessary in most cases because of global undo) and the unintuitive/frustrating behaviour regarding how scenes are tied together in the stack.

Though my aforementioned proposal and PR aim to improve the overall discongruity and breakages of the currently available undo-redo methods by providing a 'safe' cumulative abstraction layer, this could hugely reduce problems by safely separating stacks; whereas if one context becomes broken at the moment, all history becomes unavailable which could result in a lot of lost work (or at least annoyance and distrust in undo-redo/Godot in general).

@ator-dev
Copy link

ator-dev commented Mar 9, 2022

Relevant: #1630 (comment)

@ator-dev
Copy link

ator-dev commented Mar 9, 2022

Editing a built-in resource from a foreign scene can definitely break history consistency

@reduz - or someone else knowledgeable on this - I am currently working on an implementation of contextual undo which is going very well so far, and looking back at this proposal I found this suggestion (which differs from what I'm making) and was wondering if it could be reviewed?

In my opinion editing/undoing/redoing a resource independently of scene undo stacks is not a problem, and may even be beneficial to the user experience. When you change the colour of a material from blue to red, it's interesting and useful to see that now all scenes containing that material have the updated colour scheme, which makes the design very versatile and easy-to-use. I feel like the same should apply to undo-redo - if you want to undo this colour change at some point, you can open the resource and make an undo applied to it, e.g. based on Inspector focus, and now all scenes have the blue colour scheme you were trying out earlier. Trying to join this history with scenes and potentially create resource offshoots feels problematic and, to me, unnecessary.

Would you mind explaining what scenario/s you have in mind please? I can then factor this into my implementation and come up with a more suitable PR, or at least this will continue the discussion a bit. At the moment my PR gives every context necessary - such as project settings, editor settings, each resource, each scene, and potentially more - a separate history stack. I find it to work well but I'm interested in hearing your viewpoint.

I believe Godot users are quite highly concerned with having this feature as it has been suggested many times and gets brought up every single time I mention UndoRedo with the community, so some progress on it would be really good; I think it really holds back Godot's flexible scene/module-based design.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 22, 2022

Implemented in godotengine/godot#59564

@KoBeWi KoBeWi closed this as completed Aug 22, 2022
@KoBeWi KoBeWi added this to the 4.0 milestone Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants