fix(cbuffer): handle D3D raw export and vs-in fallback#265
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
📝 WalkthroughWalkthroughConstant-buffer routing now includes shader stage in VFS paths and raw export filenames. Cbuffer value flattening selects numeric lanes by reflected type. ChangesStage-aware cbuffer and mesh vs-in fallback
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by Qodofix(cbuffer): handle D3D raw export, uint types, and vs-in IA fallback
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
Context used 1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rdc/handlers/buffer.py (1)
231-297: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize
stagebefore using it in the temp filenamecbuffer_rawaccepts an arbitrarystage, and the current f-string lets/or..escapestate.temp_dirwhen the file is written. Validatestage_nameagainstSTAGE_MAPbefore building the 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/buffer.py` around lines 231 - 297, The _handle_cbuffer_raw handler currently uses the unvalidated stage_name directly in the temp file name, which can allow path traversal when writing under state.temp_dir. In _handle_cbuffer_raw, validate stage_name against STAGE_MAP before it is used for the temp_path construction, and reject or normalize any unsupported value so the generated filename cannot contain path separators or parent-directory segments.
🧹 Nitpick comments (4)
src/rdc/vfs/tree_cache.py (1)
356-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between per-stage and aggregate cbuffer directory population.
The per-stage loop (356-371) and the aggregate loop (372-379) build near-identical dir/leaf structures, differing mainly in the extra
"data"leaf_bin child. Could be factored into a shared helper parameterized by whether to add thedatachild, but current duplication is small and low-risk.🤖 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/vfs/tree_cache.py` around lines 356 - 379, The cbuffer tree population logic in tree_cache.py duplicates almost the same dir/leaf construction in the per-stage loop and the aggregate loop. Refactor the repeated VfsNode/subtree-building into a shared helper used by the stage and aggregate paths, and make it parameterized so the stage variant can optionally add the "data" leaf_bin child while the aggregate variant omits it; keep the existing behavior in the code paths around cbuffer_stage_names, cbuffer_stage_sets, and cbuffer_sets.src/rdc/handlers/buffer.py (2)
402-419: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePostVS vertex read size isn't bounds-checked like the new IA path.
v_size = getattr(mesh, "vertexByteSize", 0)feedsGetBufferData(mesh.vertexResourceId, 0, v_size)directly, without the_known_byte_sizesentinel guard that_mesh_data_from_ianow applies toBoundVBuffer.byteSize. If PostVS ever reports an unresolved/sentinel size, this could trigger an oversized read. Likely low practical risk since PostVS sizes are usually already resolved by RenderDoc, but worth aligning with the new defensive pattern for consistency.🤖 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/buffer.py` around lines 402 - 419, The PostVS read in _decode_mesh_postvs should use the same defensive byte-size handling as the new IA path, since vertexByteSize may still be an unresolved sentinel. Update the GetBufferData call to guard v_size with the existing _known_byte_size pattern before reading mesh.vertexResourceId, and keep the rest of the decoding flow unchanged. Use _decode_mesh_postvs and _mesh_data_from_ia as the reference points when aligning the bounds check behavior.
47-76: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLane-aware ShaderValue extraction verified correct against RenderDoc's row-major convention.
Checked against RenderDoc's official example scripts, which index matrix values as
v.value.f32v[r*v.columns + c]— matches the new[lane[ri*c+ci] for ri in range(r) for ci in range(c)]formula. u32v/s32v/f32v lane selection also looks right for the type codes used in tests (4→u32v, 5→s32v).Only gap: no
f64vlane for double-typed cbuffer variables — falls through tof32vdefault, which would misread double values if RenderDoc ever reflects them. Likely out of scope for this PR (targets uint specifically) but worth a follow-up if double cbuffer members are expected.🤖 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/buffer.py` around lines 47 - 76, Add support for double-typed ShaderValue extraction in _shader_variable_value_kind and _shader_variable_values so reflected cbuffer variables that use doubles are not misread as f32v. Update the lane selection logic alongside the existing u32v/s32v/f32v handling in the _shader_variable_value_kind helper, and ensure _shader_variable_values can read the correct lane from var.value when the reflected type indicates a 64-bit float.tests/unit/test_buffer_decode.py (1)
290-311: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid new coverage; one gap worth calling out.
Traced the assertions in
TestMeshVsInFallback(637-831) against_mesh_data_from_iaand they're internally consistent with the implementation's use ofaction.indexOffsetfor both indexed and non-indexed offset math. That consistency is exactly what's masking theindexOffset/vertexOffsetfield-naming issue flagged insrc/rdc/handlers/buffer.py(see comment there) — the mock doesn't model RenderDoc's real distinction between the two fields for non-indexed draws, so this suite can't catch that class of bug. Worth adding a mock field forvertexOffsetdistinct fromindexOffsetonce the source fix lands, to prevent regression.Also applies to: 314-326, 339-398, 411-411, 485-485, 533-533, 637-831, 1033-1045
🤖 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_buffer_decode.py` around lines 290 - 311, Update the buffer decode test coverage around _mesh_data_from_ia and TestMeshVsInFallback so the mock reflects RenderDoc’s real distinction between indexOffset and vertexOffset for non-indexed draws. In the existing test setup that currently only uses action.indexOffset, add a separate vertexOffset field in the mocked action object and use it in the non-indexed path assertions, while keeping indexed behavior tied to indexOffset. This will make the assertions in TestMeshVsInFallback exercise the field-naming fix in src/rdc/handlers/buffer.py and prevent the current mock from hiding regressions.
🤖 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/buffer.py`:
- Around line 475-552: The non-indexed path in _mesh_data_from_ia is using
action.indexOffset to choose the starting vertex, but that field only applies to
indexed draws. Update the non-indexed branch to read action.vertexOffset
instead, keeping the indexed logic unchanged, and make sure the vertex range
computation still flows through first_vertex, num_verts, and the final
GetBufferData call.
---
Outside diff comments:
In `@src/rdc/handlers/buffer.py`:
- Around line 231-297: The _handle_cbuffer_raw handler currently uses the
unvalidated stage_name directly in the temp file name, which can allow path
traversal when writing under state.temp_dir. In _handle_cbuffer_raw, validate
stage_name against STAGE_MAP before it is used for the temp_path construction,
and reject or normalize any unsupported value so the generated filename cannot
contain path separators or parent-directory segments.
---
Nitpick comments:
In `@src/rdc/handlers/buffer.py`:
- Around line 402-419: The PostVS read in _decode_mesh_postvs should use the
same defensive byte-size handling as the new IA path, since vertexByteSize may
still be an unresolved sentinel. Update the GetBufferData call to guard v_size
with the existing _known_byte_size pattern before reading mesh.vertexResourceId,
and keep the rest of the decoding flow unchanged. Use _decode_mesh_postvs and
_mesh_data_from_ia as the reference points when aligning the bounds check
behavior.
- Around line 47-76: Add support for double-typed ShaderValue extraction in
_shader_variable_value_kind and _shader_variable_values so reflected cbuffer
variables that use doubles are not misread as f32v. Update the lane selection
logic alongside the existing u32v/s32v/f32v handling in the
_shader_variable_value_kind helper, and ensure _shader_variable_values can read
the correct lane from var.value when the reflected type indicates a 64-bit
float.
In `@src/rdc/vfs/tree_cache.py`:
- Around line 356-379: The cbuffer tree population logic in tree_cache.py
duplicates almost the same dir/leaf construction in the per-stage loop and the
aggregate loop. Refactor the repeated VfsNode/subtree-building into a shared
helper used by the stage and aggregate paths, and make it parameterized so the
stage variant can optionally add the "data" leaf_bin child while the aggregate
variant omits it; keep the existing behavior in the code paths around
cbuffer_stage_names, cbuffer_stage_sets, and cbuffer_sets.
In `@tests/unit/test_buffer_decode.py`:
- Around line 290-311: Update the buffer decode test coverage around
_mesh_data_from_ia and TestMeshVsInFallback so the mock reflects RenderDoc’s
real distinction between indexOffset and vertexOffset for non-indexed draws. In
the existing test setup that currently only uses action.indexOffset, add a
separate vertexOffset field in the mocked action object and use it in the
non-indexed path assertions, while keeping indexed behavior tied to indexOffset.
This will make the assertions in TestMeshVsInFallback exercise the field-naming
fix in src/rdc/handlers/buffer.py and prevent the current mock from hiding
regressions.
🪄 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: f32c7721-ec10-4b7a-ab55-ab32ec042f05
📒 Files selected for processing (8)
src/rdc/commands/cbuffer.pysrc/rdc/handlers/buffer.pysrc/rdc/vfs/router.pysrc/rdc/vfs/tree_cache.pytests/unit/test_buffer_decode.pytests/unit/test_cbuffer_commands.pytests/unit/test_fix2_vfs_intermediate_dirs.pytests/unit/test_vfs_router.py
f02187d to
a9434bb
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
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 `@tests/unit/test_buffer_decode.py`:
- Around line 672-698: The non-indexed fallback in buffer decoding is using the
wrong draw field, so update the logic in the buffer handler to read the
vertex-offset field instead of indexOffset when there are no indices. Adjust the
related test in test_non_indexed_fallback_applies_first_vertex, along with any
mocked action setup, to use vertexOffset/FirstVertexLocation semantics so the
expected vertex range matches the real RenderDoc behavior.
🪄 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: d8907a84-4caa-49a2-8f0c-3c05e6fcff5c
📒 Files selected for processing (8)
src/rdc/commands/cbuffer.pysrc/rdc/handlers/buffer.pysrc/rdc/vfs/router.pysrc/rdc/vfs/tree_cache.pytests/unit/test_buffer_decode.pytests/unit/test_cbuffer_commands.pytests/unit/test_fix2_vfs_intermediate_dirs.pytests/unit/test_vfs_router.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/rdc/commands/cbuffer.py
- tests/unit/test_cbuffer_commands.py
- tests/unit/test_fix2_vfs_intermediate_dirs.py
- src/rdc/vfs/router.py
- tests/unit/test_vfs_router.py
- src/rdc/vfs/tree_cache.py
- src/rdc/handlers/buffer.py
a9434bb to
00371d2
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
I think there are two different use cases that should be separated:
VS input is not always a conventional position-backed mesh. For example, UI/sprite/procedural meshes may store only indices, tile IDs, corner IDs, or instance data in VS inputs, and reconstruct the real position in the vertex shader from constants/textures. In those cases there may be no POSITION attribute at all. So a full VS-IN export should dump all input attributes per vertex/index and should not require any attribute to be named or inferred as POSITION.
For OBJ export, selecting a position attribute is still necessary because OBJ needs vertex positions. But the current name-based lookup is too fragile. A better position selection strategy would be:
For the D3D12 NTE capture I tested, the VS-IN attributes are Unreal/D3D12 captures often expose inputs as |
Summary
f32v, including numeric uint variables reported by D3D captures.vs bNandps bNcan resolve different resources at the same set/binding, with stage validation before temp-file output.mesh --stage vs-inwhen RenderDoc PostVS VSIn data is missing or mirrors VSOut.Test Evidence
uv run --extra dev pytest tests/unit/test_buffer_decode.py tests/unit/test_cbuffer_commands.py tests/unit/test_vfs_router.py tests/unit/test_fix2_vfs_intermediate_dirs.py -qpixi run lintpixi run typecheckpixi run test(3071 passed, 6 skipped, coverage93.20%)pixi run check00371d29acfa8bae1da82d7a68604ab410f76493Notes
/draws/<eid>/cbuffer/<stage>/<set>/<binding>/data..rdccaptures was not run becauserdc doctorreports the RenderDoc Python replay module is unavailable in this environment. The unit tests cover the reported failure modes with mocks, but Windows D3D11/D3D12 capture verification remains needed.