Open hunterloftis opened 1 year ago
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.
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 Node
s 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.
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.
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
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.
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
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?
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)
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.
Well the solution is is_instance_valid
or using ObjectID
I'd say, in cases you can't rely on the reference remaining valid
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?
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
due to unreliability, reuse of references pointing to the same address, and mismatches between debug and release build
See https://github.com/godotengine/godot/pull/36189 for Godot 4, and https://github.com/godotengine/godot/pull/51796 for Godot 3.
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:
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:So, I've ended up adding this everywhere:
Describe the feature / enhancement and how it helps to overcome the problem or limitation
Add a
freeing
signal toNode
s that will fill the gap. In addition to this specific use case, it would generally allow developers to stop leaning onis_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:
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.