Make the 800ms UndoRedo merge interval customisable in Settings

[TheSofox]

Describe the project you are working on

I'm working on the UndoRedo system, fixing bugs and trying to improve the documentation. Right now there's functionality for merging multiple UndoRedo actions into one depending on MergeMode.

One undocumented feature of this is that in order for actions to be merged, they need to take within 800ms of eachother.

Describe the problem or limitation you are having in your project

There are issues with the 800ms limit

1. It's a hardcoded "magic number"
2. Because there might be changes to it in the future, it can't be documented. This lack of documentation has caused confusion multiple times to people working on it (more details on these can be found here: https://github.com/godotengine/godot-docs/issues/8533 )
3. It feels "anti-user" to not have this customisable. Worth noting that the TextEdit field has a completely separate UndoRedo system that is you can change the time interval for in Project Settings (default 3 seconds)

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

I suggest adding a customisable setting to edit this 800ms interval:

• It should probably be in Project Setting since it will need to run in any exported project. (Remember, the UndoRedo system works not just in the Editor, but can be used in any project/game exported by Godotr)
• I suggest GUI -> Timers -> Merge Undo Time Limit (ms).
• Integer type.
• Range could 0 upwards.
• However maybe we could also make -1 a "infinite" time where it won't matter the time between actions, if all other criteria is met, they merge?

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

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

It's a setting... so no.

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

See above.

[sergioabreu-g]

I don't think this fixes the real problem, which is the UndoRedo's base design.

Why should there even be a time limit to start with? That doesn't look like a feature that should be native, but rather something that might be needed for a specific project, and which can be easily built on top of a better-designed system. We could just have a way of indicating when to start and stop merging an action, for example:

var undo_redo = UndoRedo.new()
undo_redo.start_merging("my_merged_action")

# These two will be merged

undo_redo.create_action("my_merged_action")
undo_redo.add_do_method(do_something)
undo_redo.commit_action()

undo_redo.create_action("my_merged_action")
undo_redo.add_do_method(do_something_2)
undo_redo.commit_action()

undo_redo.stop_merging("my_merged_action")

# This won't be merged, it will be considered a separate action because merging is disabled for this action's name

undo_redo.create_action("my_merged_action")
undo_redo.add_do_method(do_something_2)
undo_redo.commit_action()

That way there would be no need for a timer at all, you could just tell the UndoRedo when to start and stop merging actions with a certain name, which would give the users much more control over the whole merging process. I'm currently doing something pretty similar to this in a project of mine, but I had to build a very hacky system on top of the native UndoRedo to be able to do so.

If someone wanted to merge actions only within a specified timeframe, it could be as easy as creating a timer that called the stop_merging(...) function when it ended.

undo_redo.start_merging("my_action")

undo_redo.create_action("my_action")
undo_redo.add_do_method(do_something)
undo_redo.commit_action()

var timer = create_timer()
my_timer.connect("timeout", func(): undo_redo.stop_merging("my_action"))

[TheSofox]

Your code you wrote ideally can be written with the existing API and less lines of code:

var undo_redo = UndoRedo.new()

# These two will be merged

undo_redo.create_action("my_merged_action", UndoRedo.MERGE_ALL)
undo_redo.add_do_method(do_something)
undo_redo.commit_action()

undo_redo.create_action("my_merged_action", UndoRedo.MERGE_ALL)
undo_redo.add_do_method(do_something_2)
undo_redo.commit_action()

# This won't be merged, it will be considered a separate action because merging is disabled for this action's name

undo_redo.create_action("my_merged_action", UndoRedo.MERGE_DISABLE)
undo_redo.add_do_method(do_something_2)
undo_redo.commit_action()

However, I will confess MERGE_ALL is currently broken as reported here: https://github.com/godotengine/godot/issues/26118. Currently it'll run EVERY "do" action each time a new action is merged which is not proper behaviour, and my PR to fix it has yet to be accepted: https://github.com/godotengine/godot/pull/85390

Ideally though, that's the best way to do it. MERGE_ENDS is working, but more for rapidly changing property values where only the first and last one matter.

To add to this, I've already suggested that for a negative value, the timer would be disabled.

And finally, this timer is used right now in the Editor. For instance, if you got to the Color Picker, and drag across the color wheel choosing a lot of different values, or spin the SpinBox up or down through a lot of different values. With the 800ms gap and MERGE_ENDS, all of the rapidly passed intermediate values are discarded, and the action is only saved when there's a gap. You could change the value in a bunch of sharp bursts, then easily undo the values to between those bursts.

The 800ms doesn't just affect exported projects, it affects the Editor as it exists right now.

[sergioabreu-g]

Oh, I didn't know MERGE_ALL was broken, that explains why I was getting some very weird results in my project.

Your code is indeed better than mine, I wasn't understanding properly the current merge behaviour.

Regarding the timer, I think it should be zero/negative/disabled by default, it's not useful for most use cases. Also, it makes it confusing to use, most people (me included) didn't even know there was a time limit for actions to be merged. Even if documented properly, it's not something you expect to be activated by default when you instantiate a new UndoRedo.

For example, I'm making a game that has discrete states, but transitioning between these states cannot be done with one action only, so I have to make multiple sequential actions and merge them. That way, when doing undo/redo, the state is changed entirely by applying all the subactions necessary to do so. That does not require a timer at all, and I think most use cases when building games will be similar to that.

Also, the timer should be per action and not per instance, because different actions may require different timer behaviours, either different time limits or not having a timer at all.

[dmlary]

Another data point, I hit this while working on a GDExtension adaptation of GridMap & GridMapEditorPlugin for hex-shaped cells.

When painting multiple cells by clicking and dragging, I was using MERGE_ALL to create a single undo/redo action. Worked great until I started hesitating and rotating the tile as I drew. The 800ms timeout breaks the single action into multiple actions.

A increased timeout doesn't make sense for this use case; the delay is based on user input. Instead it makes more sense to allow disabling this timeout.

[TypeOverride]

Oh, I didn't know MERGE_ALL was broken, that explains why I was getting some very weird results in my project.

Your code is indeed better than mine, I wasn't understanding properly the current merge behaviour.

Regarding the timer, I think it should be zero/negative/disabled by default, it's not useful for most use cases. Also, it makes it confusing to use, most people (me included) didn't even know there was a time limit for actions to be merged. Even if documented properly, it's not something you expect to be activated by default when you instantiate a new UndoRedo.

For example, I'm making a game that has discrete states, but transitioning between these states cannot be done with one action only, so I have to make multiple sequential actions and merge them. That way, when doing undo/redo, the state is changed entirely by applying all the subactions necessary to do so. That does not require a timer at all, and I think most use cases when building games will be similar to that.

Also, the timer should be per action and not per instance, because different actions may require different timer behaviours, either different time limits or not having a timer at all.

Exactly this! What is the state on making changes on the UndoRedo class? The timer behaviour is odd. Want to disable it at all. That is not a good thing in most usecases.