Add binding of arguments to `Callable` in C#

Justin113D commented 6 days ago

Describe the project you are working on

I am working on a plugin written in C# that uses multiple UndoRedo instances and that should not throw any errors or warnings when building the .NET project. The issues however can appear under many more circumstances.

Describe the problem or limitation you are having in your project

As of right now, the only way to "bind" to a callable in C# is by calling a method via a lambda. This works during a running game, as the .NET project is not recompiled at any time, but it causes a few issues with plugins written in C#.

Generally, when using Callable.From() or signalEventHandler += myMethod on any node that gets added to the editor, Godot will fail to re-connect it correctly after rebuilding the assembly.

To understand what i mean, see the following example plugin code:

public partial class MyPlugin : EditorPlugin
{
    public override void _EnterTree()
    {
        SceneChanged += OnSceneChanged;
        Connect(SignalName.SceneChanged, Callable.From<Node>(OnSceneChanged));
        Connect(SignalName.SceneChanged, new(this, MethodName.OnSceneChanged));
    }

    public override void _ExitTree()
    {
        SceneChanged -= OnSceneChanged;
        Disconnect(SignalName.SceneChanged, Callable.From<Node>(OnSceneChanged));
        Disconnect(SignalName.SceneChanged, new(this, MethodName.OnSceneChanged));
    }

    private void OnSceneChanged(Node sceneRoot)
    {
        GD.Print(sceneRoot.Name);
    }
}

These are the three way in which you can connect to a signal in C#. All of these run as they should, but once you change something (e.g. changing the printed message to sceneRoot.Name + "!") the identifier behind the method "OnSceneChanged" gets altered. Once that happens, disabling the addon will print out 2 error messages:

Attempt to disconnect a nonexistent connection from '<EditorPlugin#4869419234114>'. Signal: 'scene_changed', callable: 'Delegate::Invoke'.
Attempt to disconnect a nonexistent connection from '<EditorPlugin#4869419234114>'. Signal: 'scene_changed', callable: 'Delegate::Invoke'.

These come from the first 2 connections. Only the third one works correctly, as it does not bind to the actual method, but to the plugin instance and method name. As long as the method signature remains, it will work on reload. Isolated, this does not cause many issues, but can snowball to errors that make it impossible to rebuild the .NET project without restarting Godot, as the program is unable to replace the assemblies.

This especially becomes an issue once you want to pass parameters to the callable, which is possible in GDScript via the Callable.bind() function, but only doable in C# using lambdas.

While for signals, this is sometimes work-aroundable, there are situations where it is not, like when using UndoRedo.AddDoMethod and UndoRedo.AddUndoMethod:

...
public string MyValue { get; private set; }

public void SetMyValue(string value)
{
    string oldValue = myValue;

    MyUndoRedo.AddDoProperty(this, PropertyName.MyValue, value);
    MyUndoRedo.AddUndoProperty(this, PropertyName.MyValue, oldValue);

    MyUndoRedo.AddDoMethod(() => UpdateMyValueUI(value));
    MyUndoRedo.AddUndoMethod(() => UpdateMyValueUI(oldValue));
}

private void UpdateMyValueEditorUI(string value)
{
   ...
}
...

This may work, but prints an error: Can't get method on CallableCustom "Delegate::Invoke". While this is a false positive and can be ignored, it is still irritating and can quickly fill the output.

The issue was reported in https://github.com/godotengine/godot/issues/90430, fixed with https://github.com/godotengine/godot/pull/92350 and was reverted as it caused more issues than it solved (https://github.com/godotengine/godot/issues/92695). As far as i can tell, this was never brought up again.

This issue exists both in C# and GDScript, and can be circumvented in GDScript by binding to a callable of the method, instead of calling it via a lambda instruction.

On top of these issues, I am unable to find out why binding is not supported in C# in any capacity, and must assume that nobody has found a use case for it so far.

The only similar proposal i found is #5835, where lambdas were accepted as the answer. The manual simply says it is "not implemented" but does not explain why either.

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

The simple solution would be to implement

Callable.Bind(params Variant[] @args) for Callable.bind(...)
Callable.Bindv(params Variant[] @args) for Callable.bindv(arguments: Array)
Callable.Unbind(int argCount) for Callable.unbind(argcount: int)
Callable(GodotObject target, StringName method, params Variant[] @args) as a constructor

