Error when changing the TextureRID of a Texture2DRD

[DevPoodle]

Tested versions

v4.2.stable.official [46dc27791]

System information

Godot v4.2.stable - Vulkan (Forward+)

Issue description

If you free an RID associated with a Texture2DRD and then add a new RID to the texture, it outputs an error, saying it attempted to free an invalid RID.

Steps to reproduce

This code outputs an error on the last line. In my actual project, I needed to do this because you can't change the resolution of a texture without creating a new one.

var rd := RenderingServer.get_rendering_device()
var texture_rd := Texture2DRD.new()
var rid : RID

var fmt := RDTextureFormat.new()
fmt.width = 8
fmt.height = 8
fmt.format = RenderingDevice.DATA_FORMAT_R32G32B32A32_SFLOAT
fmt.usage_bits = RenderingDevice.TEXTURE_USAGE_SAMPLING_BIT + RenderingDevice.TEXTURE_USAGE_CAN_UPDATE_BIT

var image := Image.create(8, 8, false, Image.FORMAT_RGBAF)
image.fill(Color.BLACK)

rid = rd.texture_create(fmt, RDTextureView.new(), [image.get_data()])
texture_rd.texture_rd_rid = rid

rd.free_rid(rid)
rid = rd.texture_create(fmt, RDTextureView.new(), [image.get_data()])
texture_rd.texture_rd_rid = rid

There is a workaround for this, which is deleting the Texture2DRD and creating a new one. But that doesn't seem like the intended way of changing the RID of a texture.

Minimal reproduction project (MRP)

N/A

[AThousandShips]

In this case I'd say you shouldn't handle the RID tracking and freeing manually, that's the whole point of using this type, but maybe the documentation should be improved to clarify how it operates

[DevPoodle]

The only example of how to use Texture2DRD that I could find was the new compute texture demo in godot-demo-projects. In that project, the RIDs are tracked and freed manually, so if that's not the intended use then the demo should also be updated to clarify.

[AThousandShips]

That code is different from yours though, it clears the RIDs manually and updates between runs, unlike yours

Does it work with:

rid = rd.texture_create(fmt, RDTextureView.new(), [image.get_data()])
texture_rd.texture_rd_rid = rid

texture_rd.texture_rd_rid = RID()
rd.free_rid(rid)
rid = rd.texture_create(fmt, RDTextureView.new(), [image.get_data()])
texture_rd.texture_rd_rid = rid

[DevPoodle]

Thanks, that seems to be the solution I was looking for. Having to create an empty RID seems a bit unintuitive, but it's definitely better than the workaround I was using before. This should probably be clarified in the Texture2DRD class reference.

[AThousandShips]

Well you are removing the control from the resource, you leave it as thinking it's in control, if you untrack it you signal it's no longer in charge

[clayjohn]

Seems like the error is coming from this line: https://github.com/godotengine/godot/blob/15a03ed98e63590c85dac62405d785e1b7ca7fed/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp#L1375

Which is called by RenderingServer.texture_replace(), which is called by Texture2DRD.set_texture_rd_rid().

When assinging RID() we don't call texture_replace() and so we only free the RenderingServer copy of the texture.

I don't think we should be using texture_replace() for RD textures so we should switch to something else (i.e. free the RenderingServer texture and replace it). This would work the same as the workaround above (i.e. texture_rd.texture_rd_rid = RID()).

Alternatively, we can mark textures as TextureRDs in the RenderingServer. And then not free the internal RD texture within texture_replace().

What do you think @BastiaanOlij?

[DDarby-Lewis]

I have found this issue also while using Texture2DRD. My note to add here is that Texture2DRD doesn't free the RenderingDevice RID for the texture when swapping it out for a new RID, e.g.

texture_rd.texture_rd_rid = new_rid
```gdscript
The proof for this is in the fact that my GPU memory climbs up, and when exiting Godot I get messages about unfreed RenderingDevice Texture RIDs.

My solution is to then free the RID myself with `rd.free_rid(rid)` - which results in the same error as seen in the original post.

servers\rendering\rendering_device.cpp:4956 - Attempted to free invalid ID: 726777185969070

I am not 100% certain what is occurring, but I do know that the RID has to be freed manually to avoid memory leaks, and that the RID in the error message is not the RID of the actual RendingDevice Texture since I have printed out all the RIDs I am creating on the RenderingDevice and compared them.

I suspect the invalid RID is a dependency of some kind, but haven't been able to see for sure.

I also cannot assign the `RID()` then assign my texture since assigning `RID()` invalidates the texture resource in such a way that assigning the new RID then requires the actual resource to be reloaded on whatever is using it.
e.g. doing:
```gdscript
        texture_rd_rid = RID()
        texture_rd_rid = new_rid

results in the material using this texture suddenly going blank (which is from the first of these lines) and then the texture on the new RID doesn't shows until the texture is unassigned from the material and reassigned. This is perhaps a separate but related bug in the way assigning RID() happens - already linked above.

As such for now I am stuck with allowing the Attempted to free invalid ID errors to keep occurring as it is the lesser of two evils.

I think for my purpose if the Texture2DRD resource simply didn't try and do any freeing of RendingDevice resources that'd be sufficient since then I could use this and handle the freeing myself. Alternatively, if it handled it properly such that everything was freed I could simply not worry about it.

[DDarby-Lewis]

See discussion here: https://github.com/godotengine/godot/issues/95273#issuecomment-2677077376

Also I left some related comments on the MR here.

TLDR is that although the changes in the MR seem to perhaps be headed in the right direction they don't address thing completely as they leave unfreed textures on exit.