godotengine / godotengine/godot
`EditorUndoRedoManager`: Inconsistent `add_do_property` behavior
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
Reproducible in:
- v4.4.dev.gh [b3bcb2dc1]
- v4.3.stable.official [77dcf97d8]
- v4.0.stable.official [92bee43ad]
### System information
Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 980 Ti (NVIDIA; 32.0.15.6094) - 13th Gen Intel(R) Core(TM) i7-13700K (24 Threads)
### Issue description
Modifying a source `Array` or other collection when using `EditorUndoRedoManager.add_do_property` causes inconsistent redo behaviour, even when deep copy duplicates are used. Specifically, the modification to the source collection is sometimes applied during redo actions instead of the value that is provided to the `add_do_property` function.
I have tested to find that this inconsistent behaviour happens with `Array`, `Array[int]`, `Array[String]`, `PackedInt64Array`, and `PackedStringArray`. It may affect other collections types.
I have not tested with the `UndoRedo` class, but it's possible this reproduces with that class as well.
### Steps to reproduce
``` GDScript
# Example property:
var my_array: Array
var my_array_prop: Array:
get:
return my_array
set(value):
my_array = value
# Example of problematic redo behaviour:
var old_array: Array = my_array.duplicate(true) # deep or not doesn't matter
var new_array: Array = my_array.duplicate(true) # deep or not doesn't matter
new_array[0] = 42
my_array[0] = 99 # This line causes inconsistent redo behaviour
undo_redo.create_action("Action: Set array to " + str(new_array[0]))
undo_redo.add_do_property(self, &"my_array_prop", new_array)
undo_redo.add_undo_property(self, &"my_array_prop", old_array)
undo_redo.commit_action()
```
If the problematic line that modifies the source array is removed, undo and redo behaviour works fine. When this line is added as shown above, performing the redo action will sometimes apply the value that was assigned to `my_array` instead of the value of `new_array` provided to the `add_do_property` function.
### Minimal reproduction project (MRP)
[undo-redo-bug-mrp.zip](https://github.com/user-attachments/files/17478063/undo-redo-bug-mrp.zip)
The following `EditorPlugin` demonstrates the inconsistency:
``` GDScript
@tool
extends EditorPlugin
var my_int: int
var my_int_prop: int:
get:
print("getting int: " + JSON.stringify(my_int))
return my_int
set(value):
my_int = value
print("setting int: " + JSON.stringify(my_int))
var my_array: Array
var my_array_prop: Array:
get:
print("getting array: " + JSON.stringify(my_array))
return my_array
set(value):
my_array = value
print("setting array: " + JSON.stringify(my_array))
static var count: int = 0
func _enter_tree() -> void:
my_int = 0
my_array.push_back(0)
add_tool_menu_item("Undo Redo Test: Commit Action", test)
func _exit_tree() -> void:
remove_tool_menu_item("Undo Redo Test: Commit Action")
func test() -> void:
for i in range(3):
count += 1
var undo_redo: EditorUndoRedoManager = get_undo_redo()
var old_int: int = my_int
var new_int: int = count
my_int = count + 10
undo_redo.create_action("Action: Set int to " + str(new_int))
undo_redo.add_do_property(self, &"my_int_prop", new_int)
undo_redo.add_undo_property(self, &"my_int_prop", old_int)
undo_redo.commit_action()
var old_array: Array = my_array.duplicate(true) # deep or not doesn't matter
var new_array: Array = my_array.duplicate(true) # deep or not doesn't matter
new_array[0] = count
my_array[0] = count + 10 # This line causes inconsistent redo behaviour
undo_redo.create_action("Action: Set array to " + str(new_array[0]))
undo_redo.add_do_property(self, &"my_array_prop", new_array)
undo_redo.add_undo_property(self, &"my_array_prop", old_array)
undo_redo.commit_action()
```
After running the tool, then undoing and redoing all actions, the console shows the following (I've added arrows to highlight the inconsistency):
```
Action: Set int to 1
Action: Set array to 1
Action: Set int to 2
Action: Set array to 2
Action: Set int to 3
Action: Set array to 3
setting int: 1
setting array: [1]
setting int: 2
setting array: [2]
setting int: 3
setting array: [3]
Global Undo: Action: Set array to 3
setting array: [2]
Global Undo: Action: Set int to 3
setting int: 2
Global Undo: Action: Set array to 2
setting array: [1]
Global Undo: Action: Set int to 2
setting int: 1
Global Undo: Action: Set array to 1
setting array: [0]
Global Undo: Action: Set int to 1
setting int: 0
Global Redo: Action: Set int to 1
setting int: 1
Global Redo: Action: Set array to 1
setting array: [12] <-------------------
Global Redo: Action: Set int to 2
setting int: 2
Global Redo: Action: Set array to 2
setting array: [13] <-------------------
Global Redo: Action: Set int to 3
setting int: 3
Global Redo: Action: Set array to 3
setting array: [3]
```
Contributor guide
Research direction
Start with the linked minimal reproduction project and the EditorUndoRedoManager.add_do_property entry point. Reproduce the Array and packed-collection cases, then compare the values supplied to add_do_property with those applied during undo and redo. Done means redo consistently applies the recorded do-property value rather than a later source-collection mutation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100