fix(terrain-blending): transparency of landscapes#2082
Conversation
📝 WalkthroughWalkthroughModified the VR stereo reprojection depth correction payload in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
No actionable suggestions for changed features. |
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/Lighting.hlsl (1)
3203-3211:⚠️ Potential issue | 🔴 CriticalVR stereo reprojection compatibility issue: non-POM surfaces may receive unintended depth correction.
Per
StereoBlendCS.hlsl(lines 151, 222), non-POM pixels are expected to store0.0in ReflectanceTexture.w to distinguish them from POM-displaced surfaces using the> 1e-2threshold.With this change, surfaces without parallax displacement now store
psout.Diffuse.winstead. For opaque surfaces where alpha ≈ 1.0, this exceeds the threshold, causing StereoBlendCS to apply the depth correction formula(0.5 - pomOffset) * POMDepthScale—pushing non-POM geometry depth farther away and introducing visual artifacts in stereo reprojection.This breaks the documented contract that non-POM pixels store 0.0. Ensure VR stereo rendering is tested with opaque/alpha-blended terrain to confirm the change does not degrade depth accuracy in stereo modes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 3203 - 3211, The VR branch currently writes psout.Reflectance.w = psout.Diffuse.w for non-POM pixels which breaks the StereoBlendCS contract expecting 0.0 for non-POM; change the assignment so psout.Reflectance = float4(indirectLobeWeights.specular, (pixelOffset > 1e-2) ? saturate(pixelOffset) : 0.0) (or equivalent test using pixelOffset) so only POM-displaced pixels store a positive w and non-POM pixels store 0.0; keep the check in the VR block and ensure compatibility with StereoBlendCS's >1e-2 threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 3203-3211: The VR branch currently writes psout.Reflectance.w =
psout.Diffuse.w for non-POM pixels which breaks the StereoBlendCS contract
expecting 0.0 for non-POM; change the assignment so psout.Reflectance =
float4(indirectLobeWeights.specular, (pixelOffset > 1e-2) ?
saturate(pixelOffset) : 0.0) (or equivalent test using pixelOffset) so only
POM-displaced pixels store a positive w and non-POM pixels store 0.0; keep the
check in the VR block and ensure compatibility with StereoBlendCS's >1e-2
threshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ebdb716-bd13-45ae-a889-5fca162705f3
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl
|
✅ A pre-release build is available for this PR: |
|
@coderabbitai can you identify the history for the changed code before and determine what pr changed it and why? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Unrecoverable error: Request timed out after 300000ms (requestId=ed310f8f-3677-440d-ad71-f468f0ed588a) Let me try a lighter approach and also search GitHub for the relevant history. 🏁 Script executed: Length of output: 1071 🌐 Web query: 💡 Result: In
Also, the literal string If you’re chasing “StereoBlend” in a VR PR, PR #1507 (“feat(VR): enable upscaling”) explicitly removed VR stereo-eye sampling/blending from Citations:
🏁 Script executed: Length of output: 5167 🏁 Script executed: Length of output: 936
Origin: PR
|
|
@coderabbitai VR is also reporting that terrain blending is broken where the landscape is partially transparent. Are there any other locations where #2082 may have caused the issue given the info from this PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17623
The Problem: Sentinel Value Collision
// Eye 1 reprojection path:
float pomOffset = ReflectanceTexture[r.otherPx].w;
if (pomOffset > 1e-2) {
// apply POM depth correction to reprojection
}
// FULL_BLEND path:
float pomOffsetFB = ReflectanceTexture[dtid].w;
if (pomOffsetFB > 1e-2 && POMDepthScale > 0) {
// apply POM depth correction
}The original PR What PR
|
| Branch | Needs | Why |
|---|---|---|
#else (SE/AE) |
psout.Diffuse.w |
Terrain alpha must flow through for downstream passes |
#if (VR) |
0.0 sentinel for non-POM |
StereoBlendCS uses > 1e-2 to gate POM depth correction |
PR #2082 correctly fixed the #else branch but also changed the VR branch fallback, breaking the StereoBlendCS sentinel contract.
Proposed Fix Direction
The VR block needs to keep 0.0 as the no-POM sentinel. One option is to narrow the #if guard to exclude LANDSCAPE from the VR POM path (since terrain doesn't use POM parallax displacement):
// Only enter VR POM path for actual POM-capable shader types, not LANDSCAPE
`#if` defined(VR) && (defined(EMAT) || defined(TRUE_PBR)) && \
(defined(PARALLAX) || defined(EMAT_ENVMAP)) // removed LANDSCAPE and bare TRUE_PBR
psout.Reflectance = float4(indirectLobeWeights.specular,
(pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0);
`#else`
psout.Reflectance = float4(indirectLobeWeights.specular, psout.Diffuse.w);
`#endif`This way:
- VR + PARALLAX/EMAT_ENVMAP: packs POM offset as sentinel (StereoBlendCS works correctly)
- VR + LANDSCAPE: falls to
#else, storespsout.Diffuse.w(terrain alpha flows through, andpsout.Diffuse.wbeing non-zero won't break VR stereo as long as StereoBlendCS doesn't read it — but it will, which means this approach still has the same problem)
The deeper fix may be that the VR + LANDSCAPE case needs Reflectance.w = 0.0 explicitly to preserve the StereoBlendCS sentinel, accepting that terrain alpha doesn't flow through Reflectance.w in VR (if that's acceptable for the VR terrain blending pipeline). That would mean a three-way split:
`#if` defined(VR) && (defined(EMAT) || defined(TRUE_PBR)) && \
(defined(PARALLAX) || defined(LANDSCAPE) || defined(EMAT_ENVMAP) || defined(TRUE_PBR))
# if defined(LANDSCAPE)
// VR + LANDSCAPE: no POM, must keep 0.0 sentinel for StereoBlendCS
psout.Reflectance = float4(indirectLobeWeights.specular, 0.0);
# else
// VR + POM-capable: pack parallax offset
psout.Reflectance = float4(indirectLobeWeights.specular,
(pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0);
# endif
`#else`
// SE/AE: terrain alpha flows through normally
psout.Reflectance = float4(indirectLobeWeights.specular, psout.Diffuse.w);
`#endif`The key question for @InTheBottle is: does VR terrain blending (outside of StereoBlendCS) depend on Reflectance.w = psout.Diffuse.w? If not, 0.0 in VR+LANDSCAPE is safe and the above three-way split is the fix. If it does, then a different channel or approach is needed in VR to carry terrain alpha without colliding with the POM sentinel.
🧠 Learnings used
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:11.490Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:31.383Z
Learning: In `src/Features/VR/StereoBlend.cpp` (doodlum/skyrim-community-shaders), debug modes 4/5 ("Overwrite", "Overwrite Eye1") in the `StereoBlendDebugMode` selector are NOT separate shader variants. They use the same `stereoBlendOverwriteCS` shader with different cbuffer parameters. When `vrStereoOptActive` is true, `stereoBlendOverwriteCS` is always selected unconditionally and `StereoBlendDebugMode` values 4/5 do not map to any distinct code path.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:32.809Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1966
File: package/Shaders/ISVolumetricLightingGenerateCS.hlsl:0-0
Timestamp: 2026-03-11T08:05:10.801Z
Learning: In doodlum/skyrim-community-shaders, the `sqrt()` applied to `CloudShadows::GetCloudShadowMult()` in `package/Shaders/ISVolumetricLightingGenerateCS.hlsl` is intentional. The maintainer (Dlizzio) confirmed it is used to increase cloud shadow strength at harsh angles for volumetric lighting, and should not be removed or replaced with a direct linear multiply.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 2084
File: src/ShaderCache.cpp:1646-1649
Timestamp: 2026-04-10T07:34:24.381Z
Learning: In `src/ShaderCache.cpp` (doodlum/skyrim-community-shaders PR `#2084`), `GetImagespaceShaderDescriptor` intentionally returns `false` for VR when a shader is not reverse-engineered. Returning `false` causes `ResolveImageSpaceDescriptor` to return `false`, which makes all callers (GetVertexShader, GetPixelShader, GetComputeShader, CompileShader) skip custom shader handling and defer to the vanilla shader. This is the correct pattern for un-RE'd imagespace shaders on VR — do not flag it as "skipping all custom VR handling" or suggest gating on HDR feature state.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1941
File: package/Shaders/Common/ShadowSampling.hlsli:160-163
Timestamp: 2026-04-03T01:48:34.867Z
Learning: In `package/Shaders/Common/ShadowSampling.hlsli` (doodlum/skyrim-community-shaders PR `#1941`), the directional shadow fade in `GetLightingShadow` is intentionally computed as `fadeStart = shadow.EndSplitDistances.y * (1.0 - FadeWindow)` where `FadeWindow = 0.1`. This anchors the fade to the last 10% of the absolute far-cascade distance (`EndSplitDistances.y`), not to the cascade span. The narrow window is deliberate so shadows dissolve only at the hard far-cutoff without prematurely fading across the whole cascade range. Do not suggest using `lerp(StartSplitDistances.y, EndSplitDistances.y, 1.0 - FadeWindow)` as a fix.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:49.633Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:107-115
Timestamp: 2026-03-14T08:35:02.326Z
Learning: In `features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli`, the pattern of scaling `directionalAmbientColorSpecular` by `skylightingSpecular` (via `dalcScaled`) before the final `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)` is intentional pre-existing behavior preserved from the original implementation. Do not flag this as double-attenuation; the combined attenuation is by design.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:22.461Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
closes #2080
Addresses the issue seen here. Confirmed fixed in flatrim, will likely need testing in VR.
Summary by CodeRabbit
Release Notes