feat(ibl): revamp ibl and dalc sh

[jiayev]

This pull request refactors and extends the environment and sky lighting system, focusing on improving the integration of image-based lighting (IBL), directional ambient lighting color (DALC), and spherical harmonics (SH) for both interiors and exteriors. The changes introduce new methods for separating and combining environment and sky lighting, add more flexible controls for IBL/DALC blending, and update shader parameters and logic for greater accuracy and configurability.

Lighting Model Refactoring and Extension:

• Introduced new methods in ImageBasedLighting for separating environment (GetEnvIBL) and sky (GetSkyIBL) lighting using spherical harmonics, and for computing their ratio for DALC/IBL matching (GetIBLRatio). Also added settings-aware color methods (GetEnvIBLColor, GetSkyIBLColor, and updated GetIBLColor). [1] [2]
• Refactored main cubemap shaders (DynamicCubemaps.hlsli) to use the new ambient SH projection (SharedData::GetAmbient), and updated the logic for combining DALC and IBL, supporting new modes for blending and normalization. [1] [2] [3] [4]
• Updated shadow sampling logic to use the new ambient SH and IBL methods, and to support the new blending modes for DALC and sky overlays.

Shader Parameter and Data Structure Updates:

• Added new fields to SharedData::IBLSettings for separate environment and sky IBL scales and saturations, DALC blending mode, and padding.
• Added ambient SH coefficients (AmbientSHR, AmbientSHG, AmbientSHB) and a new GetAmbient method to SharedData for projecting ambient color using spherical harmonics. [1] [2]
• Included the spherical harmonics header in SharedData.hlsli to support ambient SH projection.

[1] [2] [3] [4] [5]

Summary by CodeRabbit

• New Features

  • Spherical-harmonics ambient lighting for richer, more accurate ambient illumination
  • DALC Mode (3 options) to control environment lighting behavior
  • Separate Environment and Sky IBL controls: scale, saturation, and fog mixing

• Improvements

  • Clearer env/sky IBL separation with improved HDR matching and consistent fog/reflection application
  • IBL now applies uniformly indoors/outdoors where enabled
  • Updated UI/tooltips and more consistent ambient/skylight across objects and particles

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2387dd60-5523-4e02-be76-e22b80538158

📥 Commits

Reviewing files that changed from the base of the PR and between 85aad88 and f1e1b79.

📒 Files selected for processing (3)

• package/Shaders/DeferredCompositeCS.hlsl
• package/Shaders/Lighting.hlsl
• src/Utils/SphericalHarmonics.cpp

---

📝 Walkthrough

Walkthrough

Replaces directional-ambient math with SH-based ambient (SharedData::GetAmbient), adds Spherical Harmonics utilities and DALC→SH conversion, splits IBL into Env/Sky sources with new IBL APIs and DALCMode-driven composition, renames/expands IBL settings and texture bindings, and updates many shaders/C++ paths to use the new model.

Changes

Cohort / File(s)	Summary
Spherical Harmonics Utilities src/Utils/SphericalHarmonics.h, src/Utils/SphericalHarmonics.cpp	New SH API and implementation: SH2/SH2Color types, evaluation/rotate/scale/product/convolution helpers, SH→irradiance helpers, and DALCToSH conversion.
Shared Data / State package/Shaders/Common/SharedData.hlsli, src/State.h, src/State.cpp	Add AmbientSHR/G/B to shared CB, include SH header, add SharedData::GetAmbient(normal), and write SH from DALC in State::UpdateSharedData. IBL settings renamed/expanded (EnableIBL, Env/Sky scales & saturation, DALCMode).
IBL Shader API features/IBL/Shaders/IBL/IBL.hlsli	Rename textures to Env/Sky, add GetEnvIBL/GetSkyIBL/GetIBLRatio/GetEnvIBLColor/GetSkyIBLColor/GetIBLColor, remove old diffuse APIs, and centralize env/sky color application and fog composition.
Dynamic Cubemaps / Specular features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli	Replace ambient with SharedData::GetAmbient(R), add skylightingSpecular and DALCMode-based envSpecular/skySpecular computation, unify finalIrradiance, and add non-IBL fallback.
Deferred / Composite package/Shaders/DeferredCompositeCS.hlsl, src/Deferred.cpp	Ambient uses GetAmbient; specular/irradiance flow refactored to DALCMode env/sky composition; SRV binding switched to env/sky IBL textures.
Forward / Scene Shaders package/Shaders/Lighting.hlsl, package/Shaders/DistantTree.hlsl, package/Shaders/RunGrass.hlsl, package/Shaders/Particle.hlsl	Replace mul(DirectionalAmbient, ...) with SharedData::GetAmbient(...); introduce envIBLColor and DALCMode-driven env/sky composition; replace prior iblColor usage.
Fog / Misc shader integrations package/Shaders/Effect.hlsl, package/Shaders/ISSAOComposite.hlsl, package/Shaders/Water.hlsl, package/Shaders/Common/ShadowSampling.hlsli	Gate fog IBL by EnableIBL (remove InInterior exclusion), initialize ambient via GetAmbient, and move ambient IBL composition to mode-based env/sky logic.
Feature settings / UI src/Features/IBL.h, src/Features/IBL.cpp	Rename EnableDiffuseIBL → EnableIBL; remove EnableInterior/DiffuseIBLScale/IBLSaturation; add Env/Sky IBL Scale & Saturation and DALCMode; update UI, registration, and resource bindings.

