feat: new SSGI and SSR + NRD - #2573
Conversation
Separates Screen Space Reflections from the ScreenSpaceGI feature into its
own standalone feature (features/Screen Space Reflections + src/Features/
ScreenSpaceReflections.{h,cpp}) so reflections can be toggled independently
of diffuse GI.
Promotes the NVIDIA Real-Time Denoiser (NRD) integration to a core
top-level feature (features/NRD with CORE marker + src/Features/NRD.{h,cpp})
that owns the shared guide textures (viewZ, packed normal+roughness, motion
vectors), the prepareNRDGuides compute pass, and the common per-frame
camera/jitter state. Consumers (SSGI, SSR, and any future denoising
clients) construct their own NRDReblurIntegration instances and consume
guides via the global NRD service. REBLURSettings UI/struct is centralized
on NRD so every consumer gets identical controls.
- Move specularGI / prefilterHiZDepth / depthDownsample shaders into the
new SSR feature folder; SSR's specularGI cbuffer is now SSRCB (b1).
- Move prepareNRDGuides into the new NRD feature folder.
- DeferredCompositeCS: SSGI_SPECULAR / SsgiSpecularTexture renamed to
SSR / SsrTexture and lifted out of the SSGI #ifdef block so SSR can
run without SSGI being enabled.
- SharedData: split SSGISettings (diffuse-only) and SSRSettings; SSR
appended to the FeatureData cbuffer.
- Deferred.cpp: NRD.PrepareGuides() runs first, then SSGI diffuse, then
SSR specular; the SSR composite define is gated on the SSR feature.
- Globals / Feature.cpp / FeatureBuffer.cpp / SceneSettingsManager /
VR::AnyScreenSpaceEffectLoaded updated to register the new features.
Removes SampleSSGISpecular (the roughness-biased re-sampling of the SSGI diffuse texture as approximate specular) and its companion ssgiAo→specular modulation block in DeferredCompositeCS. With ScreenSpaceReflections now providing proper stochastic Hi-Z reflections, the SSGI-derived faux specular path is redundant — and removing it means SSGI's contribution to the composite is strictly diffuse + AO, which is its actual purpose.
Pairs the existing Tracy zones with BeginPerfEvent/EndPerfEvent calls gated on State::frameAnnotations, matching the pattern used by SSS, Skylighting, HDRDisplay, and LightLimitFix. RenderDoc/PIX/Nsight captures now show the SSR pipeline as named groups (SSR → Hi-Z Depth, Specular GI, REBLUR Specular) instead of bare compute dispatches.
This reverts commit 57a080d.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change integrates NRD, replaces the SSGI pipeline with REBLUR-based processing, adds screen-space reflections, updates deferred bindings, and changes IBL skylighting controls. ChangesNRD and build integration
SSGI pipeline replacement
Screen-space reflections
Deferred rendering and shared data
IBL and localized metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Deferred
participant NRD
participant ScreenSpaceGI
participant ScreenSpaceReflections
participant Composite
Deferred->>NRD: Prepare guides
Deferred->>ScreenSpaceGI: Generate and denoise diffuse GI
ScreenSpaceGI-->>Deferred: Diffuse and SH1 outputs
Deferred->>ScreenSpaceReflections: Generate Hi-Z and trace reflections
ScreenSpaceReflections-->>Deferred: Specular output
Deferred->>Composite: Bind GI and SSR resources
Composite-->>Deferred: Composite lighting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.gitmodules (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider updating the PR title and description.
As per path instructions, please ensure the following PR conventions are met:
- Conventional Commit Titles: The PR title
feat: new SSGI and SSR + NRDshould ideally use a fully lowercase description, e.g.,feat: integrate nrd, ssgi, and ssr.- Issue References: If this PR implements features or fixes bugs, consider adding appropriate GitHub keywords to the PR description (e.g.,
Implements#123or `Fixes `#123).🤖 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 @.gitmodules around lines 11 - 13, Update the pull request metadata rather than the .gitmodules change: use a Conventional Commit title with a fully lowercase description, such as “feat: integrate nrd, ssgi, and ssr,” and add relevant GitHub issue-closing or implementation references to the PR description when applicable.Source: Path instructions
src/Features/IBL.cpp (1)
75-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid casting
uint*tobool*for ImGui.
ImGui::Checkboxexpects a 1-bytebool*, butsettings.SkylightingAffectsEnvis a 4-byteuint. Castinguint*tobool*causes ImGui to only read and write the first byte. While this happens to work on little-endian architectures for values of0and1, it violates strict aliasing rules and could lead to undefined behavior.Consider using a temporary boolean variable to interface with ImGui safely.
♻️ Proposed refactor
- ImGui::Checkbox(T(TKEY("skylighting_affects_env"), "Skylighting Affects Env/DALC"), (bool*)&settings.SkylightingAffectsEnv); + bool skylightingAffectsEnv = settings.SkylightingAffectsEnv != 0; + if (ImGui::Checkbox(T(TKEY("skylighting_affects_env"), "Skylighting Affects Env/DALC"), &skylightingAffectsEnv)) { + settings.SkylightingAffectsEnv = skylightingAffectsEnv ? 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 `@src/Features/IBL.cpp` around lines 75 - 83, Update the ImGui checkbox handling for settings.SkylightingAffectsEnv to avoid casting the uint field to bool*. Use a temporary bool initialized from the setting, pass it to ImGui::Checkbox, and write the resulting value back to the uint field when the checkbox changes.extern/NRD (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalize the PR title and add an issue reference.
Consider
feat: add ssgi and ssr with nrd; if this feature addresses an issue, addImplements #<issue-number>orAddresses #<issue-number>to the PR description.As per path instructions: use lowercase Conventional Commit descriptions and include GitHub issue keywords for feature work.
🤖 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 `@extern/NRD` at line 1, Normalize the pull request title to a lowercase Conventional Commit description, such as “feat: add ssgi and ssr with nrd”, and add an “Implements #<issue-number>” or “Addresses #<issue-number>” reference to the pull request description when an issue exists.Source: Path instructions
features/Screen Space GI/Shaders/Features/ScreenSpaceGI.ini (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePR meta: consider adding issue references.
The title
feat: new SSGI and SSR + NRDis Conventional-Commit compliant. Since this integrates several features, consider linking the tracking issues in the body (e.g.Implements#123, `Related to `#123) so the work is traceable.🤖 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 `@features/Screen` Space GI/Shaders/Features/ScreenSpaceGI.ini at line 2, Add tracking issue references to the pull request body for the SSGI, SSR, and NRD integration, using labels such as “Implements `#123`” or “Related to `#123`” as appropriate. Keep the existing Conventional Commit title unchanged.Source: Path instructions
src/Features/ScreenSpaceGI.cpp (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePR title/metadata (per path instructions).
The PR title
feat: new SSGI and SSR + NRDbundles multiple concerns and reads informally. Consider a Conventional Commit style title, e.g.feat(rendering): integrate NRD REBLUR for SSGI and SSR, and add issue-reference keywords in the body (Implements #…/Closes #…) if this tracks an issue.As per path instructions: "Conventional Commit Titles … Format: type(scope): description … lowercase description, no ending period" and "Issue References … Suggest adding appropriate GitHub keywords".
🤖 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/Features/ScreenSpaceGI.cpp` around lines 7 - 13, Update the pull request metadata rather than the code: use a Conventional Commit title in the format type(scope): lowercase description without a trailing period, such as “feat(rendering): integrate NRD REBLUR for SSGI and SSR”. Add an appropriate issue-reference keyword such as “Implements #…” or “Closes #…” in the pull request body when applicable.Source: Path instructions
🤖 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 `@features/Screen` Space GI/Shaders/ScreenSpaceGI/diffuseGI.cs.hlsl:
- Around line 265-268: Update the conditional normal-orientation check in the
diffuse GI shader to test viewspaceNormal rather than pixCenterPos. Preserve the
existing viewspaceNormal flip, but compare it against viewVec so the branch can
correctly reorient normals facing away from the view direction.
In `@src/Features/ScreenSpaceReflections.cpp`:
- Line 340: Update the SpecMaxMips assignment in the ScreenSpaceReflections
setup to depend only on the available numHiZMips, removing the SpecMaxSteps
clamp so ray-march iteration limits do not reduce the depth hierarchy mip count.
---
Nitpick comments:
In @.gitmodules:
- Around line 11-13: Update the pull request metadata rather than the
.gitmodules change: use a Conventional Commit title with a fully lowercase
description, such as “feat: integrate nrd, ssgi, and ssr,” and add relevant
GitHub issue-closing or implementation references to the PR description when
applicable.
In `@extern/NRD`:
- Line 1: Normalize the pull request title to a lowercase Conventional Commit
description, such as “feat: add ssgi and ssr with nrd”, and add an “Implements
#<issue-number>” or “Addresses #<issue-number>” reference to the pull request
description when an issue exists.
In `@features/Screen` Space GI/Shaders/Features/ScreenSpaceGI.ini:
- Line 2: Add tracking issue references to the pull request body for the SSGI,
SSR, and NRD integration, using labels such as “Implements `#123`” or “Related to
`#123`” as appropriate. Keep the existing Conventional Commit title unchanged.
In `@src/Features/IBL.cpp`:
- Around line 75-83: Update the ImGui checkbox handling for
settings.SkylightingAffectsEnv to avoid casting the uint field to bool*. Use a
temporary bool initialized from the setting, pass it to ImGui::Checkbox, and
write the resulting value back to the uint field when the checkbox changes.
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 7-13: Update the pull request metadata rather than the code: use a
Conventional Commit title in the format type(scope): lowercase description
without a trailing period, such as “feat(rendering): integrate NRD REBLUR for
SSGI and SSR”. Add an appropriate issue-reference keyword such as “Implements
#…” or “Closes #…” in the pull request body when applicable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7c6d96c-7a6e-408e-bf2d-fe0cd52976b6
📒 Files selected for processing (54)
.gitmodulesCMakeLists.txtextern/NRDfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslifeatures/Dynamic Cubemaps/Shaders/Features/DynamicCubemaps.inifeatures/Exponential Height Fog/Shaders/Features/ExponentialHeightFog.inifeatures/IBL/Shaders/Features/ImageBasedLighting.inifeatures/IBL/Shaders/IBL/IBL.hlslifeatures/NRD/COREfeatures/NRD/Shaders/Features/NRD.inifeatures/NRD/Shaders/NRD/prepareNRDGuides.cs.hlslfeatures/Screen Space GI/Shaders/Features/ScreenSpaceGI.inifeatures/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/common.hlslifeatures/Screen Space GI/Shaders/ScreenSpaceGI/composite.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/diffuseGI.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterNormal.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Screen Space Reflections/Shaders/Features/ScreenSpaceReflections.inifeatures/Screen Space Reflections/Shaders/ScreenSpaceReflections/common.hlslifeatures/Screen Space Reflections/Shaders/ScreenSpaceReflections/depthDownsample.cs.hlslfeatures/Screen Space Reflections/Shaders/ScreenSpaceReflections/prefilterHiZDepth.cs.hlslfeatures/Screen Space Reflections/Shaders/ScreenSpaceReflections/specularGI.cs.hlslpackage/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/ru.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/zh_CN.jsonpackage/Shaders/Common/SharedData.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/NRD/NRDReblurSH.hlslipackage/Shaders/RunGrass.hlslpackage/Shaders/Water.hlslsrc/Deferred.cppsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/IBL.cppsrc/Features/IBL.hsrc/Features/NRD.cppsrc/Features/NRD.hsrc/Features/NRDReblurIntegration.cppsrc/Features/NRDReblurIntegration.hsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceGI.hsrc/Features/ScreenSpaceReflections.cppsrc/Features/ScreenSpaceReflections.hsrc/Features/Skylighting.hsrc/Globals.cppsrc/Globals.hsrc/SceneSettingsManager.cpp
💤 Files with no reviewable changes (7)
- src/Features/Skylighting.h
- package/Shaders/DistantTree.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl
- package/SKSE/Plugins/CommunityShaders/Translations/ru.json
| const float3 viewVec = normalize(-pixCenterPos); | ||
|
|
||
| if (dot(viewVec, pixCenterPos) > 0) | ||
| viewspaceNormal = -viewspaceNormal; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='features/Screen Space GI/Shaders/ScreenSpaceGI/diffuseGI.cs.hlsl'
# Show surrounding context with line numbers
sed -n '230,290p' "$FILE" | cat -n
# Find definitions/usages of the relevant symbols in the file
rg -n --context 3 'pixCenterPos|viewVec|viewspaceNormal' "$FILE"Repository: community-shaders/skyrim-community-shaders
Length of output: 5553
Dead normal-flip: this branch never runs. dot(normalize(-pixCenterPos), pixCenterPos) is always <= 0, so viewspaceNormal is never re-oriented here. This should test the normal instead of the position.
🤖 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 `@features/Screen` Space GI/Shaders/ScreenSpaceGI/diffuseGI.cs.hlsl around
lines 265 - 268, Update the conditional normal-orientation check in the diffuse
GI shader to test viewspaceNormal rather than pixCenterPos. Preserve the
existing viewspaceNormal flip, but compare it against viewVec so the branch can
correctly reorient normals facing away from the view direction.
| data.FrameIndex = globals::state->frameCount; | ||
|
|
||
| data.SpecMaxSteps = settings.SpecMaxSteps; | ||
| data.SpecMaxMips = std::min((uint)settings.SpecMaxSteps, numHiZMips); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
SpecMaxMips clamped by SpecMaxSteps couples unrelated controls.
SpecMaxMips represents the available Hi-Z mip count, but it's clamped by SpecMaxSteps (max ray-march iterations). With a low Max Steps value (slider allows down to 1), this unintentionally shrinks the depth hierarchy (SSRT_DEPTH_HIERARCHY_MAX_MIP), degrading traversal quality rather than just iteration count. Consider clamping by numHiZMips alone (or a dedicated mip control).
Proposed change
- data.SpecMaxMips = std::min((uint)settings.SpecMaxSteps, numHiZMips);
+ data.SpecMaxMips = numHiZMips;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data.SpecMaxMips = std::min((uint)settings.SpecMaxSteps, numHiZMips); | |
| data.SpecMaxMips = numHiZMips; |
🤖 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/Features/ScreenSpaceReflections.cpp` at line 340, Update the SpecMaxMips
assignment in the ScreenSpaceReflections setup to depend only on the available
numHiZMips, removing the SpecMaxSteps clamp so ray-march iteration limits do not
reduce the depth hierarchy mip count.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/SKSE/Plugins/CommunityShaders/Translations/en.json (1)
793-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a scoped conventional commit title and link the tracked issue.
Consider renaming the PR to
feat(rendering): add SSGI, SSR, and NRD. If this implements a tracked feature, addImplements #<id>orAddresses #<id>to the PR body.As per path instructions, use
type(scope): descriptiontitles and appropriate GitHub issue keywords for feature work.🤖 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/SKSE/Plugins/CommunityShaders/Translations/en.json` around lines 793 - 794, Rename the pull request using the scoped conventional format `feat(rendering): add SSGI, SSR, and NRD`, and add the applicable tracked issue reference to the PR body using `Implements #<id>` or `Addresses #<id>`.Source: Path instructions
🤖 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/SKSE/Plugins/CommunityShaders/Translations/en.json`:
- Around line 793-794: Rename the pull request using the scoped conventional
format `feat(rendering): add SSGI, SSR, and NRD`, and add the applicable tracked
issue reference to the PR body using `Implements #<id>` or `Addresses #<id>`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 65007d8e-1823-4dbc-a1c0-1c43ab98cd10
📒 Files selected for processing (2)
package/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package/SKSE/Plugins/CommunityShaders/Translations/zh_CN.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package/Shaders/Lighting.hlsl (2)
2944-2947: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard the grazing-angle projection denominator.
layerViewProjection.zcan reach zero when the view direction is parallel to the layer plane, causing invalid UV coordinates and corrupted multi-layer-parallax samples.Proposed fix
float3 layerViewProjection = -layerNormal.xyz * layerViewAngle.xxx - tangentViewDirection.xyz; -float2 layerUv = uv * MultiLayerParallaxData.zw + (0.0009765625 * (layerValue / abs(layerViewProjection.z))).xx * layerViewProjection.xy; +float layerProjectionZ = max(abs(layerViewProjection.z), EPSILON_DIVISION); +float2 layerUv = uv * MultiLayerParallaxData.zw + (0.0009765625 * (layerValue / layerProjectionZ)).xx * layerViewProjection.xy;🤖 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/Lighting.hlsl` around lines 2944 - 2947, Guard the denominator in the layerUv calculation within the multi-layer parallax projection, using a small nonzero epsilon while preserving the denominator’s sign so layerViewProjection.z cannot reach zero. Keep the existing projection and UV offset behavior unchanged for non-grazing angles.
2951-2956: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the multi-layer attenuation after populating indirect lobe weights.
GetIndirectLobeWeightsruns at Line 3070 and resets the output before assigning its diffuse lobe, so this multiplication is overwritten and multi-layer-parallax surfaces retain the full base diffuse indirect contribution. (raw.githubusercontent.com)Proposed fix
- indirectLobeWeights.diffuse *= 1.0 - mlpBlendFactor; ... GetIndirectLobeWeights(indirectLobeWeights, indirectContext, material, uvOriginal); +#if defined(MULTI_LAYER_PARALLAX) +indirectLobeWeights.diffuse *= 1.0 - mlpBlendFactor; +#endif🤖 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/Lighting.hlsl` around lines 2951 - 2956, Move the multi-layer attenuation of indirectLobeWeights.diffuse from the current block to after GetIndirectLobeWeights has populated the weights, ensuring the multiplication is not overwritten while preserving the existing mlpBlendFactor calculation.
🧹 Nitpick comments (1)
package/Shaders/Lighting.hlsl (1)
3032-3036: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImprove the PR metadata before merge.
The current title is conventional, but
feat(rendering): add SSGI, SSR, and NRDis more descriptive for this cross-layer rendering change. If a separate tracking issue exists, addImplements #<issue>orAddresses #<issue>to the PR body. (github.com)As per path instructions, “When reviewing PRs, please provide suggestions for Conventional Commit Titles” and “Issue References.”
🤖 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/Lighting.hlsl` around lines 3032 - 3036, Update the pull request metadata rather than the shader code: use the Conventional Commit title “feat(rendering): add SSGI, SSR, and NRD” and, if a tracking issue exists, add an “Implements #<issue>” or “Addresses #<issue>” reference to the PR body.Source: Path instructions
🤖 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.
Outside diff comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 2944-2947: Guard the denominator in the layerUv calculation within
the multi-layer parallax projection, using a small nonzero epsilon while
preserving the denominator’s sign so layerViewProjection.z cannot reach zero.
Keep the existing projection and UV offset behavior unchanged for non-grazing
angles.
- Around line 2951-2956: Move the multi-layer attenuation of
indirectLobeWeights.diffuse from the current block to after
GetIndirectLobeWeights has populated the weights, ensuring the multiplication is
not overwritten while preserving the existing mlpBlendFactor calculation.
---
Nitpick comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 3032-3036: Update the pull request metadata rather than the shader
code: use the Conventional Commit title “feat(rendering): add SSGI, SSR, and
NRD” and, if a tracking issue exists, add an “Implements #<issue>” or “Addresses
#<issue>” reference to the PR body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 331d3714-82ed-4fa3-a93e-5a51e18bcbe6
📒 Files selected for processing (3)
package/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Features/Skylighting.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Features/Skylighting.h
- package/Shaders/RunGrass.hlsl
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/RunGrass.hlsl (1)
583-585: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the IL ambient suppression to both grass shader variants.
The non-
GRASS_LIGHTINGpixel-shader path at Lines 747-759 still addsdirectionalAmbientColorwhenSharedData::ssgiSettings.EnableIL != 0. Since both paths are deferred variants, grass lighting changes based solely on the permutation. Mirror this gate in the alternate path or factor the shared ambient handling into one helper.Proposed fix
if (SharedData::iblSettings.EnableIBL) { ... } + if (SharedData::ssgiSettings.EnableIL != 0) + directionalAmbientColor = 0; + diffuseColor += directionalAmbientColor;🤖 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/RunGrass.hlsl` around lines 583 - 585, Apply the same EnableIL ambient suppression to the non-GRASS_LIGHTING pixel-shader path near its directionalAmbientColor contribution, matching the existing gate in the GRASS_LIGHTING variant. Ensure both deferred grass shader permutations set directionalAmbientColor to zero when SharedData::ssgiSettings.EnableIL is nonzero, while preserving their existing behavior otherwise.
🧹 Nitpick comments (1)
package/Shaders/RunGrass.hlsl (1)
583-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an issue reference to the PR description if one exists.
The title already follows Conventional Commit style. Because this PR implements features, add
Implements #<issue>orAddresses #<issue>to the PR body when a tracking issue is available.As per path instructions, feature PRs should include an appropriate GitHub issue keyword when applicable.
🤖 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/RunGrass.hlsl` around lines 583 - 585, Update the pull request description to include “Implements #<issue>” or “Addresses #<issue>” when a tracking GitHub issue exists; no code changes are needed in the RunGrass shader.Source: Path instructions
🤖 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.
Outside diff comments:
In `@package/Shaders/RunGrass.hlsl`:
- Around line 583-585: Apply the same EnableIL ambient suppression to the
non-GRASS_LIGHTING pixel-shader path near its directionalAmbientColor
contribution, matching the existing gate in the GRASS_LIGHTING variant. Ensure
both deferred grass shader permutations set directionalAmbientColor to zero when
SharedData::ssgiSettings.EnableIL is nonzero, while preserving their existing
behavior otherwise.
---
Nitpick comments:
In `@package/Shaders/RunGrass.hlsl`:
- Around line 583-585: Update the pull request description to include
“Implements #<issue>” or “Addresses #<issue>” when a tracking GitHub issue
exists; no code changes are needed in the RunGrass shader.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a071b39a-c3ae-4459-8fe2-f7cc3f9ab8c2
📒 Files selected for processing (2)
CMakeLists.txtpackage/Shaders/RunGrass.hlsl
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Features/ScreenSpaceGI.cpp (1)
758-761: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix half-resolution SSGI NRD input handling.
SSGI_HALFcomputes checkerboardpxCoordfrom the compactdtid, butuavs.at(0)still receives the full-resolutiontexNRDInput. This places samples only in the left half of the NRD input and can carry over stale pixels. CleartexNRDInputbefore half-resolution dispatch and map the generated SSGI samples back to their original checkerboard positions, or move the entire SSGI/NRD pipeline onto one consistent half-width texture/format.🤖 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/Features/ScreenSpaceGI.cpp` around lines 758 - 761, Update the SSGI_HALF dispatch path so texNRDInput is cleared before use and generated samples are written to their original checkerboard positions rather than the compact left-half coordinates. Keep the full-resolution path unchanged, and ensure stale NRD input pixels cannot persist between half-resolution dispatches.features/IBL/Shaders/IBL/IBL.hlsli (2)
152-162: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply
DALCModeconsistently in both convenience helpers.
GetIBLColorandGetIBLColorOccludedalways useGetEnvIBLColor. WhenDALCMode >= 2, the main diffuse path uses vanilla DALC instead. This makes these helpers ignore the DALC + Sky mode.
VolumetricFogLightScatteringCS.hlslcallsGetIBLColorOccluded, so volumetric fog can use a different environment source from the main diffuse path. The new setting is documented to affect the environment/DALC contribution. (raw.githubusercontent.com)Select the same DALC or environment source in both helpers. Use a shared helper, or pass the caller's vanilla DALC value into these functions.
Proposed direction
float3 GetIBLColor(float3 rayDir) { - return GetEnvIBLColor(rayDir) + GetSkyIBLColor(rayDir); + float3 envColor = SharedData::iblSettings.DALCMode >= 2 ? + Color::IrradianceToLinear(Color::Ambient(max(0, SharedData::GetAmbient(rayDir))) * + SharedData::iblSettings.DALCAmount) : + GetEnvIBLColor(rayDir); + return envColor + GetSkyIBLColor(rayDir); }🤖 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 `@features/IBL/Shaders/IBL/IBL.hlsli` around lines 152 - 162, Update GetIBLColor and GetIBLColorOccluded to select the environment or vanilla DALC source consistently with the main diffuse path when SharedData::iblSettings.DALCMode is at least 2. Reuse a shared source-selection helper or accept the caller’s vanilla DALC value, while preserving each helper’s existing sky contribution and visibility handling.
119-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the irradiance color-space conversions.
GetDiffuseIBLandGetDiffuseIBLOccludedassign the DALC value directly tolinEnvand return the sum without the irradiance transfer functions.GetFogIBLColormakes the same raw DALC addition. This mixes source-space and linear-space values and changes IBL and fog brightness.The preceding implementation used
Color::IrradianceToLinearfor DALC andColor::IrradianceToGammaafter diffuse accumulation. (raw.githubusercontent.com)Proposed fix
- linEnv = vanillaDALC * SharedData::iblSettings.DALCAmount; + linEnv = Color::IrradianceToLinear(vanillaDALC * SharedData::iblSettings.DALCAmount); ... - return linEnv + linSky; + return Color::IrradianceToGamma(linEnv + linSky);Apply the same changes in
GetDiffuseIBLOccluded, and restore the conversion inGetFogIBLColor:- iblColor = dalc0 * SharedData::iblSettings.DALCAmount + GetSkyIBLColor(float3(0, 0, 0)); + iblColor = Color::IrradianceToLinear(dalc0 * SharedData::iblSettings.DALCAmount) + GetSkyIBLColor(float3(0, 0, 0));Also applies to: 138-145, 177-177
🤖 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 `@features/IBL/Shaders/IBL/IBL.hlsli` around lines 119 - 129, Restore the irradiance color-space conversions in GetDiffuseIBL, GetDiffuseIBLOccluded, and GetFogIBLColor: convert DALC values with Color::IrradianceToLinear before adding them to linear environment lighting, and apply Color::IrradianceToGamma to the accumulated diffuse result before returning it. Preserve the existing occlusion, sky, and fog calculations while ensuring all DALC contributions use the same transfer functions as the preceding implementation.
🧹 Nitpick comments (1)
features/IBL/Shaders/IBL/IBL.hlsli (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the PR metadata to match the repository convention.
The current title is
feat: new SSGI and SSR + NRD. (github.com)Use a title such as
feat(rendering): add nrd, ssgi, and ssr. If an issue tracks this feature, addImplements #<issue>orFixes #<issue>to the PR description.As per path instructions: use
type(scope): descriptiontitles and add issue references when applicable.🤖 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 `@features/IBL/Shaders/IBL/IBL.hlsli` around lines 13 - 14, Update the pull request metadata rather than the shader code: rename the PR title to follow the type(scope): description convention, such as “feat(rendering): add nrd, ssgi, and ssr,” and add an “Implements #<issue>” or “Fixes #<issue>” reference to the description when a tracking issue exists.Source: Path instructions
🤖 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.
Outside diff comments:
In `@features/IBL/Shaders/IBL/IBL.hlsli`:
- Around line 152-162: Update GetIBLColor and GetIBLColorOccluded to select the
environment or vanilla DALC source consistently with the main diffuse path when
SharedData::iblSettings.DALCMode is at least 2. Reuse a shared source-selection
helper or accept the caller’s vanilla DALC value, while preserving each helper’s
existing sky contribution and visibility handling.
- Around line 119-129: Restore the irradiance color-space conversions in
GetDiffuseIBL, GetDiffuseIBLOccluded, and GetFogIBLColor: convert DALC values
with Color::IrradianceToLinear before adding them to linear environment
lighting, and apply Color::IrradianceToGamma to the accumulated diffuse result
before returning it. Preserve the existing occlusion, sky, and fog calculations
while ensuring all DALC contributions use the same transfer functions as the
preceding implementation.
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 758-761: Update the SSGI_HALF dispatch path so texNRDInput is
cleared before use and generated samples are written to their original
checkerboard positions rather than the compact left-half coordinates. Keep the
full-resolution path unchanged, and ensure stale NRD input pixels cannot persist
between half-resolution dispatches.
---
Nitpick comments:
In `@features/IBL/Shaders/IBL/IBL.hlsli`:
- Around line 13-14: Update the pull request metadata rather than the shader
code: rename the PR title to follow the type(scope): description convention,
such as “feat(rendering): add nrd, ssgi, and ssr,” and add an “Implements
#<issue>” or “Fixes #<issue>” reference to the description when a tracking issue
exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3e93426-991a-42b9-bce1-304dd76cfd87
📒 Files selected for processing (15)
CMakeLists.txtfeatures/IBL/Shaders/Features/ImageBasedLighting.inifeatures/IBL/Shaders/IBL/IBL.hlslipackage/SKSE/Plugins/CommunityShaders/Translations/en.jsonpackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/Water.hlslsrc/Deferred.cppsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/IBL.cppsrc/Features/ScreenSpaceGI.cppsrc/Globals.cppsrc/Globals.h
🚧 Files skipped from review as they are similar to previous changes (13)
- features/IBL/Shaders/Features/ImageBasedLighting.ini
- src/Globals.cpp
- src/FeatureBuffer.cpp
- package/Shaders/Lighting.hlsl
- src/Features/IBL.cpp
- package/Shaders/Common/SharedData.hlsli
- package/Shaders/Water.hlsl
- package/SKSE/Plugins/CommunityShaders/Translations/en.json
- src/Globals.h
- package/Shaders/RunGrass.hlsl
- src/Feature.cpp
- CMakeLists.txt
- src/Deferred.cpp
4 participants
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes