godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

GDScript: Declarative signal connections, `when $Button.pressed:` #7112

Open rune-scape opened 1 year ago

rune-scape commented 1 year ago

Describe the project you are working on

a 2D physics based space ship commander, with a decent amount of GUIs

Describe the problem or limitation you are having in your project

Connecting signals is almost always a bit messy, even if your logic is simple.

Option A:

func _ready():
    # alot of distracting code
    for obj in $Fittings.get_children():
        obj._owner = self

    #setup Raycast ignores
    for raycast in $Avoidence_Raycast.get_children()
        raycast.add_exception(self)

    #add yourself to the minimap
    HUD.AddMinimapIcon(self, _minimap_icon)

    # and of course a button to blow urself up
    $Button.pressed.connect(
        func():
            var explode_node = explosion.instantiate()
            explode_node.set_transform(transform)
            add_sibling(explode_node)
            queue_free(),
        CONNECT_ONE_SHOT)

Its a lambda, which is nice, but its in a function call which is in a function definition. It has to get thrown in next to your other onready setup code. The flags just kinda hang there and dont look right when the lambda is too big.

Option B:

func _ready():
    $Button.pressed.connect(self._on_button_pressed_once, CONNECT_ONE_SHOT)
    #imagine more code here

func _on_button_pressed_once():
    print("button was definitely pressed once")
    #imagine more code here too