Sequence Diagram(s)

sequenceDiagram
    participant CPU as State (CPU)
    participant CB as SharedData CB
    participant GPU as Shader
    participant SRVs as Env/Sky IBL SRVs

    CPU->>CB: DALC colors -> DALCToSH -> write AmbientSHR/G/B
    Note right of CB: SharedData::GetAmbient(normal) available
    GPU->>CB: call GetAmbient(normal)
    GPU->>SRVs: sample EnvIBL / SkyIBL (GetEnvIBL/GetSkyIBL)
    SRVs-->>GPU: envSample, skySample
    GPU->>GPU: compute IBL ratio (GetIBLRatio) / DALCMode branch
    GPU->>GPU: build envSpecular + skySpecular -> finalIrradiance
    GPU->>CB: apply finalIrradiance with other shading terms

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat: diffuse ibl #1054 — Earlier diffuse-IBL additions that this change refactors into Env/Sky IBL and SH-based ambient.
• fix(ibl): add ibl to reflection normalization #1528 — Overlapping refactor of IBL shader/resource bindings and deferred-composite IBL handling.
• fix: interior dynamic cubemap for non-deferred #1283 — Edits to DynamicCubemaps that overlap specular/IBL accumulation and finalIrradiance logic.

Suggested reviewers

• alandtse
• doodlum

Poem

🐇 I hopped through code where light used to bend,

DALC turned to SH and old paths met their end,
Env and Sky now waltz in every pixel's tune,
Ambient hums softly beneath a shader moon,
I nibbled a compile — behold, a brighter noon!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 2.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(ibl): revamp ibl and dalc sh' is directly related to the main changes in the pull request, which involve a comprehensive refactor of IBL (Image-Based Lighting) and DALC (directional ambient lighting color) with spherical harmonics integration.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Features/IBL.cpp (1)

12-23: ⚠️ Potential issue | 🟡 Minor

Clamp newly deserialized IBL settings before they reach runtime/shader state.

These new fields are user-editable via JSON, but LoadSettings() still trusts raw values. Invalid ranges for DALCMode, scales, saturation, or fog mix should be clamped after deserialization.

🔧 Proposed hardening

+#include <algorithm>
...
 void IBL::LoadSettings(json& o_json)
 {
 	settings = o_json;
+	settings.DALCAmount = std::clamp(settings.DALCAmount, 0.0f, 1.0f);
+	settings.EnvIBLScale = std::clamp(settings.EnvIBLScale, 0.0f, 10.0f);
+	settings.SkyIBLScale = std::clamp(settings.SkyIBLScale, 0.0f, 10.0f);
+	settings.EnvIBLSaturation = std::clamp(settings.EnvIBLSaturation, 0.0f, 2.0f);
+	settings.SkyIBLSaturation = std::clamp(settings.SkyIBLSaturation, 0.0f, 2.0f);
+	settings.FogAmount = std::clamp(settings.FogAmount, 0.0f, 1.0f);
+	settings.DALCMode = std::min<uint>(settings.DALCMode, 2u);
 }

