godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 96 forks source link

There should be an easier way to duplicate signal connections setup via editor with advanced options #8087

Open MikeFP opened 1 year ago

MikeFP commented 1 year ago

Describe the project you are working on

A library of reusable component Nodes

Describe the problem or limitation you are having in your project

(Inspired by my comment here)

TLDR: The only way to fully duplicate nodes with signal connections setup with advanced editor options is by instantiating a scene.

I have some Nodes that are reusable and composable. They are supposed to be combined in a scene to achieve different behaviors. Sometimes, that includes setting up signal connections between them.

Some of those Nodes are actually just "templates" for their behaviors, and actually get duplicated at runtime, as needed. Ideally, following Godot's patterns, they should actually be put in a scene, and the scene would get instantiated at runtime. However, I consider that overkill. Imagine how many scenes the user would need to create just to make that work, even if the scene would only ever get used once in the codebase (although instantiated many times). I think Node.duplicate() should be enough for this usecase.

However, Node.duplicate only duplicates outgoing signal connections, but not incoming connections (see example section below). So, I tried to recreate the connections manually using Node.get_incoming_connections() (see work-around 2 below). However, the "Advanced" connection options (which can be setup while connecting signals in the Editor) are not present in the data returned by that method. Therefore, not all signal connections would be recreateable by code. I tried other work-arounds (see section below), but none of them worked.

Example

Consider a scene containing nodes A and B, with signals A -> B and B -> A.

In my usecase, those nodes would be duplicated. Let's call the duplicated nodes A copy and B copy.

Using Node.duplicate() by code on A, the connections from A copy -> B persist, as expected. However, duplicating only B, incoming connections from A -> B copy don't persist.

Failed work-arounds

  1. Setup A -> B as a standalone scene

Then, I could just PackedScene.instantiate(). However, that's not desirable for me. Those Nodes are already a part of a scene that has those connections. Making an extra internal scene just to encapsulate those connections is just boilerplate, in my opinion, and will scale badly with the amount of "template" nodes. Those Nodes can function by themselves already, and shouldn't need to be put in scenes just for the sake of it. Also, Node.duplicate() should cover this usecase, but it doesn't.

  1. Recreate incoming signal connections after duplicating

I tried doing that by reconnecting the signals using the info from Node.get_incoming_connections(), and it worked almost perfectly, except for this: the "Advanced" options setup via the "Edit signal connection" window are lost.

That breaks, for instance, if the connection was setup to a method with a different signature by using the "Unbinds" option. Since that information is saved in the scene, the Engine is able to connect those signals when the scene is instantiated, but the node itself doesn't have access to those advanced options in order to recreate the connection.

Therefore, this approach is way less powerful than connections setup via the Editor, and it won't work for every connection.

  1. Connect to the signal at runtime using exported variables

Since Signal can't be exported directly (as discussed here), it would look like this:

@export var signal_to_trigger: String
@export var node_with_signal: Node

func _ready():
    connect(node_with_signal, signal_to_trigger, my_method)

This would circumvent the need to duplicate the signal connections, since they would always be setup in _ready(). However, I'd still lose the ability to connect to methods of different signatures (see point 2), which is only available through the Editor. Additionally, I'd have to export the signal name, which is more error-prone.

  1. Dinamically pack a scene

The ability to connect signals with "Advanced" options is an Editor-only feature. The engine creates the connections with those options. However, that data is saved in the scene file, and thus, can't be easily retrieved by an existing Node.

If there was a way to pack a scene at runtime to later instantiate it, it would solve my problem, but the decribed issue would still exist in Node.duplicate.

My other bet was using InstancePlaceholder, but they got removed in Godot 4, and if I remember correctly, it only worked with scenes anyway, not regular Nodes.

If this work-around is already possible, please let me know.

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

I thought of some possible solutions, better detailed in the next section:

  1. Update Node.duplicate() to also duplicate incoming signal connections

Then, I could just use Node.duplicate() and everything would work.

  1. Store the advanced options used to connect a signal in the Editor

Then, I could use the info from Node.get_incoming_connections() to rebuild the incoming connections from scratch after duplicating the node.

  1. Expose a way to build a PackedScene from the tree at runtime

Then, I could create a new scene out of a branch at runtime, and PackedScene.instantiate() it as many times as I'd like.

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

Solution 1: Update Node.duplicate() to also duplicate incoming signal connections

The engine would have to access the "Advanced" connection options at runtime. I'm not sure that's possible now, since that data is stored in the scene file, and Node.get_incoming_connections() doesn't contain them either, which leads to solution 2.

Solution 2: Store the advanced options used to connect a signal in the Editor

For connections made by code, that information would have to be manually provided somehow while connecting a signal, since the code can't extract those values from context by itself (I think). The Signal.connect method could have these parameters:

# signal.gd
func connect(callable: Callable, unbinds: int = 0, binds: Array = null):
    pass

# my_node.gd
signal value_changed(value: int)
signal anything_changed()

func _ready():
    value_changed.connect(print_hi) # raises error
    value_changed.connect(print_hi, 1) # prints "hi"

    anything_changed.connect(print_value) # raises error
    anything_changed.connect(print_value, 0, [10]) # prints "10"

func print_value(value: int):
    print(value)

func print_hi():
    print("hi")

With that information, the Node.get_incoming_connections() could return a structure like this (the same could be done for Node.get_signal_connection_list()):

