Add a _predelete() virtual method for Objects

MajorMcDoom commented 4 months ago

Describe the project you are working on

An indie VR game.

Describe the problem or limitation you are having in your project

Many of my scripts have to do memory cleanup for member Objects that are created via new(). There is currently nowhere to do this that makes sense except in _notification() with a check for p_what == NOTIFICATION_PREDELETE. While this achieves the functionality I want, it leads to messy code. Whereas I can do all my initialization code in one method like _init() or _ready(), my cleanup code not only has an extra indent, but is also grouped together with other code that responds to other notifications. If I want to maintain functional segregation, I have to make my own _cleanup() method and call that from inside the if statment insde of _notification().

This is not the first proposal for this feature, and past PRs have been closed based on the argument that it's enough to just implement this in _notification(), but as mentioned above, this is not a satisfactory solution because it creates very messy code for a very simple and likely common use case.

_exit_tree() has also been brought up multiple times in the past as a solution, but that does not meet the functional specifications required - it's a convenient workaround for users who only have nodes that are added and removed from the world one time. Furthermore, it does not work for non-Node objects or Node objects that were instantiated but not added to the tree. Improperly using _exit_tree() to free up memory leads to memory leaks.

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

Having a symmetrical _predelete() virtual method for objects would alleviate the issue.

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

Give Objects a _predelete() virtual method similar to _ready() or _enter_tree().

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

I believe it will be used often if people knew it existed. See below.

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

I believe it will be used often if people knew it existed. Many users are likely misusing _exit_tree(), or are simply not freeing up their allocated (non-ref-counted) members, or are just using the _notification() workaround because they have to. It would not be a stretch to say that the lack of this method encourages and proliferates bad memory management practices, for the simple fact that Godot requires some things to be explicitly freed, yet does not provide a dedicated place to do this.

Mickeon commented 4 months ago

There's two withstanding issues with NOTIFICATION_PREDELETE that I feel like should be addressed before exposing it even more with a corresponding virtual method:

MajorMcDoom commented 4 months ago

There's two withstanding issues with NOTIFICATION_PREDELETE that I feel like should be addressed before exposing it even more with a corresponding virtual method:
* [Custom class instance extending Reference already freed when it receives NOTIFICATION_PREDELETE godot#6784](https://github.com/godotengine/godot/issues/6784)

* [Calling a method of your own Reference script within PREDELETE notification fails godot#31166](https://github.com/godotengine/godot/issues/31166)

Is there perhaps a better candidate for where to call this virtual method? For example _init doesn't correspond to any notification. Since this is meant to be the opposite of that, could we also make a virtual method that corresponds to destruction without corresponding to a notification?

EDIT: Seems like this would have the same issue. 😔

godotengine / godot-proposals