As per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/IBL.cpp` around lines 12 - 23, After deserializing IBL::Settings
via NLOHMANN_DEFINE_TYPE... ensure LoadSettings() (or the function that applies
deserialized IBL::Settings) clamps all user-editable fields before they are
stored to runtime/shader state: enforce valid enums for DALCMode, clamp
EnvIBLScale and SkyIBLScale to sensible positive ranges, clamp EnvIBLSaturation
and SkyIBLSaturation to [0,1] (or project conventions), clamp FogAmount and
DALCAmount to [0,1], and ensure PreserveFogLuminance and
EnableDiffuseIBL/UseStaticIBL are interpreted as booleans; update the code path
that moves IBL::Settings into runtime (the loader/applier function called
LoadSettings or similar) to validate and coerce these values so malformed JSON
cannot reach shader/resource setup.

🧹 Nitpick comments (3)

features/IBL/Shaders/IBL/IBL.hlsli (1)

23-107: Consider consolidating SH texture fetches to reduce per-pixel overhead.

GetEnvIBL, GetSkyIBL, and GetIBLRatio each fetch the same SH texels repeatedly. A small helper that loads SH once and reuses it in the composition path would cut redundant texture reads.

As per coding guidelines "Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@features/IBL/Shaders/IBL/IBL.hlsli` around lines 23 - 107, Multiple functions
(GetEnvIBL, GetSkyIBL, GetIBLRatio) repeatedly call DiffuseIBLTexture.Load /
DiffuseSkyIBLTexture.Load for the same SH texels; create a small helper (e.g.,
LoadDiffuseSH or LoadSHTriplet) that takes a Texture2DArray (or implicit
texture) and returns/outputs the three sh2 components (shR, shG, shB) so callers
reuse those values instead of re-loading; update GetEnvIBL, GetSkyIBL, and
GetIBLRatio to call this helper once per texture (DiffuseIBLTexture and
DiffuseSkyIBLTexture) and then pass the loaded sh2 values into
SphericalHarmonics::SHHallucinateZH3Irradiance and subsequent math to eliminate
redundant texture reads while preserving existing logic and names.

package/Shaders/Water.hlsl (1)

1270-1270: Consider linking this feature PR to a tracking issue.

If there is a related GitHub issue, add Implements #... or Addresses #... in the PR description for traceability.

As per coding guidelines: "Issue References ... Suggest adding appropriate GitHub keywords."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@package/Shaders/Water.hlsl` at line 1270, The PR should include a GitHub
issue reference for traceability: update the PR description to add a keyworded
reference like "Implements `#1234`" or "Addresses `#1234`" pointing to the related
issue for the new diffuse IBL feature (the change enabling
SharedData::iblSettings.EnableDiffuseIBL in Water.hlsl) so reviewers and
tracking tools can link the code change to the issue.

src/Utils/SphericalHarmonics.h (1)

1-54: Optional PR metadata polish

Current title is valid, but a clearer alternative could be: feat(ibl): split env/sky ibl with dalc sh blending.
If this maps to a tracked issue, consider adding Addresses #<id> in the PR description.

As per coding guidelines "**/*: When reviewing PRs, please provide suggestions for Conventional Commit Titles ... and Issue References ...".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/SphericalHarmonics.h` around lines 1 - 54, Update the PR title and
description to follow Conventional Commits and include issue references: change
the PR title to a clearer form such as "feat(ibl): split env/sky ibl with dalc
sh blending" and, if this work maps to a tracked issue, add "Addresses #<id>" in
the PR description; mention the relevant feature (e.g., DALCToSH in
SphericalHarmonics) in the description for context.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@features/Dynamic` Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:
- Around line 114-122: The env-side DALC scaling is being multiplied by
skylightingSpecular before the final lerp, causing double attenuation of the
fallback; update the specular fallback calculation in the blocks that compute
specularIrradiance and specularIrradianceReflections to remove the additional
multiplication by skylightingSpecular (i.e., compute dalcScaled via
Color::IrradianceToGamma(...) *without* multiplying by skylightingSpecular) so
that the final blend lerp(specularIrradiance, specularIrradianceReflections,
skylightingSpecular) alone controls the mix; adjust both occurrences that
reference skylightingSpecular in those calculations.

