fix(interior sun): volumetric shadows compatibility #2319
Conversation
|
No actionable suggestions for changed features. |
📝 WalkthroughWalkthroughSharedData and State gain a HasDirectionalShadows field populated from State::HasDirectionalShadows(). InteriorSun exposes IsActiveInteriorSun(). Shaders add ShadowSampling::HasDirectionalShadows() and use it to gate volumetric/directional shadow computation. VolumetricShadows centralizes SRV binding and early-exits when directional shadows are disabled. Feature INI versions incremented. ChangesInterior Sun Directional Shadow Support
Sequence Diagram(s)(omitted — changes are localized and straightforward sequential interactions are already represented in the layer table) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
![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/Interior Sun/Shaders/Features/InteriorSun.ini (1)
2-2: ⚡ Quick winAdd an issue-closing reference in the PR description.
Since this PR is a compatibility fix, please add a keyword reference like
Fixes #...orCloses #...for traceability.As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords: 'Fixes
#123' or 'Closes#123' for bug fixes."🤖 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/Interior` Sun/Shaders/Features/InteriorSun.ini at line 2, Update the PR description to include a GitHub issue-closing keyword (e.g., "Fixes `#123`" or "Closes `#123`") referencing the relevant issue for this compatibility fix; ensure the chosen keyword appears in the PR body so merging will close the issue automatically and add traceability for the change to Features/Interior Sun (the change associated with Version = 1-0-1).package/Shaders/Common/ShadowSampling.hlsli (1)
108-119: 💤 Low valueMinor: Unreachable code when
VOLUMETRIC_SHADOWSis not defined.When
VOLUMETRIC_SHADOWSis not defined, the#elseblock on line 116 returns, making line 119 unreachable. The compiler will optimize this away, but the structure could be cleaner.♻️ Suggested restructure
`#if` defined(VOLUMETRIC_SHADOWS) if (HasDirectionalShadows()) { float vsmSurfaceShadow; float shadow = VolumetricShadows::GetVSMShadow3D(startPosition, endPosition, noise, sampleCount, eyeIndex, vsmSurfaceShadow); surfaceShadow *= vsmSurfaceShadow; return worldShadow * shadow; } -#else - return worldShadow; `#endif` return worldShadow;🤖 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/Common/ShadowSampling.hlsli` around lines 108 - 119, The else-branch returning worldShadow makes the subsequent "return worldShadow;" unreachable when VOLUMETRIC_SHADOWS is not defined; remove the redundant final return or restructure the preprocessor blocks so there is exactly one return path. Locate the block using HasDirectionalShadows(), VolumetricShadows::GetVSMShadow3D, vsmSurfaceShadow, surfaceShadow and worldShadow and either move the final "return worldShadow" into the `#else` (and delete the duplicate after the `#endif`) or remove the `#else-return` and keep the single return after the `#endif` so the code is clean and unambiguous.
🤖 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/Interior` Sun/Shaders/Features/InteriorSun.ini:
- Line 2: Update the PR description to include a GitHub issue-closing keyword
(e.g., "Fixes `#123`" or "Closes `#123`") referencing the relevant issue for this
compatibility fix; ensure the chosen keyword appears in the PR body so merging
will close the issue automatically and add traceability for the change to
Features/Interior Sun (the change associated with Version = 1-0-1).
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 108-119: The else-branch returning worldShadow makes the
subsequent "return worldShadow;" unreachable when VOLUMETRIC_SHADOWS is not
defined; remove the redundant final return or restructure the preprocessor
blocks so there is exactly one return path. Locate the block using
HasDirectionalShadows(), VolumetricShadows::GetVSMShadow3D, vsmSurfaceShadow,
surfaceShadow and worldShadow and either move the final "return worldShadow"
into the `#else` (and delete the duplicate after the `#endif`) or remove the
`#else-return` and keep the single return after the `#endif` so the code is clean
and unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0eb62b20-79a2-4cd6-87ab-d46763865c7b
📒 Files selected for processing (11)
features/Interior Sun/Shaders/Features/InteriorSun.inifeatures/Volumetric Shadows/Shaders/Features/VolumetricShadows.inipackage/Shaders/Common/ShadowSampling.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Effect.hlslpackage/Shaders/Lighting.hlslsrc/Features/InteriorSun.hsrc/Features/VolumetricShadows.cppsrc/Features/VolumetricShadows.hsrc/State.cppsrc/State.h
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Is this violating the feature boundary? We shouldn't have direct dependencies on features unless it's core.
|
✅ A pre-release build is available for this PR: |
They are both in core but I changed it anyways. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/State.cpp`:
- Around line 1087-1090: HasDirectionalShadows() can be true while no shadow SRV
is actually bound and the shader calls missing VolumetricShadows functions; fix
by making the CPU-side predicate require a valid SRV before returning true and
by either providing stub implementations or removing/guarding calls to the
missing functions in the shader. Concretely: in State::HasDirectionalShadows()
check the shadow SRV (the same resource checked in CopyShadowLightData /
PSGetShaderResources slot 4) and only return true when a non-null SRV is
available; in CopyShadowLightData ensure you never bind a null to shader slot 18
(bind a default/empty SRV or skip binding and clear the directional-shadow
flag); and update ShadowSampling.hlsli to either implement
VolumetricShadows::GetVSMShadow2D/GetVSMShadow3D or wrap their calls behind a
compile-time or runtime guard so the shader won’t reference undefined functions
when the SRV is absent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1762c1a4-1e98-473b-9fbd-3f92d5fb34c0
📒 Files selected for processing (5)
package/Shaders/Common/ShadowSampling.hlslipackage/Shaders/Common/SharedData.hlslisrc/Features/VolumetricShadows.cppsrc/State.cppsrc/State.h
|
@coderabbitai please confirm all includes are necessary and weren't made extraneous after some of the later commits to preserve the feature boundary. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the changed files to verify all includes. 🏁 Script executed: Length of output: 1679 🏁 Script executed: Length of output: 405 🏁 Script executed: Length of output: 2346
Only one new
This include is necessary and not extraneous — it is actively consumed by the new bool State::HasDirectionalShadows() const
{
return !Util::IsInterior() || globals::features::interiorSun.IsActiveInteriorSun();
}Feature boundary is preserved. The cross-feature coordination is routed correctly:
No includes were added to |
(cherry picked from commit 9353367)
(cherry picked from commit 9353367)
2 participants
Adds compatibility for interior sun and volumetric shadows.
Summary by CodeRabbit
New Features
Improvements
Chores