Make Rendering Device ref counted

[clayjohn]

Alternative to: #109211

Can't be merged for 4.5.

This breaks compatibility even more than #109211 does, but this may be an important change for scripting users.

Right now there is no way for a user to free a local rendering device created with create_local_rendering_device(). All the user receives is a pointer and there is no corresponding method to free it.

This PR makes RenderingDevice inherit from RefCounted so that it can be automatically freed once there are no more references to it. This has the benefit of fixing the memory leak in #109193 (although allocating the RD in the first place is still a bug IMO).

This PR could be combined with #109211 to both fix #109193 and ensure that users don't leak RenderingDevices in their own code.

[clayjohn]

Marking as draft as I am unsure about the approach. I feel like it might be preferable to make a wrapper around RenderingDevice for exposing to scripting languages that is RefCounted. Doing so has the added benefit that we could still fallback to the old methods for GDExtension compatibility

[dsnopek]

Right now there is no way for a user to free a local rendering device created with create_local_rendering_device(). All the user receives is a pointer and there is no corresponding method to free it.

Can't they do rd.free() in GDScript or memfree(rd) in GDExtension? Or, does freeing a rendering device cause other problems?

Doing so has the added benefit that we could still fallback to the old methods for GDExtension compatibility

I'm not sure what you mean by this? This PR is definitely trickier for GDExtension compatibility than PR #109211 is - that PR only needs a compatibility method added.

Inserting a new parent class into the hierarchy is actually fine as far as GDExtension compatibility. The problem I envision here (although, I haven't done any testing) is that memory is managed differently between objects that descend from RefCounted and those that don't.

We could maybe do something sneaky, like have the compatibility method for create_local_rendering_device() manually increment the reference count by one so that it'll never get cleaned up automatically, and depend on the GDExtension using memfree() to clean it up? That could maybe work so long as nothing on the Godot side holds a long-term reference to the object.

[clayjohn]

Right now there is no way for a user to free a local rendering device created with create_local_rendering_device(). All the user receives is a pointer and there is no corresponding method to free it.

Can't they do rd.free() in GDScript or memfree(rd) in GDExtension? Or, does freeing a rendering device cause other problems?

In GDExtension they can indeed call memfree(rd). In GDScript, I don't think so. With respect to GDScript, I'm not sure. I didn't realize that object had a free() method that could apply here. I'll look into it!

Doing so has the added benefit that we could still fallback to the old methods for GDExtension compatibility

I'm not sure what you mean by this? This PR is definitely trickier for GDExtension compatibility than PR #109211 is - that PR only needs a compatibility method added.

Inserting a new parent class into the hierarchy is actually fine as far as GDExtension compatibility. The problem I envision here (although, I haven't done any testing) is that memory is managed differently between objects that descend from RefCounted and those that don't.

We could maybe do something sneaky, like have the compatibility method for create_local_rendering_device() manually increment the reference count by one so that it'll never get cleaned up automatically, and depend on the GDExtension using memfree() to clean it up? That could maybe work so long as nothing on the Godot side holds a long-term reference to the object.

Oh, I just meant that it would be easier to have two methods that coexist than to have a compat method and a new method.

At any rate, if we have a tool in GDScript to free the RD, then this is not necessary. At that point it becomes purely a documentation issue

[clayjohn]

Alright, confirmed that rd.free() does work and properly frees all the resources created by the local rendering device.

So this was a false alarm and this PR is indeed not needed after all!

I feel a bit silly now lol.

Thank you David!