Again not horrible, but I have to keep all the connection initialization code separate from what it does, and make sure I put that info in the function name itself, typing it again. Changing any of that info means the function name should also change. It also looks like c++ :(

Option C:

func _on_button_pressed():
    print("is there a bug here??")

Cleanest looking one, but,,, Is this connected in the .tscn file? Maybe. I have to open the scene to see the icon next to the function. Clicking on that icon does not tell me if its a oneshot or deferred connection. I may have just not added '_once' to the default function name because it doesn't affect its behavior. So does it only emit once? If i want to know, I can't find that info unless I go to the the node tab, signals page, right click on the connection, click edit... then I can see the checked box next to oneshot. This could obviously be changed to make to information more accessible, but why should I even put the connection in the scene file in the first place?? i want to keep that info near the code being run I don't use this one anymore.

My last point is that, (if you aren't using the lambda option), when you extend a script you can accidentally and unknowingly override a function with the same name in the super class, making for a very bad bug.

They may be minor gripes except for the last, but these are all the problems ive personally had with connecting to signals in godot.

there is another problem introduced in godot 4 that i've found this proposal 'solves':

the _ready, _process, _physics_process and functions all used to be called at all levels starting from the highest ancestor, the Object class, just like notifications. but because the behavior was unexpected, it was removed in favor of just requiring a call to super._process(), for example. its understandable, but unfortunate, because the old beavior was so useful, and once you learned it, very convenient. now, if you forget to make the call to super._process, you get a very silent bug. not ideal. if you remove the base class func _process, you have to change all inherited classes that call super._process. :/ (also the error only happens at runtime :///) the solution to this right now is to override _notification which is also a function, but does get called on all levels. it is the only that it makes sense this rule would apply to it requires a very 'C' like syntax

func _notification(what: int):
    match what:
        NOTIFICATION_READY:
            set_process(true)
            set_physics_process(true)
            print("ready!")
        NOTIFICATION_PROCESS:
            print("process frame!")
        NOTIFICATION_PHYSICS_PROCESS:
            print("physics frame!")

but you don't get the nice code organization you get from func _process(delta): you either have to throw alot of code into _notification, or make a function identical to _ready, _process and _physics process it also needs a call to set_process(true) for it to work, but this proposal can't do anything about that

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

I'm proposing a new declarative syntax for connections to signals, to be able to describe a connection directly to the compiler

idea taken from here: https://github.com/godotengine/godot-proposals/issues/828#issuecomment-634729870 and here: https://github.com/godotengine/godot-proposals/issues/2946

when-decl

class_name SomeUI extends Control

when ready:
    print("ready!")

@oneshot
@onready
when $Button.pressed:
    var explode_node = explosion.instantiate()
    explode_node.set_transform(transform)
    add_sibling(explode_node)
    queue_free()

when gui_input(event: InputEvent):
    ...

this is nice. and generic. and solves all the problems i mentioned:

when ready:
    set_process(true)

when notified NOTIFICATION_PROCESS:
    print("processing")

this when notified ... stuff is not the main syntax syntax being proposed, but it fits well along side it, and it's useful in the same ways. it would also provide a way to convert 3.x projects that use func _process while keeping its meaning the same, a big problem for me when i converted 2 of my projects. bit late tho :P, 4.0 is already out still ive split it into 2 branches. https://github.com/rune-scape/godot/tree/rune-when-decl and https://github.com/rune-scape/godot/tree/rune-when-notified

most importantly, all these blocks of code (when <signal> and when notified) get called in the default notification order, starting from the highest ancestor (the Object class), first declaration to last (if there are multiple in the same script) this means that when ready: works just like func _ready(): in godot 3.x with the benefit of being able to declare multiple blocks with the same signal

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

there is a working version at https://github.com/rune-scape/godot/tree/rune-when-decl branched from 4.0.3-stable

I have been using this in the game i am working on for months now. i've fixed bugs as they come up, there's just a couple small highlighting bugs i know of currently i've had no problems so far teaching the syntax to the other programmers, one new to gdscript, one familiar

sdf-gen2.zip is a small gui project i made in an hour recently to create sdf textures (shader credit: queenofsquiggles)

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

yes, theres nothing you can do with this feature that wasn't possible before, it simply adds a generic clean syntax for declaring connections that is very useful in GUI design the only really new mechanic this brings is when declarations annotated with @onready get automatically refreshed when ready is re-emitted

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

not possible with an addon

theraot commented 1 year ago

I haven't looked at your implementation, but from your description it appears that the special case of when notified could prevent using this to connect to a signal called notified.

Something that could both free language space and solve that mentioned issue is to require self when connecting to the same object. So instead of:

when ready:
    set_process(true)

You do:

when self.ready:
    set_process(true)

And so there would be no issue with when notified.


I'm not sure how to feel about not using the keyword func in this syntax.

By the way, in general I'm in favor of declarative signal connections. A precedent that I like is VB.NET handle clauses.

rune-scape commented 1 year ago

@theraot :O thank you for the quick feedback, you're right that a signal named notified would have to be referenced as self.notified or some other way to be clear, notified is not a keyword, when is the only keyword added here. notified after 'when' is a special identifier that changes how it parses. not sure if that keyword was what you were talking about when you said 'language space' or if you mean the space of possible special identifiers after 'when'

Something that could both free language space and solve that mentioned issue is to require self when connecting to the same object.

i don't see why requiring this would solve that issue, if you understand that 'notified' has a special meaning here, and know how to refer to it another way, 'self.notified', that shouldn't be a problem (as long as no new special modifiers like it are introduced) although the 'notified' syntax isn't ideal in my mind, it is easy to remember as if it's a modifier in a sentence

i did consider adding a function notified to Object that takes a notification id (int), and returns a new/remembered signal that would be emitted when it was notified, but i thought it was too much and kinda defied the purpose of notifications

theraot commented 1 year ago

Right, so notified is not reserved, which makes sense.

It is what I'd call a contextual keyword (a non reserved word that has an special meaning in a particular context), which terminology from C#.

What I was thinking is that requiring self would allow for other such words aside from notified, which could be useful for future features. Otherwise adding other words in the future would be a breaking change.

Anyway, it is just my take.

rune-scape commented 1 year ago

your take is valuable! good point, but i can't think of a reason another contextual keyword would be added there, and i'd rather keep the intuitive "just a regular expression" feel to the signal expression it could also be its own keyword, i considered when_notified, but i hate underscores in keywords.

AThousandShips commented 1 year ago

While this could offer some convenience, I'm unsure how flexible it can be and how to handle a number of things like binds

Other than the specified case of convenience with lambdas I don't see much of an inconvenience in just the current connect approach, and I'm struggling to see the justification in adding quite a significant new feature and syntax, and significant complexity to the parser and compiler

For the _notification part I feel it breaks up the structure and again can easily be worked around without adding new complexity

I'd argue that while this could be convenient, it's so easy to work around that I don't feel it's worth the effort and complexity, there's also threading to be aware of, depending on when connecting and disconnecting happens etc.

sairam4123 commented 1 year ago

I can think of a solution for signals with arguments.

when self.ready:
    print("ready")

when $Button.toggled(button_pressed: bool):
     print("button toggled")

For binds we can do it the same way as current approach.

when $Button.pressed(button: Button).bind($Button):
     button.text = "Button was pressed"

I hope I am clear.

Shadowblitz16 commented 1 year ago

Wow this is a good suggestion.

This could also be done at top level and we could get rid the difference between things like ready and _ready

extends Node

# infinite loop
when ready:
   ready.emit() 

Note that code is just a example of how signals and callbacks could work I am not suggesting that you invoke ready from ready

rune-scape commented 1 year ago

@AThousandShips yes, this is easy to work around, i developed this while 4.0 was in development, it was meant to be submitted while gdscript 2.0 was in flux. i almost did before firefox ate my proposal (should have used google docs) this is meant to be the easy/intuitive/no-boilerplate top level option to try instead of Signal.connect like you would try declaring signal <name> before you try to use add_user_signal

how to handle a number of things like binds

if you mean binding arguments to the end of a function object, you would just call the method in the body

extends RigidBody2D

var player_id: int

when body_entered(body):
    do_action_player(body, player_id)

if you mean unbinding signal arguments, you just leave them unused:

when body_entered(_body):
    do_action_player(player_id)

or leave out the parameter list:

when body_entered:
    do_action_player(player_id)

there's also threading to be aware of, depending on when connecting and disconnecting happens etc.

connecting happens just after _init or just before _ready if its annotated with @onready. the same time most signals and methods get connected (the latter should arguable happen just after _ready but i couldn't find any reason it shouldn't be before) i'm not seeing a problem with threading

AThousandShips commented 1 year ago

Threading is a major concern with signals unless you're careful, this would risk worsening it by doing connecting in places that aren't safe:

Also keep in mind that if it's at _init you can only access globals not any nodes

rune-scape commented 1 year ago

:o wow thank you ! this is helpful, i didn't know this

Shadowblitz16 commented 1 year ago

Threading is a major concern with signals unless you're careful, this would risk worsening it by doing connecting in places that aren't safe:

* [Crash when connecting or disconnecting a signal from multiple threads godot#34752 (comment)](https://github.com/godotengine/godot/issues/34752#issuecomment-1564596292)

* [USER ERROR: Disconnecting nonexistent signal 'changed', callable: MeshInstance3D::_mesh_changed. godot#60098 (comment)](https://github.com/godotengine/godot/issues/60098#issuecomment-1582006327)

* [if AudioStreamPlayer2D is instanced from thread, sometimes doesn't connect to AudioServer godot#77518](https://github.com/godotengine/godot/issues/77518) (this is exactly the issue that could arise)

Also keep in mind that if it's at _init you can only access globals not any nodes

Surely these could be fixed?

mrpedrobraga commented 11 months ago

I'm not sure how to feel about not using the keyword func in this syntax.

An alternative to a new keyword other than func is an annotation.

@when(ready)
func _handle_ready():
    print("I'mmmm ready!!!!")

This would also convey that you can call this function without the signal being emitted.

@when(hit)
@when(died)
func emit_noise():
      print("Eugh!")

Signals with arguments:

@onready_when($GreetButton.toggled)
func handleHello (button_pressed: bool):
    if button_pressed:
        print("Hello!")
    else:
        print("Bye!")

Add bindings to specific connections...

@when($Button1.pressed, [$Button1])
@when($Button2.pressed, [$Button2])
func handleBtnPressed(button: Button):
     print(button.text, " was pressed")

For only connecting signals when they're emitted with a certain condition, I'd propose a new signal construction method that emits when the og signal emits with specific arguments.

signal hp_changed(new_hp: int)

@when(hp_changed.emitted_with([0]))
func handle_died():
     print("u ded")

@when(notified.emitted_with([NOTIFICATION_PREDELETE]))
func handle_about_to_delete():
    print("so long")

I do like the clean look of the original proposal, this mode here with @ looks rather clunky. But I do think it's more powerful, and less intrusive regarding new syntax, so easier to learn and implement.

The fragments required for this implementation (emitted_with) can even be used elsewhere though.

That is to say, there's many ways of implementing this if syntax is a problem -- I'm super on board with the idea of declarative connections like these.