In `@package/Shaders/Common/SharedData.hlsli`:
- Around line 27-30: The three boolean fields InInterior, InMapMenu, and HideSky
in the SharedData cbuffer are declared as HLSL bools but must be 32-bit uints to
match the CPU-side SharedDataCB (src/State.h) and preserve 4-byte sizing and
16-byte constant-buffer alignment; change the types of InInterior, InMapMenu,
and HideSky from bool to uint in SharedData.hlsli so their layout and sizes
match the CPU struct.

In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 236-245: The fallback path double-scales the environment term:
dalcScaled multiplies directionalAmbientColorSpecular by skylightingSpecular and
then finalIrradiance is lerped by skylightingSpecular again, effectively zeroing
the env fallback when skylightingSpecular is low. Fix by computing dalcScaled
from directionalAmbientColorSpecular without multiplying by skylightingSpecular
(i.e., remove the extra * skylightingSpecular when computing dalcScaled) so
EnvTexture.SampleLevel/ specularIrradiance normalization remains correct and the
final lerp(specularIrradiance, specularIrradianceReflections,
skylightingSpecular) preserves the intended blend between fallback and
reflections.

In `@src/Utils/SphericalHarmonics.cpp`:
- Around line 161-170: HanningConvolution computes invW = 1.0f / w without
validating w, which can produce Inf/NaN and corrupt SH coefficients; update the
function (SphericalHarmonics::HanningConvolution) to validate the convolution
width w before using it: if w <= 0 (or nearly zero) either return the original
SH2 unchanged or clamp w to a small positive epsilon, then compute invW and
factorBand1 and apply them; ensure any early-return or clamping avoids division
by zero and prevents propagating NaNs into result.c0 and result.c1.
- Around line 234-240: In SHHallucinateZH3Irradiance, guard the division by
sh.c0 before computing ratio: check fabs(sh.c0) against a small epsilon (e.g.
1e-6) and if it's below that threshold set ratio to 0 (or otherwise handle the
degenerate case) instead of performing ratio /= sh.c0; continue computing
zonalL2Coeff with the safe ratio so zonalL2Coeff = sh.c0 * (0.08f * ratio + 0.6f
* ratio * ratio) remains stable for near-zero sh.c0.
- Around line 208-211: The normalize(const T& v) function can divide by zero for
zero-length vectors and other code computes sqrt(1 - roughness) without
validating roughness; fix both by guarding against invalid inputs: in normalize,
compute length = sqrtf(v.Dot(v)), if length is <= FLT_EPSILON (or a small
epsilon) return a safe default (e.g., zero vector or v) instead of dividing,
otherwise multiply by 1.0f/length; for the roughness usage clamp the roughness
value into [0.0f, 1.0f] before computing sqrt(1.0f - roughness) (or use
max(0.0f, 1.0f - roughness)) to avoid negative arguments, and prefer using an
epsilon guard when taking sqrt to prevent NaNs; apply the same guards to all
related functions in this file that compute vector normalization or sqrt(1 -
roughness) (lines around the normalize function and the block referenced as
215-223).
- Around line 141-149: In SphericalHarmonics::Product the assignment to
result.c1[2] mistakenly duplicates shL.c1[2]*shR.c1[2]; replace that duplicated
term so c1 components follow the pairwise pattern: set result.c1[2] = factor *
(shL.c1[0] * shR.c1[0] + shL.c1[1] * shR.c1[1]) to correct the math and restore
consistent SH multiplication.

---

Outside diff comments:
In `@src/Features/IBL.cpp`:
- Around line 12-23: After deserializing IBL::Settings via
NLOHMANN_DEFINE_TYPE... ensure LoadSettings() (or the function that applies
deserialized IBL::Settings) clamps all user-editable fields before they are
stored to runtime/shader state: enforce valid enums for DALCMode, clamp
EnvIBLScale and SkyIBLScale to sensible positive ranges, clamp EnvIBLSaturation
and SkyIBLSaturation to [0,1] (or project conventions), clamp FogAmount and
DALCAmount to [0,1], and ensure PreserveFogLuminance and
EnableDiffuseIBL/UseStaticIBL are interpreted as booleans; update the code path
that moves IBL::Settings into runtime (the loader/applier function called
LoadSettings or similar) to validate and coerce these values so malformed JSON
cannot reach shader/resource setup.

---

