Add a way to remove all children from a node quickly

[Shadowblitz16]

Describe the project you are working on:
a game in which I am generating a ton of nodes

Describe the problem or limitation you are having in your project:
I am trying to generate a 16x16 plane of objects however everytime I edit the width and height of the grid it needs to be cleared and updated

doing so by looping through all nodes is too slow in gdscript and there needs to be a way to clear a branch all at once.
possibly through pointers

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
I suggest remove_children() be added as a work around.
this would be a fast way of clearing all the children of a node

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
node.remove_children()

If this enhancement will not be used often, can it be worked around with a few lines of script?:
idk if it would be used often but we already had a merge request here but was shut down

Is there a reason why this should be core and not an add-on in the asset library?:
because its a simple way to manage nodes without slowing down godot

[Xrayez]

I've been using godotengine/godot#8337 without issues so far. I work with images and use a grid container for color palettes, spawning random objects given random seed and whatnot.

If the issue that some children are actually essential for correct functionality of a node (quite a bunch of the GUI nodes), these kind of nodes should be made internal as suggested in godotengine/godot#30391.

[groud]

doing so by looping through all nodes is too slow in gdscript and there needs to be a way to clear a branch all at once.

What do you mean it is too slow ? Do you have some numbers ? Are you sure this comes from gdscript ? Because I highly doubt your bottleneck come from gdscript, the simple removal and re-instanciation of a node is a demanding task, whether or not the call to remove_child() is made in gdscript or in native code will likely make no significant difference.

[Xrayez]

Providing some GDScript methods:

extends Node

static func queue_free_children(node: Node) -> void:
	for idx in node.get_child_count():
		node.queue_free()
		
static func free_children(node: Node) -> void:
	for idx in node.get_child_count():
		node.free()

# Usage
func _ready():
	queue_free_children(self)
	free_children(self)

propagate_call("queue_free") doesn't work because we actually need to save parent... 😄

By the way, if Godot really tries to be simplistic, we can remove Node.get_children() because it can be easily done via script similarly:

static func get_children(node):
    var children = []
    for idx in node.get_child_count():
        children.push_back(node.get_child(idx))
    return children

But I guess it's used more often which justifies it being in core...

[groud]

But I guess it's used more often which justifies it being in core...

Yes, that the difference actually. Iterating over nodes is really common.

Removing all nodes, and very likely re-instanciating them afterwards as OP does, is a really bad practice. Re-instanciating nodes instead of updating them is significantly more resource demanding, so encouraging such practices by simplifying the API for doing so is not really a great idea.

[RabbitB]

Maybe I'm misunderstanding the issue, but if removing all children from a node is a common task in your program, couldn't you streamline the process by removing the parent and instancing it again? Or if that's not feasible, create a child under the current parent, that will contain all the nodes that will regularly need to be cleared, so you can remove the single child instead of looping through all of them?

queue_free also frees any children when called on a Node, so it sounds like what you're trying to accomplish is already easily done. It just requires a slightly different solution than what you were already attempting.

[Shadowblitz16]

@groud I tried to generate a 16x16x1 grid of spatial nodes and it took like 3 seconds for 1 loop
I even added a yield to make it not lock up the editor
@RabbitB I would then have to make packed scene for it and even then its children would be instanced again

I don't see why making a engine side function that does

for n in get_children(): remove_child(n) 

would be a big issue the only difference would be that it would be faster

[groud]

@groud I tried to generate a 16x16x1 grid of spatial nodes and it took like 3 seconds for 1 loop

Once again, this still does not mean that moving the loop to native code is going to make the code significantly faster. If your code spends 99% of its time inside the removing and instantiating code, which is very likely the case, optimizing the loop will only give you a 1% speed increase.

And we won't optimize an API just to win a 1% speed increase, especially if it encourages bad practices (as I said earlier, fully replacing nodes is significantly heavier than updating them).

[Xrayez]

fully replacing nodes is significantly heavier than updating them

While I have to agree with this statement, in some cases it's important to have reproducible results. Yeah you could allocate some maximum number of nodes and update them manually, but that comes with the cost of potential inconsistent results between successive iterations/generation of a game level. Some other aspects to keep track of like ensuring node's signals are also re-connected to callbacks on the next generation. This is all handled by the queue_free.

The way I see it, it's not necessarily about speed, but about consistent results, QoL, and additional performance gain which comes from not using GDScript for loop-related heavy computations.

queue_free also frees any children when called on a Node, so it sounds like what you're trying to accomplish is already easily done. It just requires a slightly different solution than what you were already attempting.

That's a good workaround, but again we're talking about usability of the engine achieved out-of-the-box, with additional benefit of a slight increase in performance, every bit counts. 🙂

[groud]

every bit counts

Not at the price of a bloated API.

[Xrayez]

every bit counts

Not at the price of a bloated API.

Little things make big things happen. 😛

I actually have a counter proposal.

Currently propagate_call() works by calling methods on the children and the caller itself. What about adding an option to exclude the caller (which would prevent the caller from being freed)?

I mean something like:

propagate_call_children("queue_free")

Not only this would be useful for this particular proposal, but many other use cases where we want to exclude the parent by propagating calls like that.

Adding another boolean option to exclude parent would still be a bloat to API, nor adding a new method like this though.

Isn't every new addition is actually a bloat?

[Shadowblitz16]

@groud I can garentee you 1000% that it would be alot more 1% I would say at least 50% if not more

I don't need to bench mark gdscript to tell you how slow it is.
people have already done it and its and least 10 times slower then C#

[groud]

I don't need to bench mark gdscript to tell you how slow it is.

This is your assertion, which, once again, has to be proven true. Please do a benchmark.

It's simple to do, make the same loop without the function call (or eventually with a dummy call to a function), and you will see how little time it take to iterate over 16 x 16 objects... Then compare the time to the same loops with queue_free(), and the difference will be the time spent in the call to queue_free().

GDscript is slower than c++, it does not make it unable to rapidly go over 256 object...

[groud]

Alright, so I made the benchmark myself anyway (I added a button to run the _on_Button_pressed function) :

extends Node

func _ready():
	for i in range(16):
		for j in range(16):
			var node = Node3D.new()
			add_child(node)

func _on_Button_pressed():
	var time_before = OS.get_ticks_usec()
	for child in get_children():
		child.queue_free()
	var time_after = OS.get_ticks_usec() 

	print(str(time_after - time_before) + "us")

This prints:

495us

So it takes 495us (microseconds) to call queue_free on 256 nodes in gdscript... There is absolutely no way your bottleneck is here. Let's imagine your target fps is 60fps, so a frame duration is 16ms, and you could divide those 495us by 10. In the end you would end up with an improvement of (0.495 * 9/10) / 16 = 2.8%. And this would be if you absolutely wanted to do this every frame, which in not advised at all.

In your case where the whole deletion takes 3seconds, the improvement would be around (0.495 * 9/10) / 3000 = 0.015%. So, as I said, far less than 1% of the time it takes to actually remove the nodes.

And in fact, this is perfectly understandable, as queue_free() delays the freeing of the nodes at the end of the frame, so it does not do much directly. So in the end, optimizing this part of the code in C++ would make absolutely no sense.