fix(texture): export requested 3d texture slice - #271
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughChangesTexture export now supports array and 3D depth slices through the CLI, VFS routes, local saves, and remote PNG decoding. VFS skeletons expose slice nodes, with validation and regression tests covering default, explicit, and invalid slices. Texture slice export
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant VFS
participant tex_export
participant RenderDoc
participant PNGDecoder
CLI->>VFS: request texture mip slice path
VFS->>tex_export: resolve id, mip, and slice
tex_export->>RenderDoc: fetch or save requested subresource
RenderDoc-->>tex_export: return texture data
tex_export->>PNGDecoder: decode selected 3D depth slice
PNGDecoder-->>CLI: return PNG output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 (1)
tests/unit/test_tex_stats_handler.py (1)
1048-1083: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand test coverage to include the
slice=0edge case.This test verifies that
slice=1is passed down toSaveTexture, but it bypasses the falsyslice=0edge case. Adding a check forslice=0will guard against regressions where the default slice inadvertently falls back to-1(exporting a grid).♻️ Proposed refactor
state = make_daemon_state( ctrl=ctrl, current_eid=100, rd=rd, tmp_path=tmp_path, tex_map={172: tex}, is_remote=False, ) resp, _ = _handle_request(rpc_request("tex_export", {"id": 172, "slice": 1}), state) assert "result" in resp - assert slice_indices == [1] + + # Test explicit default (slice=0) + resp0, _ = _handle_request(rpc_request("tex_export", {"id": 172, "slice": 0}), state) + assert "result" in resp0 + + assert slice_indices == [1, 0]🤖 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 `@tests/unit/test_tex_stats_handler.py` around lines 1048 - 1083, Extend test_tex_export_local_texture3d_sets_requested_slice to issue a tex_export request with slice=0 and verify SaveTexture receives sliceIndex 0, preserving the existing slice=1 assertion and setup so the falsy value cannot fall back to -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/_helpers.py`:
- Around line 238-247: Update _make_texsave to assign ts.slice.sliceIndex from
array_slice unconditionally, removing the truthiness guard so an explicit slice
value of 0 is preserved. Keep the existing TextureSliceMapping creation and
other export settings unchanged.
---
Nitpick comments:
In `@tests/unit/test_tex_stats_handler.py`:
- Around line 1048-1083: Extend
test_tex_export_local_texture3d_sets_requested_slice to issue a tex_export
request with slice=0 and verify SaveTexture receives sliceIndex 0, preserving
the existing slice=1 assertion and setup so the falsy value cannot fall back to
-1.
🪄 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: 7cbb2716-2e7c-47b3-81d7-fd85bf3658b3
📒 Files selected for processing (10)
src/rdc/_skills/references/commands-quick-ref.mdsrc/rdc/commands/export.pysrc/rdc/handlers/_helpers.pysrc/rdc/handlers/texture.pysrc/rdc/vfs/router.pysrc/rdc/vfs/tree_cache.pytests/unit/test_export_commands.pytests/unit/test_tex_stats_handler.pytests/unit/test_vfs_router.pytests/unit/test_vfs_tree_cache.py
05ae109 to
fdeb5b1
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
fdeb5b1 to
fb0190c
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Thanks for the update. I re-reviewed the current head ( There are still a few blockers before merge:
All six Linux/Windows test-matrix jobs are passing. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Thanks — addressed the three blockers in follow-up commit
Validation:
I will also validate the ASTC Texture3D path against a real Android remote replay capture when I have access to the device again. The current regression coverage exercises the ReplayController fallback path with the requested mip and slice. |
BANANASJIM
left a comment
BANANASJIM
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (753ec69). The three previous blockers are resolved: remote block-compressed Texture3D export now uses the client-side ReplayController SaveTexture path with the requested mip/slice and tested failure handling; the missing VFS mip/slices intermediate nodes are present; and both generated command references are in sync.
Verification: 285 targeted unit tests passed, Ruff and mypy passed, both generator checks passed, and all required GitHub CI checks are green. An independent reviewer also verified the ReplayController -> ReplayProxy -> SaveTexture semantics against the RenderDoc source and found no P0-P3 issues.
The remaining gap is that the real Android ASTC capture was not rerun; given the regression coverage and verified RenderDoc call path, I consider that a non-blocking residual risk. Approved.
2 participants
What
Add
rdc texture --sliceso a requested 3D texture depth slice can be exported as a separate PNG.Why
#269 fixed Texture3D metadata and
tex-stats --slicevalidation, butrdc texturestill exported only the default local slice and had inconsistent remote behavior.Fixes #268.
How
--slicetordc textureand route it through the texture VFS path.Texture3D(and array size for array textures).TextureSave.slice.sliceIndex.GetTextureDatareturns the full 3D mip volume.Test plan
pixi run checkpasses (lint + typecheck + tests).rdccapture (Android GLES capture:RainDropSplash01.dds, resource8413, event3235; exported slice 0 and slice 1 as distinct64x64PNGs)Summary by CodeRabbit
--sliceto the texture export command to select a texture-array slice or a 3D depth slice.--slice.