Skip to content

Update Texture2DRD docs for set_texture_rd_rid #1212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Shakai-Dev wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update Texture2DRD docs for set_texture_rd_rid #1212

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6488f4
Update `Texture2DRD` docs for `set_texture_rd_rid`
Shakai-Dev Mar 4, 2026
70fed16
Merge branch 'master' into t2drd-doc
Shakai-Dev Mar 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/classes/Texture2DRD.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
<members>
<member name="resource_local_to_scene" type="bool" setter="set_local_to_scene" getter="is_local_to_scene" overrides="Resource" default="false" />
<member name="texture_rd_rid" type="RID" setter="set_texture_rd_rid" getter="get_texture_rd_rid">
The RID of the texture object created on the [RenderingDevice].
The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] takes ownership of the provided RID. This means the engine will handle freeing the RID automatically when the texture is no longer in use or when the RID is replaced.
[b]Note:[/b] Do not manually free the RID using [method RenderingDevice.free_rid].
Comment on lines +14 to +15

@coderabbitai coderabbitai Bot Mar 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Ownership statement is incorrect and can cause RID leaks

Line 14 and Line 15 currently state that Texture2DRD owns and auto-frees the provided RD RID, but implementation only frees the internal texture_rid wrapper (scene/resources/texture_rd.cpp:75-89, scene/resources/texture_rd.cpp:122-127). This doc text reverses ownership expectations and can lead users to skip freeing their RD RID.

Proposed doc fix 
-			The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] takes ownership of the provided RID. This means the engine will handle freeing the RID automatically when the texture is no longer in use or when the RID is replaced.
-			[b]Note:[/b] Do not manually free the RID using [method RenderingDevice.free_rid].
+			The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] references the provided RID to build an internal rendering-server texture.
+			[b]Note:[/b] [Texture2DRD] does not free this [RenderingDevice] RID. Manage its lifetime yourself and free it with [method RenderingDevice.free_rid] when it is no longer needed.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] takes ownership of the provided RID. This means the engine will handle freeing the RID automatically when the texture is no longer in use or when the RID is replaced.
[b]Note:[/b] Do not manually free the RID using [method RenderingDevice.free_rid].
The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] references the provided RID to build an internal rendering-server texture.
[b]Note:[/b] [Texture2DRD] does not free this [RenderingDevice] RID. Manage its lifetime yourself and free it with [method RenderingDevice.free_rid] when it is no longer needed.
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@doc/classes/Texture2DRD.xml` around lines 14 - 15, The documentation
incorrectly states that Texture2DRD takes ownership of the provided
RenderingDevice RID and frees it automatically; update the doc text to clarify
that Texture2DRD does NOT take ownership of the caller-supplied RD RID and users
must free their RD RID via RenderingDevice.free_rid if they created it, while
Texture2DRD only manages and frees its internal wrapper (see texture_rid in
Texture2DRD and the freeing behavior in scene/resources/texture_rd.cpp). Locate
the doc block referencing Texture2DRD and RenderingDevice.free_rid and change
the ownership wording to explicitly state that the engine only frees the
internal texture_rid wrapper and that callers remain responsible for the
original RD RID. Ensure the note warns users not to assume automatic freeing of
their RD RID by Texture2DRD and references the internal texture_rid behavior for
clarity.

@Shakai-Dev Shakai-Dev Mar 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coderabbit, in the Godot docs the same has been done (as the maintainers themselves have suggested this). I'll update it though if needed

@coderabbitai coderabbitai Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current Godot master Texture2DRD.xml doc
curl -s "https://raw.githubusercontent.com/godotengine/godot/master/doc/classes/Texture2DRD.xml"

Length of output: 1082

🏁 Script executed:

#!/bin/bash
# Check Godot PR `#116671` status and comments
curl -s "https://api.github.com/repos/godotengine/godot/pulls/116671" | jq '{state, title, merged_at, body}'
echo "---REVIEWS---"
curl -s "https://api.github.com/repos/godotengine/godot/pulls/116671/reviews" | jq '[.[] | {user: .user.login, state, body}]'
echo "---REVIEW COMMENTS---"
curl -s "https://api.github.com/repos/godotengine/godot/pulls/116671/comments" | jq '[.[] | {user: .user.login, body, path}]'

Length of output: 624

🏁 Script executed:

#!/bin/bash
# Check Texture2DRD implementation in Redot
fd "texture_rd.cpp" --type f

Length of output: 30

🏁 Script executed:

#!/bin/bash
cat scene/resources/texture_rd.cpp

Length of output: 11544

@AR-DEV-1, I've now checked both the Godot PR and the actual implementation. My concern still stands — here's the evidence:

Godot PR #116671 is still open and unmerged. The current Godot master doc for Texture2DRD still has only the bare original line. The only maintainer comment there (from AThousandShips) was a formatting suggestion for the [b]Note:[/b] line — it didn't validate the ownership claim at all.

The implementation confirms Texture2DRD does NOT take ownership of the caller-supplied RD RID. Looking at the code:

  • Destructor (~Texture2DRD()): only frees texture_rid (the internal RenderingServer wrapper), never texture_rd_rid:

    Texture2DRD::~Texture2DRD() {
    if (texture_rid.is_valid()) {
        RS::get_singleton()->free(texture_rid);  // RS wrapper only
        texture_rid = RID();
    }
    // texture_rd_rid is never freed here
}

  • _set_texture_rd_rid() on replacement: uses RS::texture_replace on the internal texture_rid wrapper — the previously stored texture_rd_rid is silently overwritten without being freed:

    texture_rd_rid = p_texture_rd_rid;  // old RD RID just discarded, not freed
if (texture_rid.is_valid()) {
    RS::get_singleton()->texture_replace(texture_rid, RS::get_singleton()->texture_rd_create(p_texture_rd_rid));
}

The documentation as written tells users not to call free_rid, which would cause a RD RID leak every time a Texture2DRD is destroyed or its RID is replaced. The suggested fix from my original comment is the accurate description of the actual behavior:

-			The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] takes ownership of the provided RID. This means the engine will handle freeing the RID automatically when the texture is no longer in use or when the RID is replaced.
-			[b]Note:[/b] Do not manually free the RID using [method RenderingDevice.free_rid].
+			The RID of the texture object created on the [RenderingDevice]. [Texture2DRD] references the provided RID to build an internal rendering-server texture.
+			[b]Note:[/b] [Texture2DRD] does not free this [RenderingDevice] RID. Manage its lifetime yourself and free it with [method RenderingDevice.free_rid] when it is no longer needed.

It may be worth also flagging this on the upstream Godot PR (#116671), since the same incorrect documentation would land there too if merged as-is.

@Shakai-Dev Shakai-Dev Mar 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, okay, I'll see what I can do

@coderabbitai coderabbitai Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AR-DEV-1, sounds good! When you're ready, the suggested diff from my previous comment should cover it. If it would help, you could also leave a note on the upstream Godot PR #116671 pointing to the implementation — that way the same correction can land there before it's merged. Happy to help with anything else in the meantime!

</member>
</members>
</class>
Loading