feat(cloud shadows): add inner cloud shadowing#2050
Closed
InTheBottle wants to merge 5 commits into
Closed
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a volumetric inner-cloud shadow path: new shader texture Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as Engine (C++)
participant SRV as Cubemap SRVs
participant SkyPS as Sky Pixel Shader
participant VolTex as VolumetricCloudShadowsTexture (t25)
participant OccTex as CloudShadowsTexture (t26)
Engine->>SRV: Bind texCubemapCloudOccCopy -> slot 25\nBind texCubemapCloudOcc / copy -> slot 26 (per pass)
SkyPS->>VolTex: SampleLevel(…, 0) for raySample.z >= 0
SkyPS->>OccTex: Sample for legacy shadowing
SkyPS->>SkyPS: Accumulate 4-sample rayShadow, apply InnerCloudShadowOpacity
SkyPS-->>Engine: Output modified psout.Color
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
No actionable suggestions for changed features. |
![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 (2)
src/Features/CloudShadows.h (1)
9-14: Add a compile-time layout guard for the shader settings struct.This now has to stay byte-for-byte compatible with
SharedData::CloudShadowsSettings; if the size or alignment drifts later, the rest ofFeatureDatashifts silently. A localstatic_assertwould fail that fast.♻️ Proposed guard
struct alignas(16) Settings { float Opacity = 0.8f; float InnerCloudShadowOpacity = 0.5f; float pad[2]; }; + + static_assert(sizeof(Settings) == 16, "CloudShadows::Settings must match SharedData::CloudShadowsSettings"); + static_assert(alignof(Settings) == 16);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/CloudShadows.h` around lines 9 - 14, The Settings struct must be byte-for-byte compatible with SharedData::CloudShadowsSettings to prevent silent FeatureData layout shifts; add compile-time layout guards by inserting static_asserts that compare sizeof(Settings) == sizeof(SharedData::CloudShadowsSettings) and alignof(Settings) == alignof(SharedData::CloudShadowsSettings) (and optionally check offsetof for members if desired) so any size/alignment drift fails at compile time; place these static_asserts after the struct definition referencing Settings and SharedData::CloudShadowsSettings.package/Shaders/Sky.hlsl (1)
246-273: Skip the ray-march when inner cloud shadows are effectively off.With
InnerCloudShadowOpacity == 0, this block still does four normalizations and up to four cubemap samples per cloud pixel for a no-op result. Guarding the branch with the setting lets users disable the cost as well as the visual effect.♻️ Proposed tweak
- if (baseColor.w > 0.0) { + if (baseColor.w > 0.0 && SharedData::cloudShadowsSettings.InnerCloudShadowOpacity > 0.0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Sky.hlsl` around lines 246 - 273, Skip the inner-cloud shadow ray-march when the setting disables it by adding an early guard that checks SharedData::cloudShadowsSettings.InnerCloudShadowOpacity > 0.0 before performing the heavy work (the whole block that computes viewDir, rayStep, PoissonDisc, the unrolled loop and the SampleLevel calls); only run the normalization, Poisson sampling and CloudShadows::VolumetricCloudShadowsTexture.SampleLevel lookups when InnerCloudShadowOpacity is non-zero, and otherwise leave psout.Color unchanged.
🤖 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/CloudShadows.cpp`:
- Around line 139-147: The uninitialized texCubemapCloudOccCopy is bound in
EarlyPrepass (t25) before any rendering copies in ReflectionsPrepass, so
initialize/clear the cubemap in SetupResources: explicitly create or zero-clear
texCubemapCloudOccCopy (and texCubemapCloudOcc) or clear all
cubemapCloudOccCopyRTVs once during SetupResources so that ReflectionsPrepass
doesn't copy garbage; ensure this initialization runs before
ReflectionsPrepass/EarlyPrepass and reference texCubemapCloudOccCopy,
cubemapCloudOccCopyRTVs, SetupResources(), ReflectionsPrepass(), and
EarlyPrepass() when making the change.
---
Nitpick comments:
In `@package/Shaders/Sky.hlsl`:
- Around line 246-273: Skip the inner-cloud shadow ray-march when the setting
disables it by adding an early guard that checks
SharedData::cloudShadowsSettings.InnerCloudShadowOpacity > 0.0 before performing
the heavy work (the whole block that computes viewDir, rayStep, PoissonDisc, the
unrolled loop and the SampleLevel calls); only run the normalization, Poisson
sampling and CloudShadows::VolumetricCloudShadowsTexture.SampleLevel lookups
when InnerCloudShadowOpacity is non-zero, and otherwise leave psout.Color
unchanged.
In `@src/Features/CloudShadows.h`:
- Around line 9-14: The Settings struct must be byte-for-byte compatible with
SharedData::CloudShadowsSettings to prevent silent FeatureData layout shifts;
add compile-time layout guards by inserting static_asserts that compare
sizeof(Settings) == sizeof(SharedData::CloudShadowsSettings) and
alignof(Settings) == alignof(SharedData::CloudShadowsSettings) (and optionally
check offsetof for members if desired) so any size/alignment drift fails at
compile time; place these static_asserts after the struct definition referencing
Settings and SharedData::CloudShadowsSettings.
🪄 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: f60a47a2-e9e6-4902-bc0f-9f760c54b670
📒 Files selected for processing (5)
features/Cloud Shadows/Shaders/CloudShadows/CloudShadows.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Sky.hlslsrc/Features/CloudShadows.cppsrc/Features/CloudShadows.h
|
✅ A pre-release build is available for this PR: |
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.
Summary by CodeRabbit
New Features
Improvements