Nitpick comments:
In `@features/IBL/Shaders/IBL/IBL.hlsli`:
- Around line 23-107: Multiple functions (GetEnvIBL, GetSkyIBL, GetIBLRatio)
repeatedly call DiffuseIBLTexture.Load / DiffuseSkyIBLTexture.Load for the same
SH texels; create a small helper (e.g., LoadDiffuseSH or LoadSHTriplet) that
takes a Texture2DArray (or implicit texture) and returns/outputs the three sh2
components (shR, shG, shB) so callers reuse those values instead of re-loading;
update GetEnvIBL, GetSkyIBL, and GetIBLRatio to call this helper once per
texture (DiffuseIBLTexture and DiffuseSkyIBLTexture) and then pass the loaded
sh2 values into SphericalHarmonics::SHHallucinateZH3Irradiance and subsequent
math to eliminate redundant texture reads while preserving existing logic and
names.

In `@package/Shaders/Water.hlsl`:
- Line 1270: The PR should include a GitHub issue reference for traceability:
update the PR description to add a keyworded reference like "Implements `#1234`"
or "Addresses `#1234`" pointing to the related issue for the new diffuse IBL
feature (the change enabling SharedData::iblSettings.EnableDiffuseIBL in
Water.hlsl) so reviewers and tracking tools can link the code change to the
issue.

In `@src/Utils/SphericalHarmonics.h`:
- Around line 1-54: Update the PR title and description to follow Conventional
Commits and include issue references: change the PR title to a clearer form such
as "feat(ibl): split env/sky ibl with dalc sh blending" and, if this work maps
to a tracked issue, add "Addresses #<id>" in the PR description; mention the
relevant feature (e.g., DALCToSH in SphericalHarmonics) in the description for
context.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a653c74 and fe22417.

📒 Files selected for processing (18)

• features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
• features/IBL/Shaders/IBL/IBL.hlsli
• package/Shaders/Common/ShadowSampling.hlsli
• package/Shaders/Common/SharedData.hlsli
• package/Shaders/DeferredCompositeCS.hlsl
• package/Shaders/DistantTree.hlsl
• package/Shaders/Effect.hlsl
• package/Shaders/ISSAOComposite.hlsl
• package/Shaders/Lighting.hlsl
• package/Shaders/Particle.hlsl
• package/Shaders/RunGrass.hlsl
• package/Shaders/Water.hlsl
• src/Features/IBL.cpp
• src/Features/IBL.h
• src/State.cpp
• src/State.h
• src/Utils/SphericalHarmonics.cpp
• src/Utils/SphericalHarmonics.h

[github-actions]

✅ A pre-release build is available for this PR:
Download

[coderabbitai]

🧹 Nitpick comments (3)

src/Utils/SphericalHarmonics.h (1)

1-3: Consider explicit DirectX header dependency

Line 2 uses DirectX::XMFLOAT3X3 without an explicit include. This works if a precompiled header provides DirectX types, but adding #include <DirectXMath.h> would make the dependency explicit and prevent compilation failures if include order changes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/SphericalHarmonics.h` around lines 1 - 3, Add an explicit include
for the DirectXMath header so the DirectX::XMFLOAT3X3 type is always available:
update SphericalHarmonics.h by adding `#include` <DirectXMath.h> before the alias
declaration (the line defining using float3x3 = DirectX::XMFLOAT3X3) to make the
dependency explicit and avoid relying on precompiled/header include order.

features/IBL/Shaders/IBL/IBL.hlsli (1)

22-44: Asymmetric clamping between GetEnvIBL and GetSkyIBL

GetSkyIBL (line 43) applies max(0, ...) to prevent negative irradiance values, but GetEnvIBL (line 31) does not. If this is intentional (e.g., EnvIBL is guaranteed non-negative from source data), consider adding a brief comment. Otherwise, add the same guard for consistency:

Proposed fix

 	float colorR = SphericalHarmonics::SHHallucinateZH3Irradiance(shR, rayDir);
 	float colorG = SphericalHarmonics::SHHallucinateZH3Irradiance(shG, rayDir);
 	float colorB = SphericalHarmonics::SHHallucinateZH3Irradiance(shB, rayDir);
