-
-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add binding of arguments to Callable
in C#
#10906
Comments
Callable
in C#
Callable
in C#Callable
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.
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 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 That said, I'm not opposed to adding |
Sorry yeah, I did forget to mention it: I wrote this post while working with 4.3. I found out that 4.4 dev 3 has had Using 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 |
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 |
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()
orsignalEventHandler += 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:
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: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
andUndoRedo.AddUndoMethod
: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 godotengine/godot#90430, fixed with godotengine/godot#92350 and was reverted as it caused more issues than it solved (godotengine/godot#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)
forCallable.bind(...)
Callable.Bindv(params Variant[] @args)
forCallable.bindv(arguments: Array)
Callable.Unbind(int argCount)
forCallable.unbind(argcount: int)
Callable(GodotObject target, StringName method, params Variant[] @args)
as a constructorWhile 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 likeIf 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.
The text was updated successfully, but these errors were encountered: