godot: UndoRedo doesn't undo in the reverse order

Godot version

3.4.4.stable

System information

Windows 10, GLES3

Issue description

When using UndoRedo to record and commit actions they are executed in the order that they are added (good, correct) but they are not undone in the reverse order (problematic).

This is a problem if you persist an UndoRedo while you accumulate actions as part of a single gesture (stroking a line). If the line self-intersects the undo corrupts the data and does not restore it to the state that it was in previously.

UndoRedo.undo() should execute everything in reverse because it’s not always possible to guarantee the ordering of the code that adds methods/data to be undone.

Steps to reproduce

tool
extends EditorPlugin

func _echo(what:String, n:int) -> void:
	prints(what, n)

func somewhere() -> void:
	var test := get_undo_redo()
	test.create_action("test")

	for n in 3:
		test.add_do_method(self, "_echo", "do", n)
		test.add_undo_method(self, "_echo", "undo", n)

	test.commit_action()
	test.undo()

do 0
do 1
do 2
test
undo 0
undo 1
undo 2

Minimal reproduction project

No response

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 17 (17 by maintainers)

Most upvoted comments

Merging only works for actions that have the same name. If your 3 2 1 and 5 4 actions are different then they should have different name and never be merged.

UndoRedo was designed specifically for editor, not for games. If you have a complex use-case where you have multiple different actions merged together and their order matters and all, it’s just out-of-scope for this class.

The main use of merging is actually in the editor. Often when changing something in the editor you might see message spam about setting some property (e.g. when using ColorPicker). These 30+ actions are merged into 1, otherwise it would make undo unusable.

KoBeWi on Apr 30, 2023

This is issue was recently brought up on chat. Reverse order could be an optional argument of create_action() method.

KoBeWi on Apr 30, 2023

IMHO, the way you structure your code assumes undo methods will be called in reverse order.

for n in 3:
    test.add_do_method(self, "_echo", "do", n)
    test.add_undo_method(self, "_echo", "undo", n)

The add_*() methods just add the command to the corresponding action.

# execute these for DO
test.add_do_method(self, "_create_that_object")
test.add_do_method(self, "_add_that_object_to_system")
test.add_do_method(self, "_update_viewport")

# execute these for UNDO
test.add_undo_method(self, "_remove_that_object_from_system")
test.add_undo_method(self, "_destroy_that_object")
test.add_undo_method(self, "_update_viewport")

It’s strange to write something like:

# you have to read this in reverse order (?)
test.add_undo_method(self, "_update_viewport")
test.add_undo_method(self, "_destroy_that_object")
test.add_undo_method(self, "_remove_that_object_from_system")

timothyqiu on May 7, 2022