## Returns an Array of signal connections received by this object.
## Each connection is represented as a Dictionary that contains three entries:
##
## - `signal` is a reference to the Signal.
## - `callable` is a reference to the Callable.
## - `flags` is a combination of ConnectFlags.
## - `binds` is an optional Array of Variants passed as extra arguments to the callable.
## - `unbinds` is the optional amount of parameters that were discarded from the signal's callback.
func get_incoming_connections() -> Dictionary:
    pass

Solution 3: Expose a way to build a PackedScene from the tree at runtime

I don't really know the lower-level capabilities of scenes, so I'm not sure that's possible. But if it was, the whole problem could be avoided by doing something like this:

# The base scene is needed, because the file contains the advanced connection options.
var base_scene := PackedScene.new()
base_scene.parse_file(scene_path) # or just export a PackedScene reference.

var scene_branch : PackedScene = base_scene.branch_off(root_node_path)
var instance := scene_branch.instantiate()

As stated in work-around 4, that wouldn't solve the described limitations of Node.duplicate(), but would definitely solve my usecase. Although, I think it would be a lot of work to expose the methods the Engine uses for editing Scene trees under the hood, without breaking anything.

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

The only currently possible work-around is to save the branch of Nodes in their own Scene.

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

It requires handling data directly related to scenes and signals, which are core entities.

Mickeon commented 1 year ago

However, the "Advanced" connection options (which can be setup while connecting signals in the Editor)

What do you mean by "Advanced connection options". What does that include overall?

This would circumvent the need to duplicate the signal connections, since they would always be setup in _ready(). However, I'd still lose the ability to connect to methods of different signatures

Could you elaborate again? I feel like there may be a misunderstanding.

Also, a small sidenote, proposing multiple vastly different solutions to the same problem in the proposal in this format makes it somewhat difficult to read.

Zireael07 commented 1 year ago

What do you mean by "Advanced connection options". What does that include overall?

The signal connection dialog in editor has an advanced checkbox which enables e.g. adding parameters. This is what OP meant

Mickeon commented 1 year ago

I wanted to be very sure, to avoid sounding incredibly patronising, but I'll give it a shot now...

The ability to connect signals with "Advanced" options is an Editor-only feature. The engine creates the connections with those options.

This is not necessarily true, although I find it difficult to infer the exact meaning of this sentence. All the Engine does with those options is a combination of Signal.connect(), Callable.bind() and Callable.unbind(). That's all.

However, I'd still lose the ability to connect to methods of different signatures (see point 2), which is only available through the Editor.

In fact, this is not true at all. You can do just the same in code. For example, do you want to know what parameters are bound to the receiving callable? Use Callable.get_bound_arguments(). Do you want to strip some arguments away? Use Callable.unbind().

my_signal.connect(my_method.unbind(1), CONNECT_FLAG_DEFERRED)

Because of this, the result of Object's get_incoming_connections is all the information on how the signals and callables were connected. Nothing is hidden there.

MikeFP commented 1 year ago

Also, a small sidenote, proposing multiple vastly different solutions to the same problem in the proposal in this format makes it somewhat difficult to read.

Yes, I agree. I just put all of them to leave room for other contributors to point out which was more feasable. Then, I could make it the main one and edit out the other ones. But otherwise I agree.

This is not necessarily true, although I find it difficult to infer the exact meaning of this sentence. All the Engine does with those options is a combination of Signal.connect(), Callable.bind() and Callable.unbind(). That's all.

Yes, I'm aware this is how the engine connects the signals under the hood. All methods are available to bind/unbind arguments and such.

The problem is having the information to do so, for instance, consider:

my_signal.connect(my_method.unbind(1), CONNECT_FLAG_DEFERRED)

In that example, I must know the amount of parameters that should be unbound (i.e. the 1). When I connect a signal by code manually, of course I know that amount, since I'm aware of the signatures of the signal and the callable. But if it's a connection already setup, I'd need to retrieve the amount of parameters to compute that value, right?

In fact, this is not true at all. You can do just the same in code. For example, do you want to know what parameters are bound to the receiving callable? Use Callable.get_bound_arguments().

That seems like it should work. I tought the Callable would actually represent just the connected target method, and not the "inline" callable with the modified binds. I'm gonna try that out, and let you know. Thanks!

MikeFP commented 1 year ago

I tried it out, and it works perfectly!

I'm not sure it will work with lambdas too, but as far as I can tell, it works for connections to methods. This is the code I used:

func duplicate_with_signals(node: Node) -> Node:
    var dupe := node.duplicate()

    for connection in node.get_incoming_connections():
        var signal_ref: Signal = connection["signal"]
        var callable_ref: Callable = connection["callable"]
        var flags: ConnectFlags = connection["flags"]

        var dupe_callable := Callable(dupe, callable_ref.get_method())

        var bind_count_diff := callable_ref.get_bound_arguments_count()
        var args := callable_ref.get_bound_arguments()
        var should_unbind := bind_count_diff < 0

        if should_unbind:
            dupe_callable = dupe_callable.unbind(-bind_count_diff)
        else:
            dupe_callable = dupe_callable.bind(args)

        signal_ref.connect(dupe_callable, flags)

    return dupe

I guess I should've explored Callables more.

Still, I'd argue that there should be an option to do so automatically with Node.duplicate(), maybe as an extra DuplicateFlags. However, I guess that's not a common usecase to begin with?

If that's the case, let me know and I'll close the issue. Thanks for the help!