Skip to content
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 node_added and node_removed signals to Node (in addition to the existing SceneTree signals) #724

Closed
Tracked by #15
ghost opened this issue Apr 18, 2020 · 11 comments · Fixed by godotengine/godot#57541
Milestone

Comments

@ghost
Copy link

ghost commented Apr 18, 2020

Describe the project you are working on:
2D Shooters.

Describe the problem or limitation you are having in your project:
Tool code API and behavior issues.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
It would be useful in many tool scripts to be alerted when a Node's descending Nodes have been added or removed using a signal.

Currently, these signals only exists at the level of the SceneTree.

Within the editor this signal fires quite frequently, as the editor itself is part of the tree. This leads any filtering logic to have to constantly filter through hundreds of nodes, and can create serious performance issues.

It makes it difficult, error prone, and costly.

Would be better if each node could report what nodes have been added or removed from their hierarchy.

There are certainly use cases for this in run-time scripts as well, but I'll have to leave that to others to provide, as my development preference is to try to avoid adding/removing nodes from the scene tree as much as possible.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

The signals might be on Node, and the GDScript would follow as:

tool
extends Node2D

func _ready():

	connect("node_added", self, "node_added")
	connect("node_removed", self, "node_removed")

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

It usually ends up being a dozen or some lines with the most minimal work around. The issue though is mainly one of performance. Having to examine and act on all these editor related nodes with each interaction in the editor.

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

Two built-in signals would be far more convenient than rebuilding with a module, and the plugin wouldn't solve the performance issue.

@Xrayez
Copy link
Contributor

Xrayez commented Apr 18, 2020

I think this would be quite useful needed, I proposed this some years ago too in godotengine/godot#19801 (comment)

The PR linked had to do with the XY problem which is actually this proposal.

@Xrayez
Copy link
Contributor

Xrayez commented Apr 18, 2020

The issue though is mainly one of performance.

Might also fill up the message queue leading to overflow with so many nodes being added as children to each, but hopefully this can be implemented: godotengine/godot#35658.

@ghost
Copy link
Author

ghost commented Apr 18, 2020

I see. I would want the signals to include the children's children. As the tool scripts very often are attached to nested nodes, and may also be managing them multiple levels deep.

From reading your past PR, it probably would be handy to have another method like is_the_parent_of() for direct children queries. If it were adjacent to the other in the API, it would be easy to reference which did what.

But it seems you encountered a lot of resistance on the idea.

@Xrayez
Copy link
Contributor

Xrayez commented Apr 18, 2020

I would want the signals to include the children's children

Oh ok, I was just thinking about direct children, with child_added name making this more clear. So we have actually several options to consider:

  1. signal node_added(node) for both direct and indirect children
  2. signal node_added(node) for indirect children, and signal child_added(node) for direct ones.
  3. signal node_added(node, direct: bool)

I think the (3) option is self-explanatory and could serve most use cases without having to add yet another signal to core.

From reading your past PR, it probably would be handy to have another method like is_the_parent_of() for direct children queries.

Well yeah that's just misunderstanding in the first place, node.get_parent() == self is kind of enough to filter direct children. In fact, adding signal node_added(node, direct: bool) would prevent me from making godotengine/godot#19801.

@ghost
Copy link
Author

ghost commented Apr 19, 2020

I personally would go for extra signals like child_added/removed. Just a preference/bias though for when I'm reading code later.

It's mostly regarding methods that are filled with true/false arguments. I would have to stop reading and double check the signatures.

With signals though, they're implemented rather than called, so the signature is in place.

My name pick would be node_added(node, is_child)

Well, without being familiar with the SceneTree implementation, I wonder if there is an efficient way to emit the signals.

I imagine it's worse to have it always do filtering logic that is only going to get utilized in a minority of the scripts, and not all projects.

@Jummit
Copy link

Jummit commented Apr 19, 2020

I'd like child_added better, because it is more specific.

@Xrayez
Copy link
Contributor

Xrayez commented Apr 19, 2020

I'd like child_added better, because it is more specific.

If it does what it does on the tin that's good, but if we aim to combine both logic (direct children and indirect siblings of a node passing through a signal), then this could lead to confusion IMO.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 20, 2020

After learning a bit more about engine internals, I've found a way to expose existing functionality, which may as well overcome limitations as described in this proposal, see godotengine/godot#43729.

@me2beats
Copy link

I need these signals too
Node.child_added, child_removed, child_moved

I am creating a container where I need to do something when the user adds, removes and moves children in that container.

I also need this to work in tool mode.

It seems that the only way to do this in this case is to override the add_child(), remove_child(), move_child() and other methods, which will emit the corresponding custom signals. But I think this is kinda ugly

@me2beats
Copy link

As for node_added signal, I think node_added would be less performant than (direct) child_added, wouldn't it?

@me2beats
Copy link

One more example when it would be convenient to have signals that a child has been added / removed:
I need to prevent the user from adding children to a specific node
including adding a node from the editor.

Having a child_added or node_added signal (in this case, I would prefer to use child_added), I could connect to it from any node and as soon as the user adds a child, I would remove it (and possibly push a warning).

This also applies when I would like to check that the user only adds children of certain types (for example, only controls) without the need to force the user to modify their nodes and their scripts so that they can work with mine ones.

@akien-mga akien-mga added this to the 4.0 milestone Feb 2, 2022
@Calinou Calinou changed the title Nodes would also benefit from having node_added and node_removed signals. Add node_added and node_removed signals to Node (in addition to the existing SceneTree signals) Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants