fix(Volumetric Shadows): Interior Sun Compatibility #1935
Closed
Dlizzio wants to merge 7 commits into
Closed
Conversation
Removes IsInterior checks when appropriate for Interior Sun. Volumetric Shadows only works for directional lighting so it makes sense to remove interior checks instead of adding an interior sun check. Added effect mesh sun influence slider for effect meshes.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds interior-sun shader data and runtime plumbing: new SharedData::InteriorSunSettings, shader logic updated to honor IsInteriorWithSun and EffectMeshSunInfluence, Feature-side GetShaderSettings() + UI/serialization, and feature buffer upload of the new settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime as Runtime (InteriorSun feature)
participant FeatureBuf as FeatureBuffer
participant GPU as GPU Shader (SharedData / Effect)
participant Shadow as ShadowSystem
Runtime->>FeatureBuf: GetShaderSettings() (EffectMeshSunInfluence, IsInteriorWithSun)
FeatureBuf->>GPU: Upload FeatureData.cbuffer (includes InteriorSunSettings)
GPU->>Shadow: Request shadow sample (shadowMap / compute3DFilteredShadow)
Shadow-->>GPU: ShadowValue
GPU->>GPU: Apply lighting path (dirColor + ambient)
note right of GPU: if IsInteriorWithSun -> lerp using EffectMeshSunInfluence\nelse -> standard shadow * dirColor
GPU-->>Runtime: Rendered output (final color)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Actionable Suggestions
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/InteriorSun.h (1)
40-52: Add compile-time layout checks forShaderSettings.
Since this struct is copied into the feature buffer and mirrored in HLSL, a size/alignmentstatic_assertwill catch accidental layout drift.♻️ Proposed refactor
struct alignas(16) ShaderSettings { float EffectMeshSunInfluence = 1.0f; uint32_t IsInteriorWithSun = 0; float pad[2] = {}; }; + +static_assert(sizeof(ShaderSettings) == 16, "ShaderSettings must match HLSL InteriorSunSettings size"); +static_assert(alignof(ShaderSettings) == 16, "ShaderSettings must match HLSL InteriorSunSettings alignment");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/InteriorSun.h` around lines 40 - 52, Add compile-time layout checks for ShaderSettings to prevent layout drift between C++ and HLSL: inside the same header (after struct ShaderSettings declaration) add static_asserts that validate sizeof(ShaderSettings) matches the expected byte size and that alignof(ShaderSettings) is 16, and optionally check offsetof(ShaderSettings, IsInteriorWithSun) (and other fields) against expected byte offsets; keep the checks near ShaderSettings/GetShaderSettings so failures point to the right type and update the expected constants if the HLSL layout changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/InteriorSun.cpp`:
- Around line 227-233: GetShaderSettings currently writes
settings.EffectMeshSunInfluence directly into ShaderSettings after multiplying
by 0.01f, which can produce out-of-range lerp values if LoadSettings
deserialized bad data; clamp settings.EffectMeshSunInfluence to [0,100] (or
clamp the resulting data.EffectMeshSunInfluence to [0.0f,1.0f]) inside
InteriorSun::GetShaderSettings before scaling/assigning so
ShaderSettings.EffectMeshSunInfluence is guaranteed within [0,1]; update
GetShaderSettings (referencing EffectMeshSunInfluence, ShaderSettings, and
GetShaderSettings) to perform the clamp and then return the sanitized value.
---
Nitpick comments:
In `@src/Features/InteriorSun.h`:
- Around line 40-52: Add compile-time layout checks for ShaderSettings to
prevent layout drift between C++ and HLSL: inside the same header (after struct
ShaderSettings declaration) add static_asserts that validate
sizeof(ShaderSettings) matches the expected byte size and that
alignof(ShaderSettings) is 16, and optionally check offsetof(ShaderSettings,
IsInteriorWithSun) (and other fields) against expected byte offsets; keep the
checks near ShaderSettings/GetShaderSettings so failures point to the right type
and update the expected constants if the HLSL layout changes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/InteriorSun.cpp (1)
1-1: PR title: scope should be lowercase per conventional commits guideline.
fix(Volumetric Shadows):uses a capitalized, multi-word scope. Per the coding guidelines, scopes should be lowercase (e.g.,fix(volumetric-shadows): compatibility with interior sun). The current title is also one character over the 50-character limit.📝 Suggested title
fix(volumetric-shadows): compatibility with interior sunAs per coding guidelines: "Format: type(scope): description … Style: lowercase description … Examples: fix(water): resolve flowmap bug".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/InteriorSun.cpp` at line 1, The PR title uses a capitalized multi-word scope "Volumetric Shadows" and exceeds the 50-character limit; update the commit/PR title to use a lowercase, hyphenated scope and shorten the description (e.g., change from `fix(Volumetric Shadows): ...` to `fix(volumetric-shadows): compatibility with interior sun`) so it follows the conventional commits style and stays within 50 characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Features/InteriorSun.cpp`:
- Around line 227-233: Ensure EffectMeshSunInfluence is clamped before scaling
so GPU value stays in [0,1]: inside InteriorSun::GetShaderSettings() clamp
settings.EffectMeshSunInfluence with std::clamp(..., 0.0f, 100.0f) and then
multiply by 0.01f to assign to ShaderSettings::EffectMeshSunInfluence; keep the
method const and set ShaderSettings::IsInteriorWithSun using isInteriorWithSun ?
1u : 0u so deserialized out-of-range inputs from LoadSettings() cannot produce
invalid GPU values.
---
Nitpick comments:
In `@src/Features/InteriorSun.cpp`:
- Line 1: The PR title uses a capitalized multi-word scope "Volumetric Shadows"
and exceeds the 50-character limit; update the commit/PR title to use a
lowercase, hyphenated scope and shorten the description (e.g., change from
`fix(Volumetric Shadows): ...` to `fix(volumetric-shadows): compatibility with
interior sun`) so it follows the conventional commits style and stays within 50
characters.
|
✅ A pre-release build is available for this PR: |
alandtse
reviewed
Feb 23, 2026
alandtse
reviewed
Feb 23, 2026
alandtse
requested changes
Feb 23, 2026
Collaborator
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Is the old inInterior flag deprecated now? Should it be removed?
alandtse
changed the title
Contributor
Author
It is used broadly with other features. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/Shaders/ISVolumetricLightingGenerateCS.hlsl (1)
89-89: PR title doesn't follow the conventional commits style.The title
fix(Volumetric Shadows): Interior Sun Compatibilityhas a Title-Case scope and description, and is one character over the 50-character limit. A conforming title would be:fix(volumetric shadows): interior sun compatibilityAs per coding guidelines, the scope and description should be lowercase with no ending period, and the title should stay within 50 characters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISVolumetricLightingGenerateCS.hlsl` at line 89, Update the PR title to follow conventional commits: change it to "fix(volumetric shadows): interior sun compatibility" (lowercase scope and description, no trailing period) and ensure it is <=50 characters; this relates to the change touching ISVolumetricLightingGenerateCS.hlsl (e.g., the noShadow / SharedData::InInterior logic) so update the pull request title accordingly before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package/Shaders/ISVolumetricLightingGenerateCS.hlsl`:
- Line 89: Update the PR title to follow conventional commits: change it to
"fix(volumetric shadows): interior sun compatibility" (lowercase scope and
description, no trailing period) and ensure it is <=50 characters; this relates
to the change touching ISVolumetricLightingGenerateCS.hlsl (e.g., the noShadow /
SharedData::InInterior logic) so update the pull request title accordingly
before merging.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Shaders/ISVolumetricLightingGenerateCS.hlslsrc/Features/InteriorSun.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/InteriorSun.h
Contributor
Author
|
Drafting this until I look at effect meshes problem with Volumetric Shadows. |
# Conflicts: # package/Shaders/RunGrass.hlsl
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
2 participants
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.
Removes IsInterior checks when appropriate for Interior Sun. Volumetric Shadows only works for directional lighting so it makes sense to remove interior checks instead of adding an interior sun check.
Added effect mesh sun influence slider for effect meshes.
Summary by CodeRabbit
New Features
Bug Fixes / Visual Improvements