Repository navigation
fix(agc): draw each face with its own depth bias - #1924
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
This is well done and I can approve it.
- The double-record-in-same-render-pass approach is the correct way to give two faces different depth bias under Vulkan's one-bias-per-draw constraint, and
depthBiasPerFacein the pipeline key means you don't accidentally reuse a single-face pipeline for a per-face state. - The
Require(!state.depthBiasPerFace || (depthBias && state.cullMode == VK_CULL_MODE_NONE))guard is exactly right — per-face only makes sense when both faces actually rasterize. ContinueBackFacesrebinds viewport/scissor/depth-bounds after the pipeline switch, which is the detail people forget when binding a secondVkPipelinemid-pass. Good.- The decode math in
decodeDepthBiascorrectly zeroes a face's bias when its offset-enable bit (PA_SU_SC_MODE_CNTL 11/12) is clear, and the tests cover front-only, back-only, equal, and culled-face cases. - The
APS5_REJECT_PER_FACE_DEPTH_BIASescape hatch preserves the old behaviour — nice. - Real GPU test (
DepthBiasPerFace.cpp) with opposite biases and aLESStest against a 0.5 clear, in both directions. Solid. - The debt entry honestly states the two real deviations (vertex stages run twice; all front faces land before all back faces). That's the right way to handle a known-different semantic.
Nothing blocking. The vertex-shader-writes-memory-twice case is the one thing a pathological title could in theory observe, and you've disclosed it. Good.
Written with AI assistance.
|
Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head
No title was run. 🤖 Addressed by Claude Code |
75af5a0 to
9884ab0
Compare
9884ab0 to
2a64f54
Compare
|
This conflicts with main in |
2a64f54 to
75239c9
Compare
|
Rebased onto main 0011248 and the switch is gone. New head: 75239c9.
Tested on an RTX 5070 Ti (NVIDIA 595.91.07), built with AI-assisted: yes |
A draw that rasterizes both faces with different front and back depth bias was rejected, since Vulkan has one depth bias per draw and the cull mode is pipeline state. Such a draw is now recorded twice in the same pass: front faces with the back faces culled and the front bias, then back faces with the front faces culled and the back bias. A face whose offset is disabled gets no bias.
75239c9 to
e0084a5
Compare
|
Rebased onto current |
…-face fix(agc): draw each face with its own depth bias
What
decodeDepthBiasrejected a draw that rasterizes both faces when the front and back depth bias differ (depth bias differing between front and back faces). Vulkan has one depth bias per draw, and the cull mode is baked into the pipeline.Such a draw is now recorded twice in the same render pass:
DecodeStatekeeps the front values indepthBias*, puts the back values in the newbackDepthBiasConstant/backDepthBiasSlope, and setsdepthBiasPerFace. A face whose offset enable bit (PA_SU_SC_MODE_CNTL bit 11 or 12) is clear gets bias 0, which is the same as no bias.Pipelinecreates a secondVkPipelinefor that state. The first one culls back faces, the second culls front faces, anddepthBiasPerFaceis part of the pipeline key.Draw.cpprecord the draw with the first pipeline and then callContinueBackFaces(bind the second pipeline, set the back bias) and record it again. Both pipelines share one layout, so descriptor sets and push constants stay bound.The TechnicalDebt entry records the difference from hardware: the vertex stages run twice, and all front faces land before all back faces instead of in primitive order. That only matters for blending or stencil updates that depend on the order, or for a vertex shader that writes memory.
Closes #1923
Tested
Nobara 44 x86-64 (Linux 7.2), GCC 16.2, RTX 5070 Ti, driver 595.91.07. First tested on
mainf145139; rebased ontomain0011248 and retested (see the end of this section).agc_driver_graphics: the depth-bias block now decodes differing front/back bias with no culling, front-only and back-only bias, and cull-front with the back bias. Onmainit fails, because the decode throws.agc_driver_depth_bias_per_face(tests/execution/DepthBiasPerFace.cpp): a front-facing and a back-facing triangle at depth 0.5, aLESStest against a surface cleared to 0.5, opposite biases on the two faces. Only the face that is pulled closer passes, in both directions. Equal biases and front-only bias are checked too. With a single pipeline that draws both faces with the front bias, it fails (the back-facing triangle at (20, 4) is 255, expected depth-rejected).ctest -j1 --timeout 60: 494 of 495 pass. The one failure,guest_dynamic_loader, also fails here on a branch without this change.tools/check_conventions.pyandtools/check_test_registration.pypass.ANYPS5_ENABLE_SPIRV_TOOLS=ON):agc_driver_depth_bias_per_faceandagc_driver_graphicspass,ctest -R agc_ -j1passes 279 of 280. The one failure,agc_driver_dcc_keys_first_write, fails on plainmain0011248 on this GPU too.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository