godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.05k stars 20.19k forks source link

EditorPlugin: undo method is not called if object parameter can't be casted to Resource #41877

Open Novakasa opened 4 years ago

Novakasa commented 4 years ago

Godot version: v3.2.2.stable.official

OS/device including version: Windows 10 x64, two different systems tested

Issue description: After adding a undo method for a UndoRedo action, it will only be called on undoing the action if the object is a builtin type, or extends EditorPlugin

I'm wondering if that is intended behaviour, in that case we should probably add something to the documentation.

Example: undo_test.gd:

tool
extends EditorPlugin

func _input(event):
    var just_pressed = event.is_pressed() and not event.is_echo()

    if Input.is_key_pressed(KEY_F12) and just_pressed:
        test_action()
        return true

func test_action():
    var undo = get_undo_redo()

    undo.create_action("test action")

    var action_ref = ActionRef.new()
    var action_plugin = ActionPlugin.new()

    undo.add_do_method(action_ref, "output", "do method in ActionRef")
    undo.add_do_method(action_plugin, "output", "do method in ActionPlugin")
    undo.add_undo_method(action_ref, "output", "UNDO method in ActionRef")
    undo.add_undo_method(action_plugin, "output", "UNDO method in ActionPlugin")

    undo.commit_action()

action_plugin.gd:

tool

class_name ActionPlugin
extends EditorPlugin

func output(message):
    print(message)

action_ref.gd:

tool

class_name ActionRef
extends Reference

func output(message):
    print(message)

Then, when executing the action (press F12), both output methods are called. When undoing the action, only the undo method for the instance which extends EditorPlugin is called.

image

Steps to reproduce:

Minimal reproduction project: https://github.com/lolligerjoj/undo_test.git

Rubonnek commented 4 years ago

I looked a bit into this and I've not found the offending code, but I found a hint.

Turns out that classes that can be casted to Resource can call undo methods:

https://github.com/godotengine/godot/blob/e49d25bac08d1205f5c2cd7126a0323fd028e0cc/core/undo_redo.cpp#L131-L135

So switching your ActionRef to extend Resource like the following should work:

tool

class_name ActionRef
extends Resource

func output(message):
    print(message)

You could use this as a workaround in the meantime.

This UndoRedo class may need to be retouched -- there are quite a few casts to Resource in there that don't show any warnings at all, but should inform the user what's happening underneath.

I'm also not sure why UndoRedo would specifically need Resource references as none of these references seem to get serialized, but perhaps it was left that way so that when users build tools with EditorPlugin or GraphEdit for example, they could save the sessions on those tools rather easily along with their UndoRedo history, but the serialization of UndoRedo's history was never implemented.