-	return float3(colorR, colorG, colorB) / Math::PI;
+	return max(0, float3(colorR, colorG, colorB) / Math::PI);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@features/IBL/Shaders/IBL/IBL.hlsli` around lines 22 - 44, GetEnvIBL lacks the
non-negative clamp that GetSkyIBL applies; either apply the same guard to
GetEnvIBL by wrapping its returned float3 in max(0, ...) (matching GetSkyIBL) or
add a concise comment in GetEnvIBL explaining why negative values cannot occur
(source guarantees) so the asymmetry is intentional; update the GetEnvIBL
function (and its return expression) or add the comment directly above GetEnvIBL
to make the behavior explicit.

package/Shaders/DistantTree.hlsl (1)

247-260: Minor: Asymmetric clamping between deferred and non-deferred paths

The deferred path (line 247) applies max(0, ...) to directionalAmbientColor while the non-deferred path (line 288) does not. If this is intentional (e.g., deferred compositing requires non-negative ambient), consider adding a brief comment. Otherwise, consider making them consistent:

Option: Add clamping to non-deferred path

-	float3 directionalAmbientColor = Color::Ambient(SharedData::GetAmbient(normal));
+	float3 directionalAmbientColor = max(0, Color::Ambient(SharedData::GetAmbient(normal)));

Also applies to: 288-301

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@package/Shaders/DistantTree.hlsl` around lines 247 - 260, The deferred path
clamps directionalAmbientColor with max(0,
Color::Ambient(SharedData::GetAmbient(normal))) but the non-deferred/IBL branch
computes directionalAmbientColor without that clamp, producing asymmetric
behavior; either apply the same non-negative clamp when assigning
directionalAmbientColor in the non-deferred/IBL branch (use max(0, ...) around
the computed envIBLColor/skyIBLColor combination or around Color::Ambient(...)
if reusing that call) or, if the asymmetry is intentional, add a concise comment
next to the non-deferred branch mentioning why non-negative clamping is omitted
(reference variable directionalAmbientColor, Color::Ambient,
SharedData::GetAmbient(normal), and the IBL branch using
ImageBasedLighting::GetEnvIBLColor / GetSkyIBLColor).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@features/IBL/Shaders/IBL/IBL.hlsli`:
- Around line 22-44: GetEnvIBL lacks the non-negative clamp that GetSkyIBL
applies; either apply the same guard to GetEnvIBL by wrapping its returned
float3 in max(0, ...) (matching GetSkyIBL) or add a concise comment in GetEnvIBL
explaining why negative values cannot occur (source guarantees) so the asymmetry
is intentional; update the GetEnvIBL function (and its return expression) or add
the comment directly above GetEnvIBL to make the behavior explicit.

In `@package/Shaders/DistantTree.hlsl`:
- Around line 247-260: The deferred path clamps directionalAmbientColor with
max(0, Color::Ambient(SharedData::GetAmbient(normal))) but the non-deferred/IBL
branch computes directionalAmbientColor without that clamp, producing asymmetric
behavior; either apply the same non-negative clamp when assigning
directionalAmbientColor in the non-deferred/IBL branch (use max(0, ...) around
the computed envIBLColor/skyIBLColor combination or around Color::Ambient(...)
if reusing that call) or, if the asymmetry is intentional, add a concise comment
next to the non-deferred branch mentioning why non-negative clamping is omitted
(reference variable directionalAmbientColor, Color::Ambient,
SharedData::GetAmbient(normal), and the IBL branch using
ImageBasedLighting::GetEnvIBLColor / GetSkyIBLColor).

In `@src/Utils/SphericalHarmonics.h`:
- Around line 1-3: Add an explicit include for the DirectXMath header so the
DirectX::XMFLOAT3X3 type is always available: update SphericalHarmonics.h by
adding `#include` <DirectXMath.h> before the alias declaration (the line defining
using float3x3 = DirectX::XMFLOAT3X3) to make the dependency explicit and avoid
relying on precompiled/header include order.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 37376c33-e40b-4493-ae7d-e53523679934

📥 Commits

Reviewing files that changed from the base of the PR and between fe22417 and 277461b.

📒 Files selected for processing (19)

