fix(lighting): create Masks2 RT and divide SSGI AO by vertexAO#2411
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds MASKS2 (RT7) carrying per-pixel vertex AO (1.0 - vertexAO). C++ allocates and exposes MASKS2 SRV; Lighting and RunGrass shaders write Masks2; DeferredComposite binds Masks2 at t9 and uses it to normalize SSGI/skylighting AO samples. ChangesVertex AO Render Target and Deferred Lighting Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.12][ERROR]: Error: exception Unix_error: No such file or directory stat src/Deferred.cpp 🔧 Infer (1.2.0)src/Deferred.cppUsage Error: Failed to execute compilation command: Error message: *** Infer needs a working compilation command to run. 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 |
|
No actionable suggestions for changed features. |
2cdf4d3 to
1cac730
Compare
1cac730 to
9c2247f
Compare
Adds a dedicated render target for vertex AO so the deferred composite can correct SSGI overdarkening: vanilla SSAO does not know about vertex-color AO that has already shadowed mesh/grass pixels, and SSGI then darkens the already-darkened pixels. - src/Deferred.cpp wires up MASKS2 (R16G16B16A16_FLOAT, blendable) as the 8th forward render target and binds its SRV at t9 for DeferredCompositeCS. The MASKS2 macro already exists in Deferred.h. The buffer is cleared every frame via the existing SRTM_CLEAR loop in StartDeferred (alongside the other forward render targets). - Lighting.hlsl and RunGrass.hlsl write the vertex AO mask, since they are the paths that bake vertex AO into the deferred color. The value is stored as `1 - vertexAO`, so the cleared default (0) correctly maps to vertexAO = 1 for any pixel that doesn't write (sky, water, distant trees, effects, decals). - DeferredCompositeCS samples Masks2.x, recovers vertexAO via `1 - x`, and divides ssgiAo by it before the multi-bounce AO step so SSGI darkens only the unshadowed remainder. - Lighting.hlsl's stale per-SNOW psout.Parameters output, which sat at SV_Target7 with no consumer, is replaced by Masks2; its writes are removed. Shader-side correctness fixes for the vertexAO value itself: - RunGrass.hlsl: both pixel shader paths previously normalised vertexColor by its own max before computing vertexAO, so vertexAO always evaluated to ~1 and never modulated skylighting diffuse. Capture vertexAO before normalising, then divide vertexColor by it. - Lighting.hlsl SKYLIGHTING + else paths: wrap the max-of-channels with Color::ColorToLinear so vertexAO lives in linear space, matching the linear skylightingDiffuse / ssgiAo it scales.
9c2247f to
f2d0a52
Compare
|
looks fine to me |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/Shaders/DeferredCompositeCS.hlsl (1)
143-146: 💤 Low valueMinor: Comment is incomplete.
The comment says "(Lighting.hlsl only)" but
RunGrass.hlslalso writes toMasks2. Consider updating to "(Lighting.hlsl and RunGrass.hlsl)" for accuracy.Otherwise, the vertexAO recovery and SSGI normalization logic is correct.
📝 Suggested comment update
- // Masks2.x stores 1 - vertexAO (Lighting.hlsl only); cleared to 0 for + // Masks2.x stores 1 - vertexAO (Lighting.hlsl, RunGrass.hlsl); cleared to 0 for // pixels with no vertex AO contribution, so vertexAO defaults to 1.🤖 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 `@package/Shaders/DeferredCompositeCS.hlsl` around lines 143 - 146, Update the inline comment above the vertexAO recovery to correctly list both writers of Masks2: change "(Lighting.hlsl only)" to "(Lighting.hlsl and RunGrass.hlsl)" so the comment accurately reflects that Masks2Texture is written by both Lighting.hlsl and RunGrass.hlsl; leave the existing vertexAO calculation (float vertexAO = 1.0 - Masks2Texture[dispatchID.xy].x;) and the ssgi normalization (ssgiAo = saturate(ssgiAo / max(vertexAO, EPSILON_DIVISION));) 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.
Nitpick comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 143-146: Update the inline comment above the vertexAO recovery to
correctly list both writers of Masks2: change "(Lighting.hlsl only)" to
"(Lighting.hlsl and RunGrass.hlsl)" so the comment accurately reflects that
Masks2Texture is written by both Lighting.hlsl and RunGrass.hlsl; leave the
existing vertexAO calculation (float vertexAO = 1.0 -
Masks2Texture[dispatchID.xy].x;) and the ssgi normalization (ssgiAo =
saturate(ssgiAo / max(vertexAO, EPSILON_DIVISION));) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 681293f0-806b-4d57-8c60-95b79a3419b9
📒 Files selected for processing (4)
package/Shaders/DeferredCompositeCS.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Deferred.cpp
|
✅ A pre-release build is available for this PR: |
jiayev
left a comment
There was a problem hiding this comment.
you are creating a r16g16b16a16 for one channel, pls make use of one alpha
Only the .x channel (vertexAO) is used, so R16G16B16A16_FLOAT wastes 3 channels. R16_UNORM is sufficient for a [0,1] value and gives full 16-bit precision. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extracted from the
effect11sh2branch as a standalone change.Summary
Adds a dedicated render target for vertex AO so the deferred composite can correct SSGI overdarkening: vanilla SSAO does not know about vertex-color AO that has already shadowed mesh/grass pixels, and SSGI then darkens the already-darkened pixels.
Plumbing (C++ + RT)
src/Deferred.cppwires upMASKS2(R16G16B16A16_FLOAT, blendable) as the 8th forward render target and binds its SRV att9forDeferredCompositeCS. TheMASKS2macro already exists inDeferred.h.SRTM_CLEARloop inStartDeferred—MASKS2lives at index 7 offorwardRenderTargets, which the loopfor (uint i = 2; i < 8; i++)already marks asSRTM_CLEAR. No extra clear calls are added.Shader-side writes
Lighting.hlslandRunGrass.hlslwrite the mask — these are the paths that bake vertex AO into the deferred color, so they're the paths SSGI needs to compensate for. Distant trees, effects, and decals don't apply vertex AO and are left alone.1 - vertexAO. The cleared default of0then correctly maps to "no occlusion" (vertexAO = 1) for any pixel that doesn't write (sky, water, distant trees, effects, decals). This avoids per-feature explicit writes for paths that have no vertex AO.vertexAOin all branches (HAIR=1, SKYLIGHTING=existing, else=new) so thepsout.Masks2write is well-defined.psout.Parametersoutput, which sat atSV_Target7with no consumer, is replaced byMasks2— its writes are removed.Shader-side read
DeferredCompositeCS.hlslsamplesMasks2.x, recoversvertexAO = 1 - x, and dividesssgiAoby it before the multi-bounce AO step, so SSGI darkens only the unshadowed remainder.vertexAO correctness fixes
RunGrass.hlsl— Both pixel shader paths previously normalisedvertexColorby its own max before computingvertexAO, sovertexAOalways evaluated to ~1 and never modulatedskylightingDiffuse. CapturevertexAObefore the normalisation divide, then dividevertexColorby it.Lighting.hlslSKYLIGHTING + else paths — wrap the max-of-channels withColor::ColorToLinearsovertexAOlives in linear space, matching the linearskylightingDiffuse/ssgiAoit scales.Test plan
Masks2is bound as the 8th deferred RT and att9SRV inDeferredCompositeCS. Confirm the RT is cleared to0,0,0,0at the start of each frame (viaSRTM_CLEAR).Masks2at the cleared0value, which the composite reads as no AO modulation.Summary by CodeRabbit