fix(texture): use SaveTexture for remote exports - #272
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
📝 WalkthroughWalkthroughTexture export now separates local decoding from remote RenderDoc ChangesTexture export paths
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant tex_export
participant export_remote
participant SaveTexture
participant output_file
tex_export->>export_remote: pass resource, destination type, mip, and slice
export_remote->>SaveTexture: save texture
SaveTexture-->>export_remote: return result
export_remote->>output_file: verify generated PNG
output_file-->>tex_export: return path and size
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
🤖 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 `@src/rdc/handlers/texture.py`:
- Around line 125-128: Update _handle_tex_export, _handle_rt_export, and the
depth fallback in _handle_rt_depth to stop treating the return value of
controller.SaveTexture(...) as a success flag. Rely on exceptions and
temp_path.exists() to detect write failure, while preserving the existing
failure handling and the _save_result_ok helper for paths where its result is
meaningful.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91f63806-81f5-48fd-8268-0d807b63a843
📒 Files selected for processing (3)
src/rdc/handlers/_helpers.pysrc/rdc/handlers/texture.pytests/unit/test_tex_stats_handler.py
| def _save_result_ok(result: Any) -> bool: | ||
| """Accept current ResultDetails and older bool SaveTexture bindings.""" | ||
| ok = getattr(result, "OK", None) | ||
| return bool(ok()) if callable(ok) else bool(result) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
RenderDoc python ReplayController SaveTexture return type ResultDetails
💡 Result:
The SaveTexture method of the RenderDoc ReplayController class does not return a ResultDetails object; its return type is None [1]. The method is defined as: controller.SaveTexture(saveData, path) Where: - saveData is a renderdoc.TextureSave object that specifies the configuration for saving the texture (e.g., resource ID, file type, mapping options) [2][3]. - path is a string representing the destination file path on disk [1]. While other methods in the RenderDoc API, such as CaptureFile.OpenCapture, return a tuple containing a ResultDetails object (which is used to handle success or error reporting) [4][5], SaveTexture performs the operation and does not provide an explicit result object as a return value [1].
Citations:
- 1: https://renderdoc.org/docs/python_api/renderdoc/replay.html
- 2: https://renderdoc.org/docs/python_api/renderdoc/analysis.html
- 3: https://renderdoc.org/docs/python_api/examples/renderdoc/save_texture.html
- 4: https://renderdoc.org/docs/python_api/examples/renderdoc_intro.html
- 5: https://github.com/baldurk/renderdoc/blob/v1.x/docs/python_api/examples/renderdoc_intro.rst
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline src/rdc/handlers/texture.py --view expanded
# Read the relevant section with line numbers
sed -n '180,340p' src/rdc/handlers/texture.py | cat -nRepository: BANANASJIM/rdc-cli
Length of output: 8902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/rdc/handlers/texture.py --view expanded
printf '\n---\n'
sed -n '180,340p' src/rdc/handlers/texture.py | cat -nRepository: BANANASJIM/rdc-cli
Length of output: 8907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the specific handler functions and any save result helpers
rg -n "_save_result_ok|_save_result_message|SaveTexture|temp_path.exists|if not success" src/rdc/handlers/texture.py
printf '\n--- context ---\n'
sed -n '180,340p' src/rdc/handlers/texture.pyRepository: BANANASJIM/rdc-cli
Length of output: 8999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,170p' src/rdc/handlers/texture.py | cat -nRepository: BANANASJIM/rdc-cli
Length of output: 2466
Local SaveTexture paths always trip the failure check. In _handle_tex_export, _handle_rt_export, and the depth fallback in _handle_rt_depth, controller.SaveTexture(...) returns None in these bindings, so if not success or not temp_path.exists() reports SaveTexture failed even when the file was written. Drop the success truthiness check and rely on the file write/exception path.
🤖 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 `@src/rdc/handlers/texture.py` around lines 125 - 128, Update
_handle_tex_export, _handle_rt_export, and the depth fallback in
_handle_rt_depth to stop treating the return value of
controller.SaveTexture(...) as a success flag. Rely on exceptions and
temp_path.exists() to detect write failure, while preserving the existing
failure handling and the _save_result_ok helper for paths where its result is
meaningful.
2 participants
What
Use RenderDoc's
SaveTexturepath for remote texture and render-target exports.Why
Remote export previously fetched raw data with
GetTextureDataand decoded it in rdc-cli. That diverged from qrenderdoc/RenderDoc export behavior and could fail for compressed or HDR resources and Texture3D subresources. Follow-up to #268..How
SaveTexture, matching qrenderdoc.SaveTexturefailures and missing output files explicitly; retain raw decoding only for local depth export.Test plan
PYTHONUTF8=1 pixi run checkpasses (2,995 passed, 83 skipped)SaveTextureerror cases.rdccapture: exported ASTC Texture3D mip 0/slice 0 and mip 1/slice 1; both matched directSaveTextureoutput byte-for-byte (SHA-256)Summary by CodeRabbit
New Features
Bug Fixes