feat(llf): expose contact-shadow settings#43
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR extends the light-limiting shader system's contact-shadow feature with perspective-correct ray marching and runtime-tunable parameters (steps, distance, stride, thickness, depth fade, min intensity), wires them through GPU constant buffers and deferred lighting, and bumps the feature version to 3-2-0. ChangesContact Shadow Parameter Tuning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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
This PR extends the Light Limit Fix contact-shadow feature by exposing the previously hardcoded raymarch tuning constants as user-adjustable settings, and adds a per-light intensity threshold to skip contact-shadow work for very weak lights (reducing cost in dense clustered-light scenes). It also updates the shared cbuffer layout and bumps the feature version to reflect the new settings fields.
Changes:
- Add new LLF UI settings for contact-shadow tuning (steps/distance/stride/thickness/fade/min-intensity) and send them through the per-frame feature cbuffer.
- Update shaders to consume the new cbuffer fields and early-out contact shadows for lights below
MinIntensity. - Bump
LightLimitFixfeature version to3-2-0.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Features/LightLimitFix.h | Adds new contact-shadow tuning fields to LLF settings and per-frame buffer struct (with updated padding). |
| src/Features/LightLimitFix.cpp | Adds ImGui controls for the new settings and populates the per-frame buffer values. |
| package/Shaders/Lighting.hlsl | Uses new cbuffer parameters for step computation and adds intensity-based skip for contact-shadow raymarch. |
| package/Shaders/Common/SharedData.hlsli | Extends LightLimitFixSettings in FeatureData cbuffer and adjusts padding for 16-byte alignment. |
| features/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli | Replaces magic numbers with cbuffer-driven values and introduces a named constant for the first-person depth gate. |
| features/Light Limit Fix/Shaders/Features/LightLimitFix.ini | Version bump to 3-2-0. |
|
✅ A pre-release build is available for this PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@package/Shaders/Lighting.hlsl`:
- Around line 2745-2753: The MinIntensity check is currently applied to all
non-simple lights and wrongly filters strict lights; update the if condition so
the intensityMultiplier >
SharedData::lightLimitFixSettings.ContactShadowMinIntensity test is only
enforced for clustered lights. Concretely, modify the branch in the
contact-shadow gate around contactShadowSteps / light.lightFlags /
intensityMultiplier (the block using contactShadowSteps, light.lightFlags,
LightLimitFix::LightFlags::Simple, shadowComponent, lightAngle,
intensityMultiplier) so the intensity comparison is guarded by a clustered-light
flag check (e.g. only evaluate intensityMultiplier > ContactShadowMinIntensity
when (light.lightFlags & LightLimitFix::LightFlags::Clustered) is set), leaving
other gates unchanged.
In `@src/Features/LightLimitFix.cpp`:
- Around line 112-117: Sanitize the contact-shadow fields on perFrame before
writing the constant buffer by clamping each value to safe ranges: ensure
ContactShadowMaxSteps is an integer >=1 and reasonably bounded (e.g., <=256),
ContactShadowMaxDistance is >=0 and <= a scene/far-plane cap,
ContactShadowStride and ContactShadowThickness are >0 and bounded,
ContactShadowDepthFade is >=0, and ContactShadowMinIntensity is clamped to
[0,1]; implement these clamps immediately before the assignments to
perFrame.ContactShadowMaxSteps/ContactShadowMaxDistance/ContactShadowStride/ContactShadowThickness/ContactShadowDepthFade/ContactShadowMinIntensity
so malformed configs can't produce invalid shader math.
In `@src/Features/LightLimitFix.h`:
- Around line 95-109: PerFrame's layout can silently change; add a compile-time
size check after the struct to lock it to the expected GPU cbuffer size. After
STATIC_ASSERT_ALIGNAS_16(PerFrame); add a static_assert(sizeof(PerFrame) == 64,
"PerFrame size changed - update GPU cbuffer layout"); reference the PerFrame
struct and the existing STATIC_ASSERT_ALIGNAS_16(PerFrame) to locate where to
insert the static_assert.
🪄 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
Run ID: 5b62883a-730c-427b-a35e-94d52865d830
📒 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
added a commit
that referenced
this pull request
May 25, 2026
Address 11 actionable Copilot review comments on PR #43: - Lighting.hlsl contact-shadow gate: compute a separate normalized falloff `(1 - lightDist*invRadius)^2` clamped to [0,1] for the MinIntensity cutoff. The previous gate compared `intensityMultiplier`, which is path-dependent -- non-ISL returns the normalized quadratic falloff but ISL returns `InverseSquareLighting::GetAttenuation()` which can exceed 1 and isn't normalized. Comparing a 0..1 threshold against it meant different things in the two permutations. - Lighting.hlsl gate scope: limit the MinIntensity cutoff to CLUSTERED lights (lightIndex >= NumStrictLights). Strict lights are always raymarched regardless of falloff -- portal-strict lights are user-controlled and should preserve their contact contribution even when their attenuation is weak at the pixel. - LightLimitFix.cpp ContactShadowMaxSteps slider: switch from `SliderInt + (int*)cast` to `SliderScalar` with `ImGuiDataType_U32`. The cast violated strict aliasing (UB under MSVC + Clang) and could also misinterpret transient negative ImGui values before clamp. SliderScalar reads/writes the uint storage directly. - LightLimitFix.cpp Min Light Intensity tooltip: clarified to match the new shader behavior -- "CLUSTERED" capitalised, normalized falloff formula spelled out, strict-lights-always-march noted. - LightLimitFix.cpp GetCommonBufferData: clamp all six contact-shadow settings before they hit the constant buffer. The sliders enforce AlwaysClamp but malformed JSON, hand-edited configs, or schema migrations can deliver out-of-range values; clamping here lets the shader assume sane inputs. - LightLimitFix.h PerFrame: add `static_assert(sizeof(PerFrame) == 64)` so any CPU/GPU cbuffer layout drift fails at compile time. The existing STATIC_ASSERT_ALIGNAS_16 only enforces 16-byte alignment and multiple-of-16 size; a field shuffle that still lands on a 16-byte boundary would silently desync the shader-side cbuffer without it. One thread resolved without code changes: - Stride auto-scaling tooltip claim: the referenced "auto-scales linearly past ~100 units" text isn't present in the current code. Outdated against an earlier draft. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alandtse
changed the title
alandtse
pushed a commit
that referenced
this pull request
May 26, 2026
Two Copilot review findings on top of #43: 1. Lighting.hlsl computed (saturate(1 - d/r))^2 but documented and intended 1 - (saturate(d/r))^2. At d=0.5*r the previous code returned 0.25; the intended non-ISL intensityMultiplier formula returns 0.75. The cutoff curve was much steeper than designed, defeating the path- uniformity fix from 5569754. Swap to match the non-ISL formula. 2. LightLimitFix.cpp::GetCommonBufferData clamped with std::clamp, but std::clamp passes NaN through (all NaN comparisons are false), so a malformed config containing NaN would still poison the cbuffer. Add a sanitizeFloat lambda that rejects non-finite values explicitly before clamping, falling back to the lower bound on NaN/inf. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
Replace hardcoded contact-shadow constants (4 steps, 1024 max distance, 0.20/0.05 thickness/fade, 2.0 stride) with user-tunable settings exposed under a new "Contact Shadow Tuning" tree, and add a per-light intensity cutoff so weak clustered lights at their reach edge skip the raymarch. Group A (settings expansion): - New PerFrame fields: ContactShadowMaxSteps, ContactShadowMaxDistance, ContactShadowStride, ContactShadowThickness, ContactShadowDepthFade, ContactShadowMinIntensity. - Repack PerFrame / LightLimitFixSettings to a 64-byte (4-row) layout with three explicit float pads before ClusterSize so CPU and HLSL match. - Magic 16.5 first-person depth gate promoted to a named constant with a comment explaining the depth-range hack and that a viewmodel stencil pass would be more robust. Group B (per-light distance cutoff): - Contact-shadow branch now also requires intensityMultiplier > MinIntensity (default 0.25), skipping the matrix-multiply + raymarch for clustered lights that contribute too little at the pixel to produce a visible shadow. Typical interior cells have many lights at their reach edge; skipping them is the primary perf lever. Bump LightLimitFix.ini 3-1-0 -> 3-2-0. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
Replace the inner EnableContactShadows check with contactShadowSteps > 0: when steps fall to zero (feature off OR pixel past MaxDistance), the inner branch already does the right thing without a second cbuffer read, and we now also skip the matrix-multiply for pixels past the fade cutoff. Trim the new contact-shadow comments to one-line "why" notes per the project's commenting convention. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
Scale the per-step march length by view-space depth past a reference of 100 units so each step covers roughly constant screen-space distance regardless of how far the recipient is from camera. Without this, far surfaces under- sample badly (a 2-unit step is ~10 pixels at z=50 but <1 pixel at z=500). Inverse-scale the thickness/fade depth-delta band by the same factor so the shadow-onset window tracks the same screen-space extent at depth instead of the wider world-space depthDelta jumps caused by the longer stride. At/below reference depth (typical near-interior recipient distances) the behavior matches the prior code exactly. Beyond reference depth, distant shadows now actually reach into screen pixels they previously skipped. Step count and step count are unchanged, so perf is neutral. Quality lift is largest at mid-range distances (z = 200-1000) where the old code's fixed view-space stride produced essentially invisible shadow halos. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
Address 11 actionable Copilot review comments on PR #43: - Lighting.hlsl contact-shadow gate: compute a separate normalized falloff `(1 - lightDist*invRadius)^2` clamped to [0,1] for the MinIntensity cutoff. The previous gate compared `intensityMultiplier`, which is path-dependent -- non-ISL returns the normalized quadratic falloff but ISL returns `InverseSquareLighting::GetAttenuation()` which can exceed 1 and isn't normalized. Comparing a 0..1 threshold against it meant different things in the two permutations. - Lighting.hlsl gate scope: limit the MinIntensity cutoff to CLUSTERED lights (lightIndex >= NumStrictLights). Strict lights are always raymarched regardless of falloff -- portal-strict lights are user-controlled and should preserve their contact contribution even when their attenuation is weak at the pixel. - LightLimitFix.cpp ContactShadowMaxSteps slider: switch from `SliderInt + (int*)cast` to `SliderScalar` with `ImGuiDataType_U32`. The cast violated strict aliasing (UB under MSVC + Clang) and could also misinterpret transient negative ImGui values before clamp. SliderScalar reads/writes the uint storage directly. - LightLimitFix.cpp Min Light Intensity tooltip: clarified to match the new shader behavior -- "CLUSTERED" capitalised, normalized falloff formula spelled out, strict-lights-always-march noted. - LightLimitFix.cpp GetCommonBufferData: clamp all six contact-shadow settings before they hit the constant buffer. The sliders enforce AlwaysClamp but malformed JSON, hand-edited configs, or schema migrations can deliver out-of-range values; clamping here lets the shader assume sane inputs. - LightLimitFix.h PerFrame: add `static_assert(sizeof(PerFrame) == 64)` so any CPU/GPU cbuffer layout drift fails at compile time. The existing STATIC_ASSERT_ALIGNAS_16 only enforces 16-byte alignment and multiple-of-16 size; a field shuffle that still lands on a 16-byte boundary would silently desync the shader-side cbuffer without it. One thread resolved without code changes: - Stride auto-scaling tooltip claim: the referenced "auto-scales linearly past ~100 units" text isn't present in the current code. Outdated against an earlier draft. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous comment claimed dropping the 0.5 factor would give "half effective noise resolution"; in fact it over-samples IGN by ~2x in X, producing a higher-frequency noise pattern, not lower. Rewrite as a clear regression warning so the 0.5 doesn't get "cleaned up" later. Comment-only — no behavior change. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
Two Copilot review findings on top of #43: 1. Lighting.hlsl computed (saturate(1 - d/r))^2 but documented and intended 1 - (saturate(d/r))^2. At d=0.5*r the previous code returned 0.25; the intended non-ISL intensityMultiplier formula returns 0.75. The cutoff curve was much steeper than designed, defeating the path- uniformity fix from 5569754. Swap to match the non-ISL formula. 2. LightLimitFix.cpp::GetCommonBufferData clamped with std::clamp, but std::clamp passes NaN through (all NaN comparisons are false), so a malformed config containing NaN would still poison the cbuffer. Add a sanitizeFloat lambda that rejects non-finite values explicitly before clamping, falling back to the lower bound on NaN/inf. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
The non-ISL branch above already computes intensityMultiplier as `1 - (saturate(d/r))^2` -- the exact value we want for normalizedFalloff. Re-deriving it from saturate(lightDist * invRadius) was a duplicate computation of the same formula under a different variable name. Keep the separate computation only in the ISL branch, where GetAttenuation() isn't [0,1]-normalized and we genuinely need a distinct falloff term for the gate. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
The comment still referenced `intensityMultiplier` (shader-side variable name no longer used on the non-ISL gate path) and didn't note the clustered-only scoping that landed in 5569754. Bring the field doc in line with the shader's actual gate. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
When contact shadows are off (default) or the pixel is past MaxDistance, contactShadowSteps == 0 and the per-light branch below rejects -- but the falloff/passesIntensityGate math still ran unconditionally for every clustered light, costing ~3-6 ops per light per pixel of pure overhead in the common-off case (8-15 clustered lights per cluster typically). Wrap the entire intensity-gate block in [branch] if (contactShadowSteps > 0). Also skip the falloff computation entirely for strict lights (passesIntensityGate is unconditionally true for them) instead of computing normalizedFalloff and OR'ing. No semantic change; default-off pixels and pixels-past-MaxDistance now skip the falloff math entirely. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
The previous comment attributed the threat model to "malformed JSON configs" but LightLimitFix doesn't implement LoadSettings/SaveSettings, so there's no JSON deserialization path delivering values here today. Reword to describe the actual surface (future persistence, mod overrides, remote-control, internal bugs) and frame this as defensive boundary validation rather than a JSON-specific guard. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
LightLimitFix didn't override SaveSettings/LoadSettings, so every game launch reset the Settings struct to default-member-initializer values. That was tolerable when the only settings were two debug toggles, but the contact-shadow restoration in #36 added EnableContactShadows and this PR added six more functional tuning fields -- none of which were surviving a restart. Wire up the standard Feature persistence pattern: NLOHMANN_DEFINE_TYPE_ NON_INTRUSIVE_WITH_DEFAULT over the Settings struct (so missing JSON fields fall back to struct defaults, keeping older configs forward- compatible) and trivial LoadSettings/SaveSettings overrides matching the ScreenSpaceGI pattern. GetCommonBufferData stays as the sole sanitization point at the cbuffer boundary, so a corrupt JSON value lands in `settings` unclamped but is clamped on its way to the GPU (and the sliders auto-clamp on the next UI interaction). https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
EnableLightsVisualisation and LightsVisualisationMode are debug-only toggles -- persisting them means a user who flipped them on to inspect something stays stuck with the overlay after a restart, which is never what's wanted. Drop both from the NLOHMANN field list; the _WITH_DEFAULT variant resets omitted fields to struct defaults on load, which is the intended behaviour for debug state. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
alandtse
force-pushed
the
claude/contact-shadows-improvements-EeGoD
branch
from
eb37b42 to
45c2d89
Compare
Two Copilot review findings on 8bd771a: 1. The PerFrame static_assert message and comment referenced a fictional shader-side "PerFrameLLF" cbuffer. The real shader-side mirror is SharedData::LightLimitFixSettings inside the shared FeatureData cbuffer (b6, package/Shaders/Common/SharedData.hlsli). Update both the block comment and the assert message to point at the actual struct so the size-lock failure mode is actionable next time it triggers. 2. Only the MaxSteps SliderScalar passed ImGuiSliderFlags_AlwaysClamp; the five SliderFloat controls did not, so Ctrl+Click text entry could land arbitrary out-of-range values in settings between UI input and the next cbuffer write. Add AlwaysClamp to all five floats so the in-memory settings struct stays bounded. The GetCommonBufferData sanitization remains as defense in depth for non-UI write paths. https://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
alandtse
added a commit
that referenced
this pull request
May 26, 2026
Merge origin/dev which landed PR #43 (contact shadows) and PR #42 (DLSSperf → PerfMode rename + perf-mode generalization). Conflicts resolved: - src/Features/LightLimitFix.h: merged PerFrame layout to include both ContactShadow* tuning (from #43) and ShadowMapSlots (from this PR). Final layout: 10 uint/float fields (EnableContactShadows, 6 ContactShadow*, 2 visualisation, ShadowMapSlots) + uint pad[2] + uint ClusterSize[4] = 64 bytes (sizeof asserted). - package/Shaders/Common/SharedData.hlsli: mirrored same layout. - src/Features/LightLimitFix.cpp: * NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT now persists both ContactShadow tuning fields (from #43) and ShadowSettings / ShowShadowOverlay (from this PR); EnableLightsVisualisation / LightsVisualisationMode remain intentionally non-persistent debug toggles. * GetCommonBufferData() populates contact-shadow params + the new ShadowMapSlots field. * LoadSettings/SaveSettings keep the ShadowCasterManager INI hooks from this PR (dev did not touch these helpers). * Dropped the dev-side redundant `static constexpr CLUSTER_MAX_LIGHTS / MAX_LIGHTS` globals -- the class statics in LightLimitFix.h are the canonical definitions and all callers already use them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 29, 2026
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
Polish on the contact-shadow feature restored in #36. Settings exposure, perspective-correct ray marching, per-light intensity cutoff, and review fixes.
What's in this PR
feat(llf): expose contact-shadow settings and skip distant lightsrefactor(llf): dedupe EnableContactShadows gate, tighten commentscontactShadowSteps > 0(also catches the past-MaxDistance case so the matrix-multiply is skipped there too). Comment cleanup.perf(llf): perspective-correct contact-shadow steppingfix(llf): address PR #43 contact-shadow review feedbackMinIntensity,SliderScalarfor the uint setting (strict-aliasing), settings clamping inGetCommonBufferData,static_assert(sizeof(PerFrame) == 64).docs(llf): correct VR contact-shadow noise coord commentfix(llf): correct normalized falloff formula and handle NaN configs(saturate(1 - d/r))^2, now matches the non-ISLintensityMultiplierformula1 - (saturate(d/r))^2.std::clamppasses NaN through, so add an explicitstd::isfinitecheck before clamping.Perf / quality expectations
MinIntensitycutoff skips the matrix-multiply + raymarch for clustered lights whose normalized falloff at the pixel drops below 0.25 (default). Strict lights always march. Neutral when contact shadows are off.MinIntensity = 0to exactly preserve prior behavior. Quality improvement at distance from the perspective-correct stride.MaxSteps = 2to halve per-eye raymarch cost. No automatic VR-specific defaults (single setting easier to reason about than a split).Test plan
static_assert(sizeof(PerFrame) == 64))MinIntensitysliderMaxSteps = 2to confirm cost reductionhttps://claude.ai/code/session_01QhBc5srHV2VBA1qFNBooJs
Summary by CodeRabbit
New Features
Improvements