Expose Node callbacks for adding and removing children

[Xrayez]

Helps godotengine/godot-proposals#724.
Helps godotengine/godot-proposals#2544.

I've found out that the engine already has such mechanism through virtual add_child_notify, remove_child_notify, and even move_child_notify (currently used only on C++ level for Control nodes such as GraphEdit).

With this, you don't have to:

1. get_tree().connect("node_added", self, "_on_node_added") and filter nodes with get_parent() checks.
2. Poll the children being added and removed from current one manually in _process().

This way, there's no need to introduce signals such as node_added/child_added, and instead use existing callbacks.

I don't know the performance impact behind get_script_instance() checks yet, but perhaps they're negligible enough for this feature to be implemented. The _process callback checks this every frame in contrast, so I think there's nothing to worry about performance-wise.

Usage

extends Node2D

func add_child_notify(child):
    # Request redraw whenever the child is "changed" is some way.
	child.connect("something_changed", self, "update")

func remove_child_notify(child):
	child.disconnect("something_changed", self, "update")

This is mostly useful for implementing general-purpose, adaptable classes to handle the API written by other developers, or when better encapsulation is desired (parent manages/controls children, and not vice versa, where get_parent() usage is typically discouraged).

---

I made this for 3.2 currently, 4.0 needs a separate PR due to removal of multilevel calls, but only if the idea behind godotengine/godot-proposals#724 is approved of course.

Test project:
expose_node_callbacks_add_remove.zip

[Xrayez]

Oh well perhaps I'm simply over-engineering this, because it's possible to "override" add_child etc via script:

func add_child(child: Node, legible:bool = false) -> void:
	.add_child(child, legible)
	print("New child added: " + str(child))

func remove_child(child: Node):
	.remove_child(child)
	print("Existing child removed: " + str(child))

But as you see, this (talking about Godot 3.2):

1. Requires specifying the exact signature used in parent class (including default values), else you'll end up with:

2. Call the parent method explicitly, else it would shadow the method.

3. Not intuitive at all, the existence of this PR is a clear example for this, and likely feature requests such as Add an "added_child" signal to Node #11260. 😛

Also, I'm not sure whether that's only GDScript's feature, or even a bug as a feature, due to things like #42968 (I've completely forgot about this behavior even after working on this, duh 😕).

Anyways, that's what C++ part of the engine provides, that's how I've discovered this in fact, so perhaps these can also be exposed to scripting, for:

1. Consistency.
2. Documentation purposes (explicit is better than implicit, as they say!)

[ghost]

it's possible to "override" add_child etc via script:

Interesting, didn't know these were available to override. That'll be a great work around.

Tested this build and it doesn't work for all nodes. Not 100% of how the virtual definitions work here, but these methods seem to exist for Node specific CPP implementations. Things like Control appear to have their own handling of it. I may of missed something, but it didn't appear as though add_child_notify() was able to be overridden in Controls.

godot/scene/gui/control.cpp

Lines 438 to 447 in 5b99365

	void Control::add_child_notify(Node *p_child) {
	Control *child_c = Object::cast_to<Control>(p_child);
	if (!child_c)
	return;
	if (child_c->data.theme.is_null() && data.theme_owner) {
	_propagate_theme_changed(child_c, data.theme_owner); //need to propagate here, since many controls may require setting up stuff
	}
	}

When looking at this, I was thinking it might be easiest to make a new signal and add it as the last line in these functions:

godot/scene/main/node.cpp

Lines 1165 to 1182 in 5b99365

	void Node::_add_child_nocheck(Node *p_child, const StringName &p_name) {
	//add a child node quickly, without name validation
	p_child->data.name = p_name;
	p_child->data.pos = data.children.size();
	data.children.push_back(p_child);
	p_child->data.parent = this;
	p_child->notification(NOTIFICATION_PARENTED);
	if (data.tree) {
	p_child->_set_tree(data.tree);
	}
	/* Notify */
	//recognize children created in this node constructor
	p_child->data.parent_owned = data.in_constructor;
	add_child_notify(p_child);
	}

Something like:

	/* Notify */
	//recognize children created in this node constructor
	p_child->data.parent_owned = data.in_constructor;
	add_child_notify(p_child);
	emit_signal("node_added", p_child);
}

This would also avoid the if branching you're concerned about.

[Xrayez]

Tested this build and it doesn't work for all nodes. Not 100% of how the virtual definitions work here, but these methods seem to exist for Node specific CPP implementations. Things like Control appear to have their own handling of it. I may of missed something, but it didn't appear as though add_child_notify() was able to be overridden in Controls.

Control::add/remove_child_notify was not calling base method (unlike other usage of this method in the engine), fixed now. Technically that can be considered an existing bug in the engine, but also I think that this could be avoided by calling those script callbacks directly in _add_child_nocheck and remove_child where these are actually called. Both approaches work, it's just that the first one allowed to catch an existing limitation as reported by you. 🙂

[Xrayez]

When looking at this, I was thinking it might be easiest to make a new signal and add it as the last line in these functions:

The reason why I went for approach with exposing callbacks is the previous rejection/concern of the idea that node_added/removed signals per node could potentially negatively impact performance for a use case which is not that common, again referring to #11260.

But yeah the use case is also described in #12224.

[ghost]

could potentially negatively impact performance

Not sure what you mean, is this speculative, or are you referring to a use case that you measured?

Control::add/remove_child_notify was not calling base method (unlike other usage of this method in the engine), fixed now.

Tested, appears to be working now.

[Xrayez]

could potentially negatively impact performance

Not sure what you mean, is this speculative, or are you referring to a use case that you measured?

I haven't tried to implement per-node node_added/node_removed signals, so I cannot tell what performance impact would this have, again it might be negligible since signals like tree_entered are also emitted, and according to reduz, signal handling should be efficient by design, so I don't know really.

I'll amend the PR description to say it only helps your proposal then, because it seems like we might have different use cases.

[samdze]

My main use case with this PR would be getting notified of children added when a scene is istantiated, so that each node in the istanced scene receives all the relevant notification for all of their children, top to bottom.
Is this use case covered?

[Xrayez]

@samdze if I understand correctly, then yes, add_child_notify(child) would be called in any case, both while instancing and adding nodes at run-time manually with add_child(node), and NOTIFICATION_READY (onready) would be called after all add_child_notify(child) callbacks has been called.

If what you ask for is getting notification to scene root, the closest thing you can use is NOTIFICATION_INSTANCED, but this is likely not what you ask.

[akien-mga]

We just merged a PR which solves the two related proposals with new signals: #57541.

That being said we discussed this PR in a PR review meeting too and it seems still relevant to expose the existing Node _notify callbacks to the scripting API. This should be redone for master using the new GDVIRTUAL macro which makes defining and using virtual methods more efficient and user-friendly.

[Xrayez]

Thanks, this PR would be nice to implement, but it's possible to workaround it, especially with #57541. If someone feels motivated enough, feel free to build upon this PR, as I don't currently use master branch.

[akien-mga]

Superseded by #62661.