feat(llf): restore contact shadows with VR-aware noise#36
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds contact-shadow rendering to enhance deferred lighting in the Light Limit Fix feature. It introduces shader helper functions for ray-marched contact shadows, extends CPU and GPU data structures to carry an ChangesContact Shadows Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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.
Pull request overview
Restores Light Limit Fix contact shadows for clustered/strict point lights and adds a VR-aware noise coordinate helper to reduce stereo flicker, wiring the result into the deferred lighting evaluation path via the shared feature constant buffer.
Changes:
- Reintroduces an
EnableContactShadowstoggle (C++ settings + shared feature cbuffer field) and UI control. - Restores the
ContactShadows()helper in LLF shader code and activates the call site inLighting.hlsl(deferred-only), including hoisted per-pixel noise. - Bumps LLF feature version to reflect the updated feature cbuffer layout.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Features/LightLimitFix.h | Adds EnableContactShadows to per-frame feature data and settings. |
| src/Features/LightLimitFix.cpp | Adds UI checkbox + writes EnableContactShadows into the feature constant buffer payload. |
| package/Shaders/Lighting.hlsl | Computes per-pixel contact-shadow noise (deferred) and multiplies contact shadow into point light shadowing. |
| package/Shaders/Common/SharedData.hlsli | Extends LightLimitFixSettings with EnableContactShadows while keeping packing/alignment. |
| features/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli | Restores ContactShadows() and adds GetContactShadowNoiseCoord() for VR noise coordination. |
| features/Light Limit Fix/Shaders/Features/LightLimitFix.ini | Version bump to 3-1-0. |
|
✅ A pre-release build is available for this PR: |
alandtse
force-pushed
the
feat/llf-contact-shadows-restore
branch
from
5e9d661 to
380a568
Compare
alandtse
force-pushed
the
feat/llf-contact-shadows-restore
branch
from
380a568 to
58d4fcb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
features/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli (1)
50-70: Add targeted shader validation for LLF contact-shadow permutations.Given this helper is shared through includes, run targeted
hlslkit-compilepermutations forLIGHT_LIMIT_FIX + DEFERREDwithVRon/off, plus hlslkit buffer scanning to catch register overlap early.As per coding guidelines, "Highlight GPU register conflicts and recommend hlslkit buffer scanning for shader development" and "Use targeted hlslkit-compile shader validation for specific features during development rather than full validation".
🤖 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/Light` Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli around lines 50 - 70, The shared helper GetContactShadowNoiseCoord can introduce GPU register/buffer conflicts when compiled across permutations; run targeted hlslkit-compile validation for the LIGHT_LIMIT_FIX + DEFERRED permutations with VR defined and VR undefined to ensure the two branches compile correctly, and run hlslkit's buffer/register scanner on the generated HLSL for these permutations to catch register overlaps early; update CI/dev docs or the shader build step to run these targeted checks rather than full-suite validation so LLF contact-shadow permutations (GetContactShadowNoiseCoord, VR macro paths) are validated during development.package/Shaders/Lighting.hlsl (1)
2746-2748: ⚡ Quick winAvoid full per-light position transform for VS contact-shadow direction.
At Line 2746-Line 2747, you can reuse
lightDirection(already computed at Line 2704) and transform it as a direction (w=0) to reduce ALU in this hot loop.As per coding guidelines, "Always consider GPU workload and user experience when implementing graphics features".♻️ Suggested micro-optimization
- float3 lightPositionVS = mul(FrameBuffer::CameraView[eyeIndex], float4(light.positionWS[eyeIndex].xyz, 1)).xyz; - float3 normalizedLightDirectionVS = normalize(lightPositionVS - viewPosition.xyz); + float3 lightDirectionVS = mul((float3x3)FrameBuffer::CameraView[eyeIndex], lightDirection); + float3 normalizedLightDirectionVS = normalize(lightDirectionVS);🤖 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 2746 - 2748, The code currently transforms the light world-position to view space then subtracts viewPosition to get a per-light direction (lightPositionVS - viewPosition), which is ALU-heavy; instead reuse the already-computed world-space light direction symbol lightDirection (from earlier around Line 2704) and transform it as a direction by using mul(FrameBuffer::CameraView[eyeIndex], float4(lightDirection, 0)).xyz, then normalize that result and pass it into LightLimitFix::ContactShadows (replace lightPositionVS/viewPosition subtraction with normalized transformed direction), keeping contactShadowNoise, contactShadowSteps and eyeIndex 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 `@features/Light` Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli:
- Around line 50-70: The shared helper GetContactShadowNoiseCoord can introduce
GPU register/buffer conflicts when compiled across permutations; run targeted
hlslkit-compile validation for the LIGHT_LIMIT_FIX + DEFERRED permutations with
VR defined and VR undefined to ensure the two branches compile correctly, and
run hlslkit's buffer/register scanner on the generated HLSL for these
permutations to catch register overlaps early; update CI/dev docs or the shader
build step to run these targeted checks rather than full-suite validation so LLF
contact-shadow permutations (GetContactShadowNoiseCoord, VR macro paths) are
validated during development.
In `@package/Shaders/Lighting.hlsl`:
- Around line 2746-2748: The code currently transforms the light world-position
to view space then subtracts viewPosition to get a per-light direction
(lightPositionVS - viewPosition), which is ALU-heavy; instead reuse the
already-computed world-space light direction symbol lightDirection (from earlier
around Line 2704) and transform it as a direction by using
mul(FrameBuffer::CameraView[eyeIndex], float4(lightDirection, 0)).xyz, then
normalize that result and pass it into LightLimitFix::ContactShadows (replace
lightPositionVS/viewPosition subtraction with normalized transformed direction),
keeping contactShadowNoise, contactShadowSteps and eyeIndex unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f03a640-ee29-4b45-aadd-a4e164109b78
📒 Files selected for processing (6)
features/Light Limit Fix/Shaders/Features/LightLimitFix.inifeatures/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.h
alandtse
force-pushed
the
feat/llf-contact-shadows-restore
branch
from
58d4fcb to
8d0e81d
Compare
alandtse
force-pushed
the
feat/llf-contact-shadows-restore
branch
from
8d0e81d to
58d4fcb
Compare
alandtse
force-pushed
the
feat/llf-contact-shadows-restore
branch
from
58d4fcb to
6ac678a
Compare
Reintroduce Light Limit Fix contact shadows for clustered point lights, reversing the maintainability-driven removal in 347cb55. That removal cited no defect, and the shader call site was already commented out at the time of deletion, so this is reviving a dormant feature rather than introducing new behaviour. Adapted from the original implementation: - LightLimitFix::ContactShadows() raymarch helper restored verbatim - EnableContactShadows toggle re-added to LightLimitFix Settings and the SharedData::LightLimitFixSettings cbuffer - Shadows section restored in the LLF settings UI (session-only, matching the current LLF settings-persistence stance) - Wired into Lighting.hlsl as a multiplier on pointLightShadow, which the upstream DirectContext/EvaluateLighting refactor funnels into both the PBR and legacy paths uniformly VR jitter stabilization (concept from ParticleTroned's 8e615ea): - Hoist the contact-shadow noise sample out of the per-light loop and compute it once per pixel from (uv * eye-buffer dim) in VR or raw pixel position in flat. Using raw pixel coords in VR makes each eye sample a different noise pattern at the same world position, which reads as flicker on contact-shadow recipients. - New helpers LightLimitFix::GetContactShadowScreenDim() and GetContactShadowNoiseCoord() centralise the stereo-aware sampling. Bumps Light Limit Fix feature version to 3-1-0 (minor: new persisted cbuffer field, new user-facing toggle). Original contact-shadow implementation by jiayev (merged via #9 prior to the 347cb55 removal). VR jitter analysis and fix from ParticleTroned's fork. Co-Authored-By: jiayev <jiayev1@gmail.com> Co-Authored-By: ParticleTroned <248299730+ParticleTroned@users.noreply.github.com>
alandtse
force-pushed
the
feat/llf-contact-shadows-restore
branch
from
6ac678a to
6a78cef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restores Light Limit Fix contact shadows for all point lights (strict and clustered, excluding simple emissive markers), reversing the maintainability-only removal in 347cb55. Adds a stereo-aware noise sample so VR doesn't flicker.
Why now
PR community-shaders#1495 (the upstream removal) cited no defect — no human discussion, no linked issue, no rationale beyond CodeRabbit's auto-summary. The shader call site in
Lighting.hlslwas already//-commented out at the time of deletion, so this is reviving a dormant feature, not introducing new behaviour.The ParticleTroned fork (
cs-1.5-PL-VR-unleashed) has been carrying a restored implementation for months. This PR pulls just the contact-shadow piece — the fork's elaborate budgeted/cluster-aware variant requires Particle Lights to be meaningful and is deferred to a follow-up.Changes
LightLimitFix.hlsli— restoreIsSaturated()andContactShadows()raymarch helper verbatim. AddGetContactShadowNoiseCoord()for stereo-aware noise sampling (one#if defined(VR)branch, leans onFrameBuffer::ViewToUV's per-eye return value).Lighting.hlsl— hoist contact-shadow noise out of the per-light loop, compute once per pixel. Uncomment + activate theEnableContactShadowscall site (gated toDEFERRED). MultiplycontactShadowintopointLightShadowso it flows through the newDirectContext/EvaluateLightingpath uniformly for both PBR and legacy.SharedData.hlsli— re-adduint EnableContactShadowstoLightLimitFixSettingscbuffer (replaces onefloat2 pad0slot withuint EnableContactShadows; float pad0).LightLimitFix.h— re-addbool EnableContactShadows = falsetoSettings.LightLimitFix.cpp— restore Shadows section inDrawSettings()using current upstream's///////separator style. Did not restore NLOHMANN persistence — current LLF is session-only per chore(llf): remove LightsVisualisationMode settings loading community-shaders/skyrim-community-shaders#1834, kept that stance.LightLimitFix.ini— bump feature version3-0-3 → 3-1-0(minor: new persisted cbuffer field + user toggle).VR jitter fix
Concept lifted from
ParticleTroned/skyrim-community-shaders@8e615ea fix(contact-shadows): stabilize VR eye jitter, simplified to one helper / one ifdef.In VR, using raw
input.Position.xyfor the noise sample makes each eye hash a different value at the same world position. The helper switches toscreenUV * BufferDim.xyin VR —screenUVfromFrameBuffer::ViewToUVis already per-eye viaCameraProj[eye], so both eyes hash the same input and the noise stays stereo-stable. Flat keepsinput.Position.xyto match the pre-removal implementation byte-for-byte.What's not in this PR
InsertContactShadowCandidate,ContactShadowFlags, separatecontactShadowGrid). Requires Particle Lights to be meaningful; deferred to a follow-up after PL lands.Test plan
ALLpreset (verified locally;CommunityShaders.dllproduced)feedback_shader_validation_scope.mdSharedData::extendedMaterialSettings.EnableShadows)Attribution
Both are credited in the commit trailers.
Summary by CodeRabbit
Release Notes
New Features
Chores