Godot: queue_free doesn't work if node has been removed from the SceneTree

[GalanCM]

Operating system or device - Godot version:
Mac OS and Android, at least

Issue description:
The queue_free function doesn't free objects from memory if they aren't in the SceneTree.

Steps to reproduce:
Call queue_free() on a node that had been removed from the SceneTree via remove_child, the object count doesn't change and NOTIFICATION_PREDELETE never occurs. Using call_deferred('free') works.

Link to minimal example project:
queue_free bug tester.tscn.zip

In the project, if the main node's "fix" script variable is false, it deletes "Child" via queue_free, and if true, it uses call_deferred('free'), when the "Child" node is deleted, it should print "Success!" to the console.

[RandomShaper]

I've not been able to reproduce it on 2.1.3 under Windows.

[neikeq]

This is not a bug. queue_free can only be used if the node is inside the SceneTree. You should be getting this error: Condition '!is_inside_tree()' is true.

As you have found out, free must be used if the node is not inside the SceneTree. e.g.:

if is_inside_tree():
    queue_free()
else:
    call_deferred("free")

[bojidar-bg]

@neikeq I guess it would be better if the Node class ran this check automatically.. 😃

[neikeq]

The situation in which not only you delete a node manually, and it also happens to be outside the scene tree is very rare. Most of the times you either let the parent control its lifetime, or you delete it while it's inside the tree.

[HummusSamurai]

@neikeq by letting a parent control its lifetime, you mean using remove_child() ?

[neikeq]

No, I mean that when the parent is deleted, it will delete its children automatically.

[GalanCM]

I'll admit this is an edge case. I believe I was either doing it to avoid a particular bug, or for a small performance boost (it's been awhile since I wrote it). But if this is intended, it _is_ unintuitive, and could at least use a less obtuse error, especially since it can lead to a memory leak.

[akien-mga]

I'd propose this change to avoid this confusion:

diff --git a/scene/main/node.cpp b/scene/main/node.cpp
index 319f123da..64dd53e0f 100755
--- a/scene/main/node.cpp
+++ b/scene/main/node.cpp
@@ -2572,8 +2572,11 @@ void Node::print_stray_nodes() {

 void Node::queue_delete() {

-       ERR_FAIL_COND(!is_inside_tree());
-       get_tree()->queue_delete(this);
+       if (is_inside_tree()) {
+               get_tree()->queue_delete(this);
+       } else { // not in tree, free directly
+               memdelete(this);
+       }
 }

 Array Node::_get_children() const {

[volzhs]

@akien-mga when we do node.queue_free(), we should be able to refer that instance in current frame.
but the changes prevent to do that.

[akien-mga]

@volzhs That's right. @reduz suggested something like this:

diff --git a/scene/main/node.cpp b/scene/main/node.cpp
index 319f123da..0ab41b40d 100755
--- a/scene/main/node.cpp
+++ b/scene/main/node.cpp
@@ -2572,8 +2572,11 @@ void Node::print_stray_nodes() {

 void Node::queue_delete() {

-       ERR_FAIL_COND(!is_inside_tree());
-       get_tree()->queue_delete(this);
+       if (is_inside_tree()) {
+               get_tree()->queue_delete(this);
+       } else {
+               SceneTree::get_singleton()->queue_delete(this);
+       }
 }

 Array Node::_get_children() const {

[volzhs]

oh. that looks nice :+1:

[volzhs]

@akien-mga wait...
can we just use a single line SceneTree::get_singleton()->queue_delete(this); then?

[akien-mga]

@volzhs There can be several SceneTrees in theory (though rarely in practice), and I think we want the node's own tree to handle its deletion when relevant.

[volzhs]

got it. :)