Add `@connect(object.signal)` method targeted annotation

tischenkob commented 3 years ago

Describe the project you are working on

A generic platformer in GDScript.

Describe the problem or limitation you are having in your project

Declarations of methods that get called by signals are spread apart from the connections to those signals. If a person wants to find out which signal a method is connected to (or if it is even connected to a signal), they have to go to the _ready() method and/or search around the editor through all the signals. It is also possible to use different code editors with Godot which makes it especially hard to track signals connected from the editor.

Currently, it is possible to connect any method to any signal from any place in any file potentially leading to tightly coupled design where a child node has self.signal.connect(parent.method)-like code making it unable to be reused.

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

A method targeted @connect (EDIT: or @on; more in comments) annotation that accepts a signal or a list of signals we want to connect this method to. It is placed right above the method so that it is clear from the first glance which signals are connected to this method.

The proposed feature also forces a better approach to signals by keeping everything in one place in the parent node.

It declutters the _ready(), too.

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

@onready
var child = $Child

# lots of other code

@connect(child.fired_signal)
func _on_child_fired_signal(arg):
  print(arg)

@connect([child.signal_one, child.signal_two]) # or multiple @connect's
func _on_child_special_signals():
  pass

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

If we talk about convenience, clarity, and visibility, it can't.

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

It cannot be an add-on since custom annotations are not yet planned to be supported in GDScript.

YuriSizov commented 3 years ago

For a reference, when I suggested this in the original annotations proposal, people reacted very well: https://github.com/godotengine/godot-proposals/issues/828#issuecomment-634729870

MaaaxiKing commented 3 years ago

So, @pycbouh suggested @connect(object, signal) and here it says @connect(object.signal). I find the latter better since you do not actually need two arguments for this purpose. One argument for the signal itself, one for parameters (Array), one for flags, which is in total three instead four!

YuriSizov commented 3 years ago

@MaaaxiKing I suggested it this way because signals were only accessible as strings back then. Now that signals are first-class properties, object.signal makes the most sense.

tischenkob commented 3 years ago

For a reference, when I suggested this in the original annotations proposal, people reacted very well: #828 (comment)

Yeah, sorry, I skimmed through that thread but somehow missed your and the other person's suggestions. I was actually genuinely surprised this was not one of the first things implemented with annotations since it looks like a very natural place for them to be used at

sairam4123 commented 3 years ago

And could we be passing extra arguments? Like we currently have bind arguments.

tischenkob commented 3 years ago

And could we be passing extra arguments? Like we currently have bind arguments.

Sure. Code taken from https://github.com/godotengine/godot/pull/42683

# old style 
    $Button.connect("pressed",this,"myfunc",["watsup"])
# new style
    $Button.pressed.connect( myfunc.bind("watsup") )
# annotation style
    @connect($Button.pressed, ["watsup"])
    func myfunc(extraarg):
      print("hello",extraarg)

Then passing an array of signals is probably a bad suggestion. Multiple @connect it is.

tischenkob commented 3 years ago

I've also got a questionable idea: change the name from @connect to @on in favor of a more declarative style.

Since the connection and the method are close together, we no longer need to name the method conventionally like on_Object_signal() which is great since names like these are not descriptive, go against the general rule to use verbs for methods/functions, and seem to be strictly bound to a signal. Example: enemy should explode on timer timeout or when it hits a wall; this method should probably be called explode() but it is not.

if self.hit_wall():
    self._on_Timer_timeout()

Of course, we may/should write an explode() method and call it from on_Timer_timeout() but that's extra code. However, with an annotation we'll only have

@connect($Timer.timeout)
func explode():
  # logic

Which is more concise.

That's where the @on suggestion comes into play. This code

@on($Timer.timeout)
func explode():
  # logic

can be read as a simple English sentence: "On timer timeout, explode". Whereas @connect looks like a separate instruction to do something.

MaaaxiKing commented 3 years ago

At the moment, I'm not sure if I'd like multi-connect, but of course, it could work having method overloading:

@connect(object.signal) func foo1():
print("1")

same as

@connect([object.signal]) func foo1():
print("1")

or multiple signals:

@connect(object.signal, object.signal2) func foo2():
print("2")

same as

@connect([object.signal, object.signal2]) func foo2():
print("2")

with one parameter

@connect(object.signal, "This is a string argument") func foo3(param1: String):
print("3")

same as

@connect(object.signal, ["This is a string argument"]) func foo3(params: String[]):
print("3")

same as

@connect([object.signal], "This is a string argument") func foo3(param1: String):
print("3")

same as

@connect([object.signal], ["This is a string argument"]) func foo3(params: String[]):
print("3")

with two parameters

@connect(object.signal, "This is a string argument", 42) func foo3(params args):
print("3")

same as

@connect(object.signal, ["This is a string argument", 42]) func foo3(params: []):
print("3")

same as

@connect([object.signal], "This is a string argument", 42) func foo3(params args):
print("3")

same as

@connect([object.signal], ["This is a string argument", 42]) func foo3(params: []):
print("3")

and also with flags; I don't write it here, since I do not like the way it's implemented right now (integers for enum's values)

What I've done above requires in some cases obviously varargs—I used args for clearness but this doesn't need to be the final way. See also here.

Amatrelan commented 2 years ago

There is one case I would like to see in this is to connect multiple signals in one go, for ex:

var children := get_children()

@connect(children, signal_name, arg)
func on_child_signal(arg):
    ...

In this case it could be hard to specify signal as object.signal because were in this case talking about array of elements. This would be especially helpful when having custom signals for different things, not that much use in builtin signals.

MikeFP commented 1 year ago

In this case it could be hard to specify signal as object.signal because were in this case talking about array of elements. This would be especially helpful when having custom signals for different things, not that much use in builtin signals.

You could make it a different annotation, like so:

@connect(child.signal_name, arg)
@connect_all(children, signal_name, arg)

Having the first option is preferable IMO because it's more robust for referencing the signal directly instead of a String.

vnen commented 1 year ago

I'm not against this but it's unlikely to be done for 4.0.

I would like to see a more fleshed out proposal. There has been some questions answered in the thread that weren't added to the original post, so it's tough to understand what's actually wanted. For instance:

How to pass connection flags?
How to add bound arguments? In code you bind them to the Callable, but in this case you do not have access to that.
When does the signal connection happen? It probably is done when _ready() but I don't see a reason to limit this feature to nodes. If it's on _init() then you cannot access the nodes in the tree which is probably a more common usage.
- I do say it's technically possible to detect if you used get_node() (or the $ shorthand), or if it's an @onready variable, and connect on _ready() in this case and otherwise connect on _init(). The annotation has to use instance variables or function calls since it doesn't have a local context to rely on. This can become too much magic though.

I personally don't like the idea of connecting multiple signals at once. If you need that just use the annotation multiple times. If you need to connect to all objects in a list, don't use the annotation at all, instead use a for loop in _ready() (I don't believe this case happens often enough to warrant its own annotation syntax).

I also don't like having multiple meanings for arguments of the annotation. It's hard to understand and really hard to implement. If @connect(object.signal, other_object.signal) connects multiple signals, then @connect(object.signal, bound_arg) cannot work: it would try to connect bound_arg as if it were a signal, and vice-versa. This is pretty much impossible to implement since it may be unknown at compile time if the value is a signal or not, and leaving this to be resolved at runtime is unnecessary overhead. Not to mention nothing really stops you of sending a signal as a bound argument, so it's an assumption that won't always be true.

godotengine / godot-proposals