Update Texture2DRD docs for set_texture_rd_rid#1212
Update Texture2DRD docs for set_texture_rd_rid#1212AR-DEV-1 wants to merge 2 commits intoRedot-Engine:masterfrom
Texture2DRD docs for set_texture_rd_rid#1212Conversation
WalkthroughUpdated documentation for the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/classes/Texture2DRD.xml`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24184fca-3934-4084-9600-34509afd1cb9
📒 Files selected for processing (1)
doc/classes/Texture2DRD.xml
| 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]. |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
Coderabbit, in the Godot docs the same has been done (as the maintainers themselves have suggested this). I'll update it though if needed
There was a problem hiding this comment.
🧩 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 freestexture_rid(the internal RenderingServer wrapper), nevertexture_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: usesRS::texture_replaceon the internaltexture_ridwrapper — the previously storedtexture_rd_ridis 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.
There was a problem hiding this comment.
Hmm, okay, I'll see what I can do
There was a problem hiding this comment.
@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!
2 participants
TL;DR
Texture2DRDdocs forset_texture_rd_ridgodotengine/godot#116671Texture2DRD'sset_texture_rd_ridfunctionNote
Contributed by 2LazyDevs.
Summary by CodeRabbit