While i don't know about the technical viability of exposing the 3 methods, the constructor should be possible and enough for the majority of all use cases.

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

The UndoRedo issue above to be solved like

...
public string MyValue { get; private set; }

public void SetMyValue(string value)
{
    string oldValue = myValue;

    MyUndoRedo.AddDoProperty(this, PropertyName.MyValue, value);
    MyUndoRedo.AddUndoProperty(this, PropertyName.MyValue, oldValue);

    MyUndoRedo.AddDoMethod(new(this, UpdateMyValueUI, value));
    MyUndoRedo.AddUndoMethod(new(this, UpdateMyValueUI, oldValue));
}

private void UpdateMyValueEditorUI(string value)
{
   ...
}
...

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

For signals, it is sometimes possible to create a wrapper method around the method that should be called. This however only works when the variable that should be bound is constant or reference-able by the wrapper (e.g. a property or field of the class).

Any other scenario highlighted by the proposal is not work-aroundable.

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

This addresses a missing feature in the core and cannot be added via an add-on. Additionally, this is a feature disparity between C# and GDScript.

raulsntos commented 6 days ago

As of right now, the only way to "bind" to a callable is by calling a method via a lambda. This works during a running game, as the .NET project is not recompiled at any time, but it causes a few issues with plugins written in C#.

There should be no issues using Callables created from C# lambdas, that would be a bug. It sounds like you are referring to reloading issues, there are a number of issues open about that. Reloading is complicated, since every C# instance must be released before the assembly can be unloaded, and you may have code incompatible with unloading.

You also didn't mention which version of Godot you are using, a few reloading issues were fixed recently. Please, try the latest 4.3 release if you haven't already.

I am unable to find out why binding is not supported in C# in any capacity, and must assume that nobody has found a use case for it so far.

It's difficult to implement and an easy workaround exists. C# users tend to prefer to use lambdas anyway, and other than the reloading issues I'm not aware of other problems.

I've tried to add Callable.Bind and Callable.Unbind in the past and it was troublesome. We need to make sure the original Callable is not disposed, it's easy to end up with a dangling pointer which will crash the editor/game.

Regarding the reloading issues, it may be possible to re-architecture your plugin to avoid the unloading issues. It's always easy to run into reloading issues with tool scripts, because these scripts run in the editor so special care must be taken to ensure your code is unloadable.

Sometimes it's the engine side preventing unloading, in that case that's a bug that should be fixed, regardless of whether we add Callable.Bind and Callable.Unbind.

That said, I'm not opposed to adding Callable.Bind and Callable.Unbind, but keep in mind it may not solve all your reloading issues.

Justin113D commented 5 days ago

You also didn't mention which version of Godot you are using, a few reloading issues were fixed recently. Please, try the latest 4.3 release if you haven't already.

Sorry yeah, I did forget to mention it: I wrote this post while working with 4.3. However, since writing this, I realized that my plan to use multiple UndoRedo instances the way i wanted to does not work in the first place and looked for different solutions.

I found out that 4.4 dev 3 has had EditorUndoRedoManager.ClearHistory() exposed and EditorInterface.GetEditorUndoRedo() added, so switching to the dev build solved my issues here.

Using EditorUndoRedoManager works in this case because it has a different method signatures for AddDoMethod and AddUndoMethod: (GodotObject @object, StringName method, params Variant[] @args). These two methods, in the backend, just create a callable and bind to it:

void EditorUndoRedoManager::add_do_methodp(Object *p_object, const StringName &p_method, const Variant **p_args, int p_argcount) {
    UndoRedo *undo_redo = get_history_for_object(p_object).undo_redo;
    undo_redo->add_do_method(Callable(p_object, p_method).bindp(p_args, p_argcount));
}

Would this not mean that something similar can be done for Callable in C#, or does the EditorUndoRedoManager work for different reasons?

raulsntos commented 5 days ago

That works because you never get a reference to the bound Callable in C#. The current Callable implementation in C# just doesn't support custom Callables like CallableCustomBind. The problem with Callable.Bind is that we need to return a valid Callable that wraps a custom Callable, while keeping the original Callable alive so it's not disposed; otherwise, we end up with dangling pointers.

godotengine / godot-proposals