• features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
• features/IBL/Shaders/IBL/IBL.hlsli
• package/Shaders/Common/ShadowSampling.hlsli
• package/Shaders/Common/SharedData.hlsli
• package/Shaders/DeferredCompositeCS.hlsl
• package/Shaders/DistantTree.hlsl
• package/Shaders/Effect.hlsl
• package/Shaders/ISSAOComposite.hlsl
• package/Shaders/Lighting.hlsl
• package/Shaders/Particle.hlsl
• package/Shaders/RunGrass.hlsl
• package/Shaders/Water.hlsl
• src/Deferred.cpp
• src/Features/IBL.cpp
• src/Features/IBL.h
• src/State.cpp
• src/State.h
• src/Utils/SphericalHarmonics.cpp
• src/Utils/SphericalHarmonics.h

🚧 Files skipped from review as they are similar to previous changes (5)

• package/Shaders/ISSAOComposite.hlsl
• package/Shaders/Particle.hlsl
• src/State.cpp
• package/Shaders/Common/SharedData.hlsli
• package/Shaders/Common/ShadowSampling.hlsli

[coderabbitai]

♻️ Duplicate comments (3)

src/Utils/SphericalHarmonics.cpp (3)

209-223: ⚠️ Potential issue | 🟠 Major

Guard normalization and roughness domain to avoid NaN propagation.

Line 212 can divide by zero for zero-length vectors, and Line 221 can evaluate sqrt(1 - roughness) with invalid input when roughness is outside [0,1].

Proposed fix

 template <typename T>
 static T normalize(const T& v)
 {
-	return v * (1.0f / sqrtf(v.Dot(v)));
+	const float len2 = v.Dot(v);
+	if (len2 <= 1e-8f) {
+		return v;
+	}
+	return v * (1.0f / sqrtf(len2));
 }
