Give Resources/RefCounteds a proper way to react to NOTIFICATION_PREDELETE

[vbettaque]

Describe the project you are working on

A volumetric sky renderer using ray marching. An efficient way to implement this is by computing both the sky and the clouds in compute shaders that output to one or multiple look-up textures (represented by TextureRD2Ds), which are then combined and sent to a sky shader for rendering. This is necessary because Godot's sky shader does not allow for certain performance tricks like staggered rendering using interpolation. An example on how it can be done is found here.

Describe the problem or limitation you are having in your project

Godot's philosophy of composition implies that abstract Nodes and Resources should have as little cross-dependence as possible. It is therefore reasonable to have all the code relevant for setting up and running a compute shader be local to the TextureRD (e.g. TextureRD2D) that it is supposed to output to.

However, working with compute shaders requires that the user correctly frees all the relevant RIDs at the end of the TextureRD's lifetime to avoid memory leaks. But while Resources can initialize these RIDs through RenderingDevice at the beginning (e.g. using _ready() ), they can't free them at the end because listening to NOTIFICATION_PREDELETE does not work for any objects inheriting from RefCounted. While this behavior was initially reported as a bug, judging from the comments by @reduz et al. it seemed to be intentional or at least WONTFIX at the time.

But a lot of time has passed since that report and TextureRD is now a thing, which means that any such Resource has to be permanently associated with a Node object that manually frees the RIDs on its behalf. This ultimately unnecessary cross-dependence does not only contradict Godot's aforementioned philosophy, but it is also highly error-prone, especially since Node-Resource relationships can often change during refactoring.

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

The aforementioned "bug" should be fixed, and the usage of NOTIFICATION_PREDELETE to define a destructor should be better documented (especially in the context of TextureRD). This would solve the aforementioned issues and inform people new to using TextureRDs about the practice.

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

N/A

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

Yes, but doing so would violate Godot's core principles and could easily lead to memory leaks in large codebases.

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

The proposal depends on fixing a "bug" in core.

Addendum: It should be noted that one can work around this "bug", e.g. by not invoking any functions defined in the custom Resource/RefCounted (accessing user-defined variables is fine though). However this is undefined behavior and not guaranteed to be possible indefinitely.

[ydeltastar]

However, working with compute shaders requires that the user correctly frees all the relevant RIDs at the end of the TextureRD's lifetime to avoid memory leaks.

Are you sure this is needed? These high-level types like Texture2DRD are intended to wrap and automatically manage server resources. It should handle initialization and cleanup on its own; otherwise there is no point in using it instead of a server API like RenderingDevice.texture_create(). Looking at Texture2DRD's source, it frees its RenderingServer texture which should also free the actual RenderingDevice texture. If it is not the case, it is a memory leak bug that should be addressed apart from the proposal.

[theraot]

If we had a higher level API for compute shaders that would automatically manage resources, then I'd say this is not needed. Given the precedents, that is likely how this would be resolved.

I've also wished for some kind of notification that I could use after an object has been freed to release associated resources. Now thinking about it... Perhaps we can have a RefCounted monitor another: If we add a RefCounted in the metadata of another, and that is the only reference to it, and it has an script listening for NOTIFICATION_PREDELETE, that could be a way to react to the deletion.

I know this is not about Nodes (which are not RefCounted) but this proposal reminds me of these:

• Emit a freeing signal when queue_free is called on a Node #7441
• Really need Node destroy callback function, just like "OnDestroy" in Unity #707

Addendum: I have posted a proof of concept at #7441 (comment) - I made using a Node, but it shouldn't be hard to adapt.

[vbettaque]

However, working with compute shaders requires that the user correctly frees all the relevant RIDs at the end of the TextureRD's lifetime to avoid memory leaks.

Are you sure this is needed? These high-level types like Texture2DRD are intended to wrap and automatically manage server resources. It should handle initialization and cleanup on its own; otherwise there is no point in using it instead of a server API like RenderingDevice.texture_create(). Looking at Texture2DRD's source, it frees its RenderingServer texture which should also free the actual RenderingDevice texture. If it is not the case, it is a memory leak bug that should be addressed apart from the proposal.

It does seem to free the texture itself, but it does not affect e.g. the shader and pipeline instances that are needed to run the compute process (although I think the latter gets freed when the former is). I don't think Godot has a way of detecting those, so you would have to take care of them manually.