Emit a freeing signal when queue_free is called on a Node

[hunterloftis]

Describe the project you are working on

A 2D game with large, procedurally-generated levels with lots of entities being spawned, added, removed, re-added, & freed over the lifetime of each top-level scene.

Describe the problem or limitation you are having in your project

Many nodes create dynamic references to other nodes in the game. For example:

• Targeter -> Enemy
• Item -> Wielder
• Inventory -> Item

The referenced nodes may be freed. Enemies, Wielders, and Items can all be destroyed. They can also be removed from the scene tree temporarily, and moved to other locations in the scene tree.

Initially, this led to runtime crashes when I tried to work with the original references. I patched that with is_instance_valid, but that's unreliable and can leave things in a weird state. I checked for a Node signal that I could listen to instead, but the closest one - tree_exiting - sends false-positives:

Called when the node is about to leave the SceneTree (e.g. upon freeing, scene changing, or after calling remove_child in a script)

So, I've ended up adding this everywhere:

signal freeing

func safe_queue_free() -> void:
    freeing.emit()
    queue_free()

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

Add a freeing signal to Nodes that will fill the gap. In addition to this specific use case, it would generally allow developers to stop leaning on is_instance_valid(), which is a slow anti-pattern.

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

It would be used by the nodes that hold references to potentially-freed objects, like:

var _target: Enemy

# ...

_target = _get_nearest_enemy()
if _target:
    _target.freeing.connect(func(): _target = null)

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

Given how commonly this is asked about on discord/reddit/Q&A/stackoverflow/etc, I expect it will be used often. Godot doesn't really have a canonical answer for dealing with freed nodes, and is_instance_valid is a bandaid.

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

Design-wise, it's a corollary to the queue_free method defined on Node. The signal that a behavior is about to occur should be defined alongside the behavior.

[YuriSizov]

You can use the NOTIFICATION_PREDELETE notification if you need to handle that specific moment in object's life cycle. I'm pretty sure that emitting signals in that moment (or subscribing to them) would be problematic and could lead to crashes.

[theraot]

I see what you mean about tree_exiting (which also applies to tree_exited), you can check is_queued_for_deletion which will tell you if it is as a result of queue_free.

---

With that said, be aware that there is a NOTIFICATION_PREDELETE to notification, which you can get like this:

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        prints("predelete")

---

The reason for is_instance_valid is that when an object is freed, variables that point to it do not become null automatically ※1. Of course this is not a problem for RefCounted ※2, but Nodes are not RefCounted.

※1: which would require either: go over all the variables and update them, or hide the logic is_instance_valid inside the == operator, or inject the check in every access.

※2: as when they are freed, we assume there are no variables pointing to them, barring WeakRef and messing with reference and unreference.

Please notice that what we have is performant and does not hide logic.

[hunterloftis]

NOTIFICATION_PREDELETE is a notification that the to-be-freed object receives on itself, correct? Is there a way that be listened to by other objects that are holding (soon-to-be-invalid) references to it? If not, it doesn't seem relevant, unless maybe you're suggesting to change my workaround to something like this?

signal freeing

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        freeing.emit()

The reason for is_instance_valid is that when an object is freed, variables that point to it do not become null automatically

Understood, that's why a signal would enable references to be nulled by the user. To clarify, this is not a suggestion to auto-null references, but to provide a new capability for the user:

_target.freeing.connect(func(): _target = null)

TIL about is_queued_for_deletion, thanks! In that case, I should be able to work around this more simply (without having to extend the referenced classes), via something like:

_target.tree_exiting.connect(func(): if _target.is_queued_for_deletion(): _target = null)

... which is nearly as good as a freeing signal.

[AThousandShips]

Sure it can be seen as an anti-pattern, but having multiple references to a non-refcounted type isn't really recommended, I don't see adding a signal to handle this over just using is_instance_valid when you do insist on using multiple, non-handled references to a type like this