@@
 SH2 SphericalHarmonics::FauxSpecularLobe(float3 N, float3 V, float roughness)
 {
+	roughness = std::clamp(roughness, 0.0f, 1.0f);
 	// https://www.gdcvault.com/play/1026701/Fast-Denoising-With-Self-Stabilizing
 	// get dominant ggx reflection direction
 	float f = (1 - roughness) * (sqrt(1 - roughness) + roughness);

As per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/SphericalHarmonics.cpp` around lines 209 - 223, The normalize
function and SphericalHarmonics::FauxSpecularLobe need defensive checks: in
normalize(const T& v) check v.Dot(v) (or v.LengthSq()) and if it is <= epsilon
return a safe fallback (e.g. zero vector or unit-axis) instead of dividing by
zero, and in FauxSpecularLobe clamp the roughness input to [0,1] before
computing f (use max(min(roughness,1.0f),0.0f)) and guard the sqrt(1 -
roughness) call with that clamped value; additionally, after computing D or
using normalize(R*... + N*...), ensure the vector passed to normalize is not
zero and handle that case (early return a sensible SH2 default or use N) to
prevent NaN propagation.

---

236-242: ⚠️ Potential issue | 🟠 Major

Add an epsilon guard before dividing by sh.c0.

Line 241 can blow up ratio for dark/degenerate SH inputs and destabilize irradiance output.

Proposed fix

 float SphericalHarmonics::SHHallucinateZH3Irradiance(SH2 sh, float3 direction)
 {
+	if (std::abs(sh.c0) <= 1e-8f) {
+		return 0.0f;
+	}
 	float3 zonalAxis = normalize(float3(sh.c1[2], sh.c1[0], sh.c1[1]));
 	float ratio = 0.0;
 	ratio = abs(zonalAxis.Dot(float3(-sh.c1[2], -sh.c1[0], sh.c1[1])));
 	ratio /= sh.c0;

As per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/SphericalHarmonics.cpp` around lines 236 - 242, In
SphericalHarmonics::SHHallucinateZH3Irradiance, guard the division by sh.c0 when
computing ratio: before doing ratio /= sh.c0, test fabs(sh.c0) against a small
epsilon (e.g. 1e-6) and if it's below the threshold treat sh.c0 as degenerate
(set ratio = 0 or use a safe fallback) so zonalL2Coeff is computed stably; apply
the same epsilon check anywhere sh.c0 is used as a divisor to avoid blowups for
dark/degenerate SH inputs.

---

141-149: ⚠️ Potential issue | 🟠 Major

Fix the duplicated term in Product output.

Line 148 repeats the same term twice, which breaks the component pattern used by Line 146–147 and can skew SH multiplication results.

Proposed fix

 SH2 SphericalHarmonics::Product(SH2 shL, SH2 shR)
 {
 	const float factor = 1.0f / (2.0f * sqrt(3.14159265358979323846f));
 	SH2 result;
 	result.c0 = factor * Dot(shL, shR);
 	result.c1[0] = factor * (shL.c1[0] * shR.c1[2] + shL.c1[2] * shR.c1[0]);
 	result.c1[1] = factor * (shL.c1[1] * shR.c1[2] + shL.c1[2] * shR.c1[1]);
-	result.c1[2] = factor * (shL.c1[2] * shR.c1[2] + shL.c1[2] * shR.c1[2]);
+	result.c1[2] = factor * (shL.c1[0] * shR.c1[1] + shL.c1[1] * shR.c1[0]);
 	return result;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/SphericalHarmonics.cpp` around lines 141 - 149,
SphericalHarmonics::Product currently assigns result.c1[2] using the same term
twice (shL.c1[2] * shR.c1[2]), which is a copy/paste bug; update result.c1[2] in
the SH2 Product function to use the correct cross-pairing expression: factor *
(shL.c1[0] * shR.c1[1] + shL.c1[1] * shR.c1[0]) so the three c1 components
follow the same pairing pattern as result.c1[0] and result.c1[1].

🧹 Nitpick comments (1)

src/Utils/SphericalHarmonics.cpp (1)

1-1: Consider adding an issue reference in the PR description.

Since this is a feature PR, linking the tracked issue (for example, Implements #<id> or Addresses #<id>) improves traceability.

As per coding guidelines "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/SphericalHarmonics.cpp` at line 1, Add a GitHub issue reference to
the PR description for traceability by including a keyword such as "Implements
#<id>" or "Addresses #<id>" that links this feature change touching
SphericalHarmonics (see src/Utils/SphericalHarmonics.cpp and
SphericalHarmonics.h) to the tracked issue; update the PR title/body to include
the appropriate keyword and issue number so the merge will auto-close or
reference the issue per project guidelines.

🤖 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/Utils/SphericalHarmonics.cpp`:
- Around line 209-223: The normalize function and
SphericalHarmonics::FauxSpecularLobe need defensive checks: in normalize(const
T& v) check v.Dot(v) (or v.LengthSq()) and if it is <= epsilon return a safe
fallback (e.g. zero vector or unit-axis) instead of dividing by zero, and in
FauxSpecularLobe clamp the roughness input to [0,1] before computing f (use
max(min(roughness,1.0f),0.0f)) and guard the sqrt(1 - roughness) call with that
clamped value; additionally, after computing D or using normalize(R*... +
N*...), ensure the vector passed to normalize is not zero and handle that case
(early return a sensible SH2 default or use N) to prevent NaN propagation.
- Around line 236-242: In SphericalHarmonics::SHHallucinateZH3Irradiance, guard
the division by sh.c0 when computing ratio: before doing ratio /= sh.c0, test
fabs(sh.c0) against a small epsilon (e.g. 1e-6) and if it's below the threshold
treat sh.c0 as degenerate (set ratio = 0 or use a safe fallback) so zonalL2Coeff
is computed stably; apply the same epsilon check anywhere sh.c0 is used as a
divisor to avoid blowups for dark/degenerate SH inputs.
- Around line 141-149: SphericalHarmonics::Product currently assigns
result.c1[2] using the same term twice (shL.c1[2] * shR.c1[2]), which is a
copy/paste bug; update result.c1[2] in the SH2 Product function to use the
correct cross-pairing expression: factor * (shL.c1[0] * shR.c1[1] + shL.c1[1] *
shR.c1[0]) so the three c1 components follow the same pairing pattern as
result.c1[0] and result.c1[1].

---

Nitpick comments:
In `@src/Utils/SphericalHarmonics.cpp`:
- Line 1: Add a GitHub issue reference to the PR description for traceability by
including a keyword such as "Implements #<id>" or "Addresses #<id>" that links
this feature change touching SphericalHarmonics (see
src/Utils/SphericalHarmonics.cpp and SphericalHarmonics.h) to the tracked issue;
update the PR title/body to include the appropriate keyword and issue number so
the merge will auto-close or reference the issue per project guidelines.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f66af0d3-1d03-49c8-897a-32a699ab354d

📥 Commits

Reviewing files that changed from the base of the PR and between 277461b and 85aad88.

📒 Files selected for processing (1)

• src/Utils/SphericalHarmonics.cpp