fix(shadows-temporal-jitter): use stable hashing#1655
Conversation
📝 WalkthroughWalkthroughAdds frame-stable shadow filtering to reduce temporal jitter by introducing hash-based rotation angles and stable frame offsets instead of per-frame noise values, while enhancing cascade selection logic for shadow masking operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
package/Shaders/Utility.hlsl (3)
399-400: Casting negative floats touintmay produce inconsistent results.When
positionMScomponents are negative,(uint)(negative_float)behavior can vary. Consider usingasuint(asint(...))or adding an offset to ensure positive values before casting. Additionally, the weighting coefficients (1000, 100, 10) may create hash collisions along certain spatial diagonals.🔎 Suggested fix for robustness
- uint stableOffset = (uint)(positionMS.x * 1000.0) + (uint)(positionMS.y * 100.0) + (uint)(positionMS.z * 10.0); - uint frameOffset = stableOffset * sampleCount; // Use stable to minimize temporal jitter + // Use bit reinterpretation for consistent hashing across negative coordinates + int3 quantized = int3(floor(positionMS * 16.0)); + uint stableOffset = asuint(quantized.x) ^ (asuint(quantized.y) * 0x27d4eb2dU) ^ (asuint(quantized.z) * 0x9e3779b9U); + uint frameOffset = stableOffset % sampleCount; // Use stable offset to minimize temporal jitter
650-654: LGTM - Stable angle integration looks correct.The
stableAngleproperly replaces frame-dependent rotation, and usinglightIndex=0is appropriate for directional shadow masks. Theprecisequalifier helps ensure consistent results.One minor note: the
noisevariable (line 650) is still computed but appears unused in this code path after the change. It's only transformed (line 656) but the original noise-based rotation is now replaced bystableAngle. Consider whethernoisecan be removed from this path to avoid unnecessary computation.
294-296: Consider adding issue reference per coding guidelines.The PR description references issue #1479. As per coding guidelines, consider updating the PR description to include
Fixes #1479orCloses #1479so the issue is automatically linked and closed when merged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Utility.hlsl
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/Utility.hlsl
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Utility.hlsl
🧠 Learnings (6)
📓 Common learnings
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: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Utility.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Utility.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Utility.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Utility.hlsl
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
package/Shaders/Utility.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
package/Shaders/Utility.hlsl (1)
641-648: LGTM!The cascade index computation correctly mirrors the existing cascade selection logic and is necessary to compute
stableAnglewith the appropriate cascade context before the shadow map lookups.
| float StableShadowAngle(float3 worldPos, uint lightIndex, uint cascadeIndex) | ||
| { | ||
| // Quantize world position to ~6cm precision (conservative to eliminate jitter) | ||
| int3 quantizedPos = int3(floor(worldPos * 16.0)); | ||
| uint hash = StableShadowHash(asuint(quantizedPos.x), asuint(quantizedPos.y), lightIndex, cascadeIndex); | ||
| // Map hash to [0, TAU) for rotation angle | ||
| return Math::TAU * ((hash & 0x00FFFFFFu) / 16777216.0f); | ||
| } |
There was a problem hiding this comment.
The z-coordinate of quantizedPos is not used in the hash.
quantizedPos.z is computed but never passed to StableShadowHash. The hash only uses x, y, lightIndex, and cascadeIndex. This means objects at different vertical positions but same x/y will share the same rotation angle, which may cause visible banding on vertical surfaces or in scenes with significant vertical depth variation.
If this is intentional (e.g., to maintain consistency along vertical axes), consider adding a brief comment. Otherwise:
🔎 Suggested fix to include z in the hash
float StableShadowAngle(float3 worldPos, uint lightIndex, uint cascadeIndex)
{
// Quantize world position to ~6cm precision (conservative to eliminate jitter)
int3 quantizedPos = int3(floor(worldPos * 16.0));
- uint hash = StableShadowHash(asuint(quantizedPos.x), asuint(quantizedPos.y), lightIndex, cascadeIndex);
+ uint hash = StableShadowHash(
+ asuint(quantizedPos.x) ^ asuint(quantizedPos.z),
+ asuint(quantizedPos.y),
+ lightIndex,
+ cascadeIndex);
// Map hash to [0, TAU) for rotation angle
return Math::TAU * ((hash & 0x00FFFFFFu) / 16777216.0f);
}📝 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.
| float StableShadowAngle(float3 worldPos, uint lightIndex, uint cascadeIndex) | |
| { | |
| // Quantize world position to ~6cm precision (conservative to eliminate jitter) | |
| int3 quantizedPos = int3(floor(worldPos * 16.0)); | |
| uint hash = StableShadowHash(asuint(quantizedPos.x), asuint(quantizedPos.y), lightIndex, cascadeIndex); | |
| // Map hash to [0, TAU) for rotation angle | |
| return Math::TAU * ((hash & 0x00FFFFFFu) / 16777216.0f); | |
| } | |
| float StableShadowAngle(float3 worldPos, uint lightIndex, uint cascadeIndex) | |
| { | |
| // Quantize world position to ~6cm precision (conservative to eliminate jitter) | |
| int3 quantizedPos = int3(floor(worldPos * 16.0)); | |
| uint hash = StableShadowHash( | |
| asuint(quantizedPos.x) ^ asuint(quantizedPos.z), | |
| asuint(quantizedPos.y), | |
| lightIndex, | |
| cascadeIndex); | |
| // Map hash to [0, TAU) for rotation angle | |
| return Math::TAU * ((hash & 0x00FFFFFFu) / 16777216.0f); | |
| } |
🤖 Prompt for AI Agents
In package/Shaders/Utility.hlsl around lines 306 to 313, quantizedPos.z is
computed but not used in the StableShadowHash call, causing identical rotation
for different vertical positions; either include the z component in the hash
call or document that omitting z is intentional. To fix, change the
StableShadowHash invocation to pass asuint(quantizedPos.z) as an additional
parameter (and update the StableShadowHash signature/uses accordingly), or add a
clear comment explaining the deliberate omission and rationale if vertical
invariance is required.
NickStefan
changed the title
|
✅ A pre-release build is available for this PR: |
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
Temporally denoising shadows is a common optimisation technique. Removing it but not applying an alternative denoise step is not a fix.
|
After looking deeper into this, I agree that this isn't actually an improvement for all platforms since it doesn't fully replace the denoise with an alternative. The resulting shadows of this PR are actually about the same as just replacing We could apply this automatically, if we're willing to detect proton during shader compilation. |
2 participants
End-User Problem
"Shadow Jittering" on linux. As reported by multiple users (videos and photos in the report) #1479. Fixes #1479.
Manifests in multiple ways:
Shadows added by mods like "Window Shadows RT" do not cause these problems. This might be because the problem is with specific shadow filtering functions rather than all shadow related code.
Technical Problem
The main shadow filtering functions in
package/Shaders/Utility.hlslleverageSharedData::FrameCountfor noise, spatial randomization, and sampling. This introduces "temporal inconsistency", where shadow positioning varies wildly frame-to-frame, since the shadow calculations aren't frame independent.Solution
Replace frame-dependent randomization with world-position based hashing for shadow filtering. The
StableShadowAnglefunction quantizes world position (~6cm precision) and uses stable hashing to generate consistent rotation angles, eliminating temporal jitter while preserving spatial variation.This should provide technical benefits for all platforms: more stable shadows and reduced artifacts at lower shadow mask resolutions (like 1024), while maintaining the same quality of stochastic shadow filtering on all platforms.
This PR is inspired by a patch made by @SulfurNitride, in the original bug report, but it's not a copy/paste. I removed parts of their fix that I determined were unnecessary and rewrote several other parts. Still, thank you @SulfurNitride for clearly identifying the problem!
Testing
I've tested this a bunch on my machine, Nobara Linux with a 6700XT, and it works great in every shadow situation I could find. I don't have a windows machine to test further on. Nor do I have a great video capture setup. Maybe we can get more testers via the discord.
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.