Tighten up Node memory leak areas

[cgbeutler]

Describe the project you are working on

A 2D platformer. (Examples in C#, but applies to GDScript as well.)

I have a few areas where I create nodes in code that are then added to the tree. There are a few areas where I move nodes out of the tree to maintain data, but stop processing.

Describe the problem or limitation you are having in your project

Nodes created from code are leaked by default. That's scary.

When creating nodes from code, you have to be sure to add the node to the tree right away or you could leak a node. A naïve dev may do:

var cr = new ColorRect();
cr.Color = player.Color;
AddChild( cr );

but say player is null. Then the node will be leaked due to the null exception thrown!

Nodes also get hairy when you don't want to put them in the tree or you want to move them around.

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

To tighten existing memory issues I propose 2 changes to ensure Nodes are safer.

1. When nodes are not in the tree, they can be wrapped in a NodeRef handle that is reference counted and deletes the node on deref. This could lead or a danger of circular references, but it would still be safer than things stand now.
2. Encourage node constructors that take a parent Node, ensuring that nodes are tracked from allocation onward. This would plug the leak by making sure nodes made from code are not leaked by default. (Still seems wild to me that that is the case!)

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

When removing children from the tree, the method could return a NodeRef type.

var childRef = RemoveChild( GetChild(0) );  //Returns NodeRef
childRef = null;  //Node gets queued for deletion

This reference-counted handle would delete the node if it's reference count reaches 0. If a non-breaking version is desired, it could be called RemoveChildSafe or something.

When nodes are created, the preferred constructor would take a parent node to ensure it is tracked from the moment of allocation.

var colorRect = new ColorRect( this );

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

This proposal could exist in parallel to the way things are done now or be designed so they can't be worked around (to ensure extra safety from scripts.) I, personally, don't care which.

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

This is designed to help core be safer. Something like NodeRef may be doable in an asset library.

[girng]

I might be misunderstanding your post, but weakref comes to mind, would that work?

[cgbeutler]

I might be misunderstanding your post, but weakref comes to mind, would that work?

Nope. That's something else entirely. WeakRef does NOT delete nodes for you.

For those who may not know how memory management works in Godot, here's a brief overview.

Any Reference-derived class is reference counted. That means things like resources add 1 to a counter every time they are referenced. They subtract 1 when stuff stops referencing them. When that number reaches 0, the reference is deleted. The reason GDScript is so touchy about circular references is because circular references mean the counter may never reach 0 and could leak objects. WeakRef is a way to reference a Reference without adding to the counter. Its like saying, I want to be able to get this data, but I don't need it if no one else is using it anymore.

Any Node-derived class uses the tree to manage its memory. Nodes often need to refer to each other, as their parent-child relationships send a lot of data both ways. When a branch on the tree is deleted, all its children are also cleaned up. If you remove a node from the tree or never put it there, then that node is leaked and you will get a notice when you exit Godot that something was leaked. WeakRef can point to a node, but even a regular, non-weak ref would die if the node died. It really doesn't offer anything new other than making it clearer that you may loose the node.

If it doesn't inherit from Node or Reference, then you have to manage the memory all by yourself. You have to call delete. When a node is not in the tree it acts like the rest of the object types and must be cleaned up by YOU. This sounds a touch scarier than it is. Most of the code around nodes is pretty safe. The few dangerous ones warn you in the docs. This proposal just seeks to make nodes a touch safer by cleaning up the few other little areas where nodes are not in the tree, like when a node is freshly created.

Between the Node and Reference systems of memory management, Godot can avoid using a more complex garbage collector, (which can be slow or cause big GC slowdowns for a frame or two. You may have seen a game freeze for a sec if written in a GC Language.)

[mieldepoche]

Asking out of curiosity since I don't know the inner workings of godot that much but why not simply make Node extend Reference? What kind of unwanted effect could this have? Wouldn't this simply prevent such leaks?

[cgbeutler]

...why not simply make Node extend Reference? What kind of unwanted effect could this have? Wouldn't this simply prevent such leaks?

Nodes were opted out of reference counting because they often are passed up and down the tree to one another. Calls like like GetNode or GetParent would become very dangerous circular reference makers if Nodes were always reference counted. I mean, parents and children always reference each other atm. A case could be made for making them references, but that case would be really hard to sell, as it would involve a lot of WeakRef hackery. It kinda makes more sense to only reference count nodes when not actively in the tree by giving them a handle that is reference counted.

[viksl]

So does this mean making the parent mandatory for a new node? I think this would be problematic for many cases, such as preloading without parent, preparing pooling objects, preparing nodes through threading which doesn't provide parent too. Just in general preparing any node before adding it. Unless i misunderstood this proposal.

[cgbeutler]

So does this mean making the parent mandatory for a new node? I think this would be problematic for many cases, such as preloading without parent, preparing pooling objects, preparing nodes through threading which doesn't provide parent too. Just in general preparing any node before adding it. Unless i misunderstood this proposal.

For those cases i picture passing in null, so it's explicit, or passing In a new node handle, which becomes the owner in the meantime.

[jamie-pate]

You can make a version of weakref that works with node by abusing the set_meta() to put a RefCounted token in the metadata..

When the node is deleted the token will be deleted.. your NodeRef could hold a weakref to that token..

godot4_node_ref.zip