Having this check is a bit of a luxury tbh, there's no equivalent in c++ and there's no safe way to check if a non-null pointer points to valid memory or not

[hunterloftis]

Having this check is a bit of a luxury tbh, there's no equivalent in c++ and there's no safe way to check if a non-null pointer points to valid memory or not

I'm not sure that comparing Godot to C++ is useful, since of course Godot's value proposition is operating at a much higher level of abstraction than C++. By definition, it offers "luxuries" that C++ doesn't have.

having multiple references to a non-refcounted type isn't really recommended

What is the alternative in Godot for nodes that need to interact with one another? I'd like to use whatever the recommended pattern is here, but I haven't seen any recommendation documented anywhere.

[AThousandShips]

Using is_instance_valid, or ObjectID, or make sure that whenever you free something you also clear the values

I'd say that if you aren't able to make sure that your references are tracked clearly, by making sure you do the freeing with clear intent

I don't see the value in adding new signals, which reduces performance, to handle usage that isn't IMO recommended, emitting signals isn't free

[hunterloftis]

make sure that whenever you free something you also clear the values

Yes, this is the purpose of the safe_queue_free example above.

Do you have another mechanism in mind for clearing references whenever something is freed, in a system that has no "freeing" signal to listen to?

[AThousandShips]

Have what ever mechanisms calls the freeing also perform this, since you're suggesting a new method anyway that won't be fired by the general freeing, which deals with the performance, but then you're already making the manual decision so you can just work around this instead, for these cases (that most users won't be dealing with AFAIK)

[hunterloftis]

Searching for answers to the issue has led me to dozens of people posting about the same problem, all seemingly looking for a good solution from godot:

https://www.reddit.com/r/godot/comments/s6g1mw/psa_question_use_is_instance_valid_instead_of/

...but if it's not a priority, I will continue to work around it as I'm already doing. This was just a suggestion for a general-purpose solution.

[AThousandShips]

Well the solution is is_instance_valid or using ObjectID I'd say, in cases you can't rely on the reference remaining valid

[hunterloftis]

I have been warned away from is_instance_valid in the past - iirc from core Godot maintainers - due to unreliability, reuse of references pointing to the same address, and mismatches between debug and release builds. Has it become reliable in a particular version of Godot?

[AThousandShips]

It ensures that the pointer is valid and points to the same pointer, so it would require an extremely unlikely event, i.e. the ObjectID and pointer to both match between two different objects

[theraot]

due to unreliability, reuse of references pointing to the same address, and mismatches between debug and release build

See godotengine/godot#36189 for Godot 4, and godotengine/godot#51796 for Godot 3.

[jinyangcruise]

I support this proposal because such cases exist:

node.freeing.connect(func():
    if xxx:
        node.cancel_free()
)

The point is that sometimes the freeing node doesn't have a script in which we can add some code like:

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        cancel_free()

or even if we can attach a script to this script only to add such code above, it's too heavy and clumsy.

[jinyangcruise]

One example is

void SceneReplicationInterface::_free_remotes(const PeerInfo &p_info) {
	for (const KeyValue<uint32_t, ObjectID> &E : p_info.recv_nodes) {
		Node *node = tracked_nodes.has(E.value) ? get_id_as<Node>(E.value) : nullptr;
		ERR_CONTINUE(!node);
		node->queue_free();
	}
}

https://github.com/godotengine/godot/blob/99a8ab795d65e816ea7c452aa0fb55d02385c048/modules/multiplayer/scene_replication_interface.cpp#L83-L89

Currently MultiplayerSpawner does not support leaving the spawned node remaining in the scene tree when peer disconnected because SceneReplicationInterface directly call node->queue_free() as above. So in order to prevent this, node must have script containing this:

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        if xxxx: # user's choice whether to retain this node
            cancel_free()

I'm working on a multiplayer add-on to simplify multiplayer playing, so it's unacceptable that users must add these codes to their spawnabel scenes. If there is a freeing signal, the cancel_free() code can achieved within the scope of the add-on.