feat(fog): volumetric fog#2361
Open
jiayev wants to merge 17 commits into
Open
Conversation
…g light scattering
…or volumetric fog
Contributor
📝 WalkthroughWalkthroughAdds a volumetric froxel-based exponential-height-fog pipeline: new HLSL helpers and compute shaders for material, conservative depth, light scattering, and integration; refactors exponential fog shader to blend volumetric and analytical results; updates pixel shaders; and extends C++ runtime for resource management, dispatch, history, and UI. ChangesVolumetric Fog Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.21.0)OpenGrep fatal error (exit code 2): [00.16][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/ExponentialHeightFog.h 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. |
![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: 6
🧹 Nitpick comments (1)
features/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogCommon.hlsli (1)
1-3: Consider linking this feature PR to a tracking issue.If there is an existing ticket, add a keyword like
Implements#123or `Addresses `#123in the PR description for traceability.As per coding guidelines
**/*: “Issue References … Suggest adding appropriate GitHub keywords … for features.”🤖 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/Exponential` Height Fog/Shaders/ExponentialHeightFog/VolumetricFogCommon.hlsli around lines 1 - 3, This PR adds the Exponential Height Fog volumetric feature (see header guard __EXPONENTIAL_HEIGHT_FOG_VOLUMETRIC_COMMON_HLSLI__); update the PR description to include a tracking-issue keyword such as "Implements #<issue-number>" or "Addresses #<issue-number>" that references the related GitHub issue so the feature is traceable per the coding guidelines.
🤖 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 `@features/Exponential` Height
Fog/Shaders/ExponentialHeightFog/ExponentialHeightFog.hlsli:
- Around line 168-172: The early return when fogDensity <= 0.0f in
ExponentialHeightFog.hlsli prematurely disables the whole feature; instead
remove the return and treat fogDensity==0 as disabling only
analytical/ground-based density while still allowing volumetric sampling and
blending to run. Modify the code around
SharedData::exponentialHeightFogSettings.fogDensity and fogHeightFalloff so that
if fogDensity <= 0.0f you set fogDensity to 0.0f (or branch to skip analytical
density computations) but do not return from the function, ensuring subsequent
volumetric sampling/blending code still executes.
- Around line 109-142: SampleVolumetricFog is not stereo-aware: it computes
volumeUV from screenPosition without stereo-conversion or eye-clamping so VR
right-eye reads the wrong froxels; update this path to mirror the world-space
sampler by threading an eyeIndex through
CombineVolumetricFog/GetExponentialHeightFog callers, convert screen-space UVs
with StereoToTextureUV (or equivalent) and apply the same eye clamping used near
the world-space sampler (use ClampToEye/eye-aware UV bounds) before calling
ExponentialHeightFogIntegratedLightScattering.SampleLevel; ensure the new
eyeIndex parameter is added to SampleVolumetricFog and propagated to its callers
so the correct texture slice is sampled per eye.
In `@features/Exponential` Height
Fog/Shaders/ExponentialHeightFog/VolumetricFogLightScatteringCS.hlsl:
- Around line 277-304: ComputeCellWorldPosition overwrites the out uint eyeIndex
when computing cellCornerWS, which then causes subsequent accesses like
light.positionWS[eyeIndex] to use the wrong eye for local-light sampling;
preserve the original eye index by saving it to a temporary (e.g., savedEye)
before calling ExponentialHeightFog::ComputeCellWorldPosition for the corner
sample, then use savedEye when indexing light.positionWS and any other per-eye
arrays during the localScattering accumulation.
In `@package/Shaders/Effect.hlsl`:
- Around line 891-894: The current EXP_HEIGHT_FOG path overwrites the vanilla
blend because blendedColor is first computed as lerp(lightColor,
vanillaFogColor, vanillaFogFactor.xxx) then immediately reset using lightColor
again; change the second assignment so it blends the existing blendedColor with
fogColor using fogFactor (i.e., lerp(blendedColor, fogColor, fogFactor.xxx)) so
the vanillaFogColor contribution is preserved; update the code around the
EXP_HEIGHT_FOG block (symbols: blendedColor, lightColor, vanillaFogColor,
vanillaFogFactor, fogColor, fogFactor, EXP_HEIGHT_FOG and the
ShouldDisableVanillaFog() path) accordingly.
In `@src/Features/ExponentialHeightFog.cpp`:
- Around line 256-295: The texture allocations (e.g., vBufferA,
conservativeDepth, conservativeDepthHistory, lightScattering,
lightScatteringHistory, integratedLightScattering created with
Texture3D/Texture2D and subsequent CreateSRV/CreateUAV calls) lack defensive
checks for allocation or SRV/UAV creation failures; wrap each allocation and
view-creation in a try/catch or check the underlying resource/creation-result
and handle failures by logging an error with context (e.g.,
"ExponentialHeightFog::ConservativeDepth"), releasing any partially-created
resources (resetting the related unique_ptr), and returning/propagating a clear
failure code/path instead of letting exceptions or invalid resources leak into
the rest of the system. Ensure checks reference the same symbols (vBufferA,
conservativeDepth, conservativeDepthHistory, lightScattering,
integratedLightScattering and their CreateSRV/CreateUAV calls) and provide
deterministic cleanup and error reporting.
- Around line 324-361: The shader getters (GetMaterialSetupCS,
GetConservativeDepthCS, GetLightScatteringCS, GetIntegrationCS) currently return
the raw result of Util::CompileShader which can be nullptr on failure; update
each getter to detect a nullptr from Util::CompileShader and log a warning once
(e.g., via processLogger or existing logging utility) including the shader path
and any defines used so failures are visible, and ensure the log only repeats on
the first failure for that specific shader to avoid spamming; keep the existing
lazy initialization and return behavior otherwise.
---
Nitpick comments:
In `@features/Exponential` Height
Fog/Shaders/ExponentialHeightFog/VolumetricFogCommon.hlsli:
- Around line 1-3: This PR adds the Exponential Height Fog volumetric feature
(see header guard __EXPONENTIAL_HEIGHT_FOG_VOLUMETRIC_COMMON_HLSLI__); update
the PR description to include a tracking-issue keyword such as "Implements
#<issue-number>" or "Addresses #<issue-number>" that references the related
GitHub issue so the feature is traceable per the coding guidelines.
🪄 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 Plus
Run ID: 571aaa10-5f02-4da7-bb15-ca4a29bdd956
📒 Files selected for processing (16)
features/Exponential Height Fog/Shaders/ExponentialHeightFog/ExponentialHeightFog.hlslifeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogCSCommon.hlslifeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogCommon.hlslifeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogConservativeDepthCS.hlslfeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogIntegrationCS.hlslfeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogLightScatteringCS.hlslfeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogMaterialCS.hlslfeatures/Exponential Height Fog/Shaders/Features/ExponentialHeightFog.inipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Effect.hlslpackage/Shaders/ISSAOComposite.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/Water.hlslsrc/Features/ExponentialHeightFog.cppsrc/Features/ExponentialHeightFog.hsrc/State.cpp
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Features/ExponentialHeightFog.cpp`:
- Around line 680-733: The DrawSettings-exposed volumetric controls are missing
registry entries—add RegisterVariable calls for "volumetricFogStartDistance"
(WeatherVariables::FloatVariable), "volumetricFogNearFadeInDistance"
(WeatherVariables::FloatVariable), and
"volumetricDirectionalScatteringIntensity" (WeatherVariables::FloatVariable) in
the same block where other volumetric settings are registered (in
ExponentialHeightFog.cpp), binding each to the corresponding settings member
(settings.volumetricFogStartDistance, settings.volumetricFogNearFadeInDistance,
settings.volumetricDirectionalScatteringIntensity), and use sensible
defaults/ranges consistent with the existing volumetric variables (e.g.,
distances in the same scale/range as volumetricFogDistance and intensities
similar to volumetricSkyLightingIntensity), following the pattern of
std::make_shared<WeatherVariables::FloatVariable>(...) used for the other
entries.
- Around line 556-563: When temporal reprojection is disabled, avoid the
unnecessary GPU copies of the light scattering and conservative depth history;
update the block that currently calls
context->CopyResource(lightScatteringHistory->resource.get(),
lightScattering->resource.get()) and
context->CopyResource(conservativeDepthHistory->resource.get(),
conservativeDepth->resource.get()) so those CopyResource calls are skipped when
temporalReprojection is false, and ensure hasLightScatteringHistory and
hasConservativeDepthHistory are set correctly (false when reprojection is
disabled or when depthSrv is absent) to reflect that history SRVs will not be
consumed; modify the logic around temporalReprojection, lightScatteringHistory,
conservativeDepthHistory, depthSrv, hasLightScatteringHistory and
hasConservativeDepthHistory to gate the copies and flags accordingly.
🪄 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 Plus
Run ID: cc2181cc-0b25-4485-92f2-a645c020e633
📒 Files selected for processing (5)
features/Exponential Height Fog/Shaders/ExponentialHeightFog/ExponentialHeightFog.hlslifeatures/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogLightScatteringCS.hlslpackage/Shaders/Effect.hlslpackage/Shaders/ISSAOComposite.hlslsrc/Features/ExponentialHeightFog.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- package/Shaders/ISSAOComposite.hlsl
- package/Shaders/Effect.hlsl
- features/Exponential Height Fog/Shaders/ExponentialHeightFog/VolumetricFogLightScatteringCS.hlsl
- features/Exponential Height Fog/Shaders/ExponentialHeightFog/ExponentialHeightFog.hlsli
Comment on lines
+556
to
+563
| context->CopyResource(lightScatteringHistory->resource.get(), lightScattering->resource.get()); | ||
| hasLightScatteringHistory = true; | ||
| if (depthSrv) { | ||
| context->CopyResource(conservativeDepthHistory->resource.get(), conservativeDepth->resource.get()); | ||
| hasConservativeDepthHistory = true; | ||
| } else { | ||
| hasConservativeDepthHistory = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Skip history copies when temporal reprojection is disabled.
When temporalReprojection is false, the history SRVs are never consumed, but this still copies the full light-scattering volume every frame and copies conservative depth whenever a depth SRV exists. On larger froxel grids, that's avoidable bandwidth on a hot render path.
♻️ Proposed change
- context->CopyResource(lightScatteringHistory->resource.get(), lightScattering->resource.get());
- hasLightScatteringHistory = true;
- if (depthSrv) {
- context->CopyResource(conservativeDepthHistory->resource.get(), conservativeDepth->resource.get());
- hasConservativeDepthHistory = true;
- } else {
- hasConservativeDepthHistory = false;
- }
+ if (temporalReprojection) {
+ context->CopyResource(lightScatteringHistory->resource.get(), lightScattering->resource.get());
+ hasLightScatteringHistory = true;
+ if (depthSrv) {
+ context->CopyResource(conservativeDepthHistory->resource.get(), conservativeDepth->resource.get());
+ hasConservativeDepthHistory = true;
+ } else {
+ hasConservativeDepthHistory = false;
+ }
+ } else {
+ hasLightScatteringHistory = false;
+ hasConservativeDepthHistory = false;
+ }As per coding guidelines, "Flag potential performance impacts on rendering performance when implementing graphics features and suggest user toggles for feature control".
🤖 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 `@src/Features/ExponentialHeightFog.cpp` around lines 556 - 563, When temporal
reprojection is disabled, avoid the unnecessary GPU copies of the light
scattering and conservative depth history; update the block that currently calls
context->CopyResource(lightScatteringHistory->resource.get(),
lightScattering->resource.get()) and
context->CopyResource(conservativeDepthHistory->resource.get(),
conservativeDepth->resource.get()) so those CopyResource calls are skipped when
temporalReprojection is false, and ensure hasLightScatteringHistory and
hasConservativeDepthHistory are set correctly (false when reprojection is
disabled or when depthSrv is absent) to reflect that history SRVs will not be
consumed; modify the logic around temporalReprojection, lightScatteringHistory,
conservativeDepthHistory, depthSrv, hasLightScatteringHistory and
hasConservativeDepthHistory to gate the copies and flags accordingly.
Comment on lines
+680
to
733
| registry->RegisterVariable(std::make_shared<WeatherVariables::FloatVariable>( | ||
| "Volumetric View Distance", | ||
| "volumetricFogDistance", | ||
| "Maximum distance covered by exponential height volumetric fog", | ||
| &settings.volumetricFogDistance, | ||
| 60000.0f, | ||
| 1000.0f, 200000.0f)); | ||
|
|
||
| registry->RegisterVariable(std::make_shared<WeatherVariables::FloatVariable>( | ||
| "Volumetric Extinction Scale", | ||
| "volumetricFogExtinctionScale", | ||
| "Scale applied to volumetric fog extinction", | ||
| &settings.volumetricFogExtinctionScale, | ||
| 1.0f, | ||
| 0.0f, 10.0f)); | ||
|
|
||
| registry->RegisterVariable(std::make_shared<WeatherVariables::FloatVariable>( | ||
| "Volumetric Scattering Distribution", | ||
| "volumetricFogScatteringDistribution", | ||
| "Henyey-Greenstein scattering distribution for volumetric fog", | ||
| &settings.volumetricFogScatteringDistribution, | ||
| 0.2f, | ||
| -0.9f, 0.9f)); | ||
|
|
||
| registry->RegisterVariable(std::make_shared<WeatherVariables::Float4Variable>( | ||
| "Volumetric Albedo", | ||
| "volumetricFogAlbedo", | ||
| "Volumetric fog albedo color", | ||
| &settings.volumetricFogAlbedo, | ||
| float4{ 1.0f, 1.0f, 1.0f, 1.0f })); | ||
|
|
||
| registry->RegisterVariable(std::make_shared<WeatherVariables::Float4Variable>( | ||
| "Volumetric Emissive", | ||
| "volumetricFogEmissive", | ||
| "Volumetric fog emissive color", | ||
| &settings.volumetricFogEmissive, | ||
| float4{ 0.0f, 0.0f, 0.0f, 0.0f })); | ||
|
|
||
| registry->RegisterVariable(std::make_shared<WeatherVariables::FloatVariable>( | ||
| "Volumetric Sky Lighting Intensity", | ||
| "volumetricSkyLightingIntensity", | ||
| "Scale applied to volumetric fog sky lighting", | ||
| &settings.volumetricSkyLightingIntensity, | ||
| 1.0f, | ||
| 0.0f, 10.0f)); | ||
|
|
||
| registry->RegisterVariable(std::make_shared<WeatherVariables::FloatVariable>( | ||
| "Volumetric Local Light Scattering Intensity", | ||
| "volumetricLocalLightScatteringIntensity", | ||
| "Scale applied to volumetric fog local light scattering", | ||
| &settings.volumetricLocalLightScatteringIntensity, | ||
| 1.0f, | ||
| 0.0f, 100.0f)); | ||
| } |
Contributor
There was a problem hiding this comment.
Register the remaining WeatherUI-backed volumetric settings.
DrawSettings() exposes volumetricFogStartDistance, volumetricFogNearFadeInDistance, and volumetricDirectionalScatteringIntensity through Util::WeatherUI::*, but they never get a matching RegisterVariable() entry here. That leaves those controls outside the weather registry/blending path while the rest of the volumetric fog settings participate in it.
🤖 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 `@src/Features/ExponentialHeightFog.cpp` around lines 680 - 733, The
DrawSettings-exposed volumetric controls are missing registry entries—add
RegisterVariable calls for "volumetricFogStartDistance"
(WeatherVariables::FloatVariable), "volumetricFogNearFadeInDistance"
(WeatherVariables::FloatVariable), and
"volumetricDirectionalScatteringIntensity" (WeatherVariables::FloatVariable) in
the same block where other volumetric settings are registered (in
ExponentialHeightFog.cpp), binding each to the corresponding settings member
(settings.volumetricFogStartDistance, settings.volumetricFogNearFadeInDistance,
settings.volumetricDirectionalScatteringIntensity), and use sensible
defaults/ranges consistent with the existing volumetric variables (e.g.,
distances in the same scale/range as volumetricFogDistance and intensities
similar to volumetricSkyLightingIntensity), following the pattern of
std::make_shared<WeatherVariables::FloatVariable>(...) used for the other
entries.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/State.cpp (1)
100-101:⚠️ Potential issue | 🟠 MajorGate
ExponentialHeightFog::CaptureDirectionalShadowMap()to active volumetric fog
src/Features/ExponentialHeightFog.cpp’sExponentialHeightFog::CaptureDirectionalShadowMap()unconditionally reads PS shader resource slot 4 and copies it intodirectionalShadowMap; it does not checksettings.volumetricFogEnabled/settings.enabledand has nolastPrepassFrame/frameCount guard. As a result, the call fromsrc/State.cpp’s shadowmask hot path will still do work when volumetric fog is off, risking avoidable per-frame overhead.🤖 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 `@src/State.cpp` around lines 100 - 101, The call to ExponentialHeightFog::CaptureDirectionalShadowMap() is currently unconditional and causes work even when volumetric fog is disabled; modify the logic so CaptureDirectionalShadowMap() early-returns when fog is not active (check settings.enabled and settings.volumetricFogEnabled) and add a frame guard using lastPrepassFrame vs current frameCount to avoid duplicate work per-frame; alternatively (or in addition) gate the call site in State.cpp (where globals::features::exponentialHeightFog.CaptureDirectionalShadowMap() is invoked) so it only calls when exponentialHeightFog.loaded && settings.enabled && settings.volumetricFogEnabled and the lastPrepassFrame/frameCount guard prevents repeated captures. Ensure directionalShadowMap reads only occur behind these guards.
🤖 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.
Outside diff comments:
In `@src/State.cpp`:
- Around line 100-101: The call to
ExponentialHeightFog::CaptureDirectionalShadowMap() is currently unconditional
and causes work even when volumetric fog is disabled; modify the logic so
CaptureDirectionalShadowMap() early-returns when fog is not active (check
settings.enabled and settings.volumetricFogEnabled) and add a frame guard using
lastPrepassFrame vs current frameCount to avoid duplicate work per-frame;
alternatively (or in addition) gate the call site in State.cpp (where
globals::features::exponentialHeightFog.CaptureDirectionalShadowMap() is
invoked) so it only calls when exponentialHeightFog.loaded && settings.enabled
&& settings.volumetricFogEnabled and the lastPrepassFrame/frameCount guard
prevents repeated captures. Ensure directionalShadowMap reads only occur behind
these guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ae30107-a690-4304-9a27-d1eea5b58073
📒 Files selected for processing (3)
package/Shaders/Common/SharedData.hlslisrc/Features/ExponentialHeightFog.hsrc/State.cpp
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.
This pull request introduces a comprehensive implementation of volumetric fog for the Exponential Height Fog system. The changes add support for 3D volumetric fog grids, new compute shaders for fog material, conservative depth, and light integration, and a variety of utility functions and settings to control the volumetric fog rendering pipeline. The update also refactors and expands the fog shader code to support both analytical and volumetric fog blending, with support for VR and various quality/performance settings.
The most important changes are:
Volumetric Fog Feature Implementation
VolumetricFogMaterialCS.hlsl), conservative depth calculation (VolumetricFogConservativeDepthCS.hlsl), and light scattering integration (VolumetricFogIntegrationCS.hlsl). These shaders implement the core volumetric fog rendering pipeline. [1] [2] [3]VolumetricFogCommon.hlsliandVolumetricFogCSCommon.hlsliwith reusable functions for grid coordinate conversions, fog parameter calculations, and the Henyey-Greenstein phase function, centralizing volumetric fog math and logic. [1] [2]Shader and API Extensions
ExponentialHeightFog.hlslito sample and blend volumetric fog with analytical fog, handle VR, and expose new utility functions for sampling and combining fog. The Henyey-Greenstein function was moved to the common file. [1] [2] [3]exponentialHeightFogSettingsstruct inSharedData.hlsli, enabling control over grid size, distances, albedo, extinction, and more.Configuration
.inito1-3-0to reflect the new volumetric fog capabilities.Summary by CodeRabbit
New Features
Documentation
Chores