Fix Vulkan garbage data in frame (and grammar update =D) - #1316
Merged
JoltedJon merged 1 commit intoAug 2, 2026
Merged
Conversation
WalkthroughThe project manager backup prompt now uses “Back up project first” across localized message keys. Newly allocated RD render-target textures are cleared to transparent black and support copy-to usage. ChangesBackup prompt wording
Render-target initialization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (2)
servers/rendering/renderer_rd/storage_rd/texture_storage.h (1)
465-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the grammatical inconsistency in the documentation comment.
Consider updating the comment to use consistent verb forms (e.g., "Allocates" to match "wires").
📝 Proposed fix
- /// Allocate GPU image (RenderTarget::color) and wires it to RenderTarget::texture, + /// Allocates GPU image (RenderTarget::color) and wires it to RenderTarget::texture,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/renderer_rd/storage_rd/texture_storage.h` around lines 465 - 466, Update the documentation comment above the RenderTarget GPU image allocation declaration to use consistent third-person verb forms, changing “Allocate” to “Allocates” while preserving the existing description.servers/rendering/renderer_viewport.cpp (1)
933-937: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out debug code.
Commented-out debugging logs should be removed to keep the production codebase clean. If these diagnostics are still needed for future debugging, consider placing them behind a feature flag or a disabled-by-default macro instead of commenting them out.
♻️ Proposed fix
- // print_verbose(vformat("[DRAW] rt=%d used_by_vp=%d was_used=%s", - // vp->render_target.get_id(), - // vp->used_by_viewport.get_id(), - // RSG::texture_storage->render_target_was_used(vp->render_target) ? "Y" : "N"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/renderer_viewport.cpp` around lines 933 - 937, Remove the commented-out print_verbose debug block near the viewport rendering code; do not replace it with another disabled comment, and leave the surrounding rendering logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/translations/editor/fr.po`:
- Around line 9120-9121: Update the French msgstr for “Back up project first” to
use a fully localized, natural French translation such as “Sauvegarder d'abord
le projet” or “Sauvegardez d'abord le projet,” replacing the mixed English
wording.
---
Nitpick comments:
In `@servers/rendering/renderer_rd/storage_rd/texture_storage.h`:
- Around line 465-466: Update the documentation comment above the RenderTarget
GPU image allocation declaration to use consistent third-person verb forms,
changing “Allocate” to “Allocates” while preserving the existing description.
In `@servers/rendering/renderer_viewport.cpp`:
- Around line 933-937: Remove the commented-out print_verbose debug block near
the viewport rendering code; do not replace it with another disabled comment,
and leave the surrounding rendering logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0adefa3-c20b-4865-a230-82328d96eaf0
📒 Files selected for processing (21)
editor/project_manager/project_manager.cppeditor/translations/editor/de.poeditor/translations/editor/es.poeditor/translations/editor/fa.poeditor/translations/editor/fr.poeditor/translations/editor/ga.poeditor/translations/editor/it.poeditor/translations/editor/ja.poeditor/translations/editor/ko.poeditor/translations/editor/pl.poeditor/translations/editor/pt_BR.poeditor/translations/editor/ru.poeditor/translations/editor/sv.poeditor/translations/editor/ta.poeditor/translations/editor/tr.poeditor/translations/editor/uk.poeditor/translations/editor/zh_Hans.poeditor/translations/editor/zh_Hant.poservers/rendering/renderer_rd/storage_rd/texture_storage.cppservers/rendering/renderer_rd/storage_rd/texture_storage.hservers/rendering/renderer_viewport.cpp
Comment on lines
+9120
to
9121
| msgid "Back up project first" | ||
| msgstr "Backup le projet d'abord" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the French translation.
msgstr still says “Backup le projet d'abord,” which mixes English and French and uses an unnatural verb form. Use a fully localized label such as Sauvegarder d'abord le projet or Sauvegardez d'abord le projet.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/translations/editor/fr.po` around lines 9120 - 9121, Update the French
msgstr for “Back up project first” to use a fully localized, natural French
translation such as “Sauvegarder d'abord le projet” or “Sauvegardez d'abord le
projet,” replacing the mixed English wording.
JoltedJon
reviewed
Aug 1, 2026
GeneralProtectionFault
force-pushed
the
viewport_texture_fix
branch
from
August 1, 2026 01:02
c7e0cd1 to
4c223e1
Compare
GeneralProtectionFault
force-pushed
the
viewport_texture_fix
branch
from
August 1, 2026 01:04
4c223e1 to
fc3bc59
Compare
GeneralProtectionFault
force-pushed
the
viewport_texture_fix
branch
from
August 1, 2026 02:48
fc3bc59 to
03d85a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@servers/rendering/renderer_rd/storage_rd/texture_storage.cpp`:
- Line 3386: Update the new texture_clear calls in the render-target setup and
related paths around rt->color and rt->color_multisample (including the
locations near 4336-4339) to run only when the backend reports
API_TRAIT_TEXTURE_OUTPUTS_REQUIRE_CLEARS. Add CAN_COPY_TO_BIT only for the
explicit-usage path, unless an existing in-code consumer requires it, while
preserving the current clear behavior on backends that need output clears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 258a6ec4-48df-430f-a152-107fb047c26a
📒 Files selected for processing (20)
editor/project_manager/project_manager.cppeditor/translations/editor/de.poeditor/translations/editor/es.poeditor/translations/editor/fa.poeditor/translations/editor/fr.poeditor/translations/editor/ga.poeditor/translations/editor/it.poeditor/translations/editor/ja.poeditor/translations/editor/ko.poeditor/translations/editor/pl.poeditor/translations/editor/pt_BR.poeditor/translations/editor/ru.poeditor/translations/editor/sv.poeditor/translations/editor/ta.poeditor/translations/editor/tr.poeditor/translations/editor/uk.poeditor/translations/editor/zh_Hans.poeditor/translations/editor/zh_Hant.poservers/rendering/renderer_rd/storage_rd/texture_storage.cppservers/rendering/renderer_rd/storage_rd/texture_storage.h
🚧 Files skipped from review as they are similar to previous changes (19)
- servers/rendering/renderer_rd/storage_rd/texture_storage.h
- editor/project_manager/project_manager.cpp
- editor/translations/editor/ja.po
- editor/translations/editor/fr.po
- editor/translations/editor/de.po
- editor/translations/editor/it.po
- editor/translations/editor/es.po
- editor/translations/editor/zh_Hans.po
- editor/translations/editor/ga.po
- editor/translations/editor/pl.po
- editor/translations/editor/tr.po
- editor/translations/editor/pt_BR.po
- editor/translations/editor/ko.po
- editor/translations/editor/sv.po
- editor/translations/editor/uk.po
- editor/translations/editor/ru.po
- editor/translations/editor/fa.po
- editor/translations/editor/zh_Hant.po
- editor/translations/editor/ta.po
| // explicit clear here, a consumer (e.g. Sprite3D) can sample this texture before | ||
| // any draw ever writes to it (e.g. SubViewport with VIEWPORT_UPDATE_WHEN_VISIBLE, | ||
| // whose was_used starts false), reading uninitialized VRAM garbage for one frame. | ||
| RD::get_singleton()->texture_clear(rt->color, Color(0, 0, 0, 0), 0, 1, 0, rd_color_attachment_format.array_layers); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 10 \
--glob '*.cpp' \
--glob '*.h' \
'API_TRAIT_TEXTURE_OUTPUTS_REQUIRE_CLEARS|texture_clear\(|_texture_check_pending_clear|render_target_get_color_usage_bits|CAN_COPY_TO_BIT'Repository: Redot-Engine/redot-engine
Length of output: 163
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Repo files matching texture_storage.cpp:"
fd 'texture_storage\.cpp$' . || true
echo
echo "Targeted symbol search across repo (text):"
rg -n -C 8 \
'API_TRAIT_TEXTURE_OUTPUTS_REQUIRE_CLEARS|texture_clear\(|_texture_check_pending_clear|render_target_get_color_usage_bits|CAN_COPY_TO_BIT|pending_clear' . || true
echo
echo "Diff stat:"
git diff --stat || true
echo
echo "Relevant diff hits:"
git diff --unified=8 -- servers/rendering/renderer_rd/storage_rd/texture_storage.cpp 2>/dev/null || trueRepository: Redot-Engine/redot-engine
Length of output: 50382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Line count and relevant ranges:"
wc -l servers/rendering/renderer_rd/storage_rd/texture_storage.cpp servers/rendering/rendering_device.cpp servers/rendering/rendering_device_driver_d3d12.cpp || true
echo
echo "texture_storage.cpp around changed ranges:"
sed -n '3360,3415p' servers/rendering/renderer_rd/storage_rd/texture_storage.cpp
sed -n '4320,4350p' servers/rendering/renderer_rd/storage_rd/texture_storage.cpp
echo
echo "rendering_device.h API_TRAIT declarations/classes:"
sed -n '700,810p' servers/rendering/rendering_device_driver.h
echo
echo "rendering_device.cpp resource_trait_get and texture_clear implementations:"
rg -n -C 12 'api_trait_get|texture_clear|_texture_check_pending_clear|pending_clear|TextureFormatID' servers/rendering/rendering_device.cpp
echo
echo "D3D12 trait_get implementation:"
sed -n '5745,5790p' drivers/d3d12/rendering_device_driver_d3d12.cpp
echo
echo "Other non-doc/translation CAN_COPY_TO_BIT code usages:"
rg -n --glob '!doc/**' --glob '!drivers/sdl2/**' \
'TEXTURE_USAGE_CAN_COPY_TO_BIT|CAN_COPY_TO_BIT' servers modules drivers scripts plugins | head -n 250Repository: Redot-Engine/redot-engine
Length of output: 50381
Make the new clear path backend-conditional.
RenderingDevice::texture_clear() checks pending clears first, then queues its own implicit clear. D3D12 returns true for API_TRAIT_TEXTURE_OUTPUTS_REQUIRE_CLEARS, so the new texture_clear(rt->color, ...) and texture_clear(rt->color_multisample, ...) add a second full clear after the pending clear. Gate these calls on backends needing API_TRAIT_TEXTURE_OUTPUTS_REQUIRE_CLEARS, and add CAN_COPY_TO_BIT only for the explicit-usage path unless another in-code consumer requires it.
Also applies to: 3404, 4336-4339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/rendering/renderer_rd/storage_rd/texture_storage.cpp` at line 3386,
Update the new texture_clear calls in the render-target setup and related paths
around rt->color and rt->color_multisample (including the locations near
4336-4339) to run only when the backend reports
API_TRAIT_TEXTURE_OUTPUTS_REQUIRE_CLEARS. Add CAN_COPY_TO_BIT only for the
explicit-usage path, unless an existing in-code consumer requires it, while
preserving the current clear behavior on backends that need output clears.
JoltedJon
approved these changes
Aug 2, 2026
JoltedJon
enabled auto-merge (rebase)
August 2, 2026 02:43
Shakai-Dev
approved these changes
Aug 2, 2026
Shakai-Dev
left a comment
Shakai-Dev
left a comment
Contributor
There was a problem hiding this comment.
LGTM from the rendering side of things
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #1308
This partially addresses this issue.
The crux is, a (Vulkan) frame is not initialized in the same frame as a scene/subviewport being added to the tree. This might also overlap the previous fix #1303 , which did fix the "hope-based" sibling subviewport ordering. However, immediately when added to the tree, there's a potential for more than 1 sibling subviewport to have no dependencies.
The above basically results in rendering garbage data from the GPU in some cases. This PR just blanks that out so we don't get bugged visuals in the subviewport. Before we could actually see visual artifacts/textures , but this makes it a blank/transparent frame.
If we change the example project so the timout on the background_3d scene is 1.0 instead of 0.1, then we see the blank frame even after this PR, but no garbage data.
It looks like it's more than 1 frame in some cases, so this might be good to note for posterity, if we have non-deterministic behavior in the Vulkan layer, but this should alleviate the problem in the majority of cases.
Anyhoo, also changed "Backup" to "Back up" for the migration pop-up.
Summary by CodeRabbit
Bug Fixes
Documentation