fix(ao): better ao calculations

[jiayev]

source:
https://docs.google.com/spreadsheets/d/1qKRTn53eDupSDrC7O1_O8p9zQv-G8Ck6v7xGL4XPc9E/edit?usp=sharing

Summary by CodeRabbit

• Refactor

  • Ambient occlusion and ambient lighting now use a multi-bounce AO approximation and specular AO handling was reworked for more consistent, realistic shading.

• New Features

  • Added shading utilities to better approximate multi-bounce ambient interactions and refined specular occlusion behavior.

• Tests

  • Updated and expanded shader tests to validate multi-bounce AO, specular occlusion, and luminance variants.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds Common/Shading.hlsli with MultiBounceAO, SpecularAOLagarde, and SpecularOcclusion; removes SpecularAOLagarde from Color.hlsli; updates Skylighting, PBR, and DeferredComposite shaders to use MultiBounceAO/SpecularOcclusion and adjust ambient/diffuse gamma placement; adds unit tests for the new shading helpers.

Changes

Cohort / File(s)	Summary
Shading utilities package/Shaders/Common/Shading.hlsli	New header adding MultiBounceAO(float3, float), SpecularAOLagarde(...), and SpecularOcclusion(...) with include guards.
Color utilities package/Shaders/Common/Color.hlsli	Removed SpecularAOLagarde implementation; callers updated to use the new Shading helpers.
Skylighting feature features/Skylighting/Shaders/Skylighting/Skylighting.hlsli	Included Common/Shading.hlsli; replaced skylighting diffuse multiplier with MultiBounceAO(...) and moved gamma conversion after multiplication.
PBR / Deferred shading package/Shaders/Common/PBR.hlsli, package/Shaders/DeferredCompositeCS.hlsl	Added include of Common/Shading.hlsli; diffuse AO uses MultiBounceAO(baseColor, AO), specular occlusion uses SpecularOcclusion(NdotV, roughness^2, AO); adjusted AO application points and gamma handling.
Tests package/Shaders/Tests/TestShading.hlsl, package/Shaders/Tests/TestColor.hlsl	Added TestShading.hlsl with unit tests for MultiBounceAO, SpecularAOLagarde, and SpecularOcclusion; removed TestSpecularAOLagarde from TestColor.hlsl and added luminance-variant tests.

Sequence Diagram(s)

sequenceDiagram
  participant Material as Material (BaseColor, AO, Roughness)
  participant Shading as Common/Shading (MultiBounceAO, SpecularOcclusion)
  participant Skylight as Skylighting / SSGI
  participant Lighting as PBR / DeferredComposite

  Material->>Shading: multiBounce = MultiBounceAO(BaseColor, AO)
  Material->>Shading: specOccl = SpecularOcclusion(NdotV, Roughness^2, AO)
  Skylight->>Lighting: directionalAmbientColor, skylightingDiffuse
  Lighting->>Lighting: linAmbient = IrradianceToLinear(directionalAmbientColor)
  Lighting->>Lighting: ambientTerm = IrradianceToGamma(linAmbient * multiBounce)
  Lighting->>Lighting: diffuse *= sqrt(multiBounce)
  Lighting->>Lighting: specular *= specOccl

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• fix(pbr): fix inconsistencies #1526: Refactors shading/AO utilities and updates usages across skylighting and deferred shading (moves/introduces MultiBounceAO/SpecularOcclusion).
• test: add hlsl unit tests #1653: Introduces Common/Shading helpers and updates shader call sites/test coverage for the new functions.
• perf(ssgi): optimize  #1499: Modifies skylighting/ambient AO and deferred composite AO/specular code paths similar to these changes.

Suggested reviewers

• alandtse

Poem

🐰 I nibbled at code beneath the moon,

MultiBounce hops made shadows tune,
Specular lagarde hopped into a shared lair,
Tests now sing — pixels fluff the air,
Tiny hops — renderings hum in tune.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'fix(ao): better ao calculations' is vague and uses non-descriptive phrasing that doesn't clearly convey what specific improvements were made to AO calculations.	Use a more specific title that describes the actual changes, such as 'refactor(ao): introduce multi-bounce and specular occlusion calculations' or 'feat(ao): add MultiBounceAO and SpecularOcclusion functions'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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]

🧹 Nitpick comments (1)

package/Shaders/Tests/TestColor.hlsl (1)

137-151: Consider adding tests for Shading.hlsli utilities.

The removal of TestSpecularAOLagarde is appropriate since the function moved to Shading.hlsli. However, there are no corresponding tests for the newly introduced MultiBounceAO and SpecularAOLagarde functions in Shading.hlsli.

Would you like me to draft a TestShading.hlsl test file covering MultiBounceAO and SpecularAOLagarde with boundary conditions (ao=0, ao=1) and typical values?

🤖 Prompt for AI Agents

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

In `@package/Shaders/Tests/TestColor.hlsl` around lines 137 - 151, Add a new
TestShading.hlsl test kernel that exercises the Shading.hlsli utilities
MultiBounceAO and SpecularAOLagarde: create a [numthreads(1,1,1)] test function
that calls MultiBounceAO and SpecularAOLagarde with boundary inputs (ao = 0 and
ao = 1) and typical values (e.g., ao = 0.5) and with extreme roughness/metalness
where applicable; use ASSERT(IsTrue, ...) to verify outputs are valid
(non-negative, within expected [0,1] or physically-plausible ranges) and check
specific invariants (e.g., SpecularAOLagarde returns no greater energy than
input, MultiBounceAO decreases/influences occlusion monotonically), so the tests
mirror the style in existing TestColor.hlsl and reference the functions
MultiBounceAO and SpecularAOLagarde for locating the code.

🤖 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/Tests/TestColor.hlsl`:
- Around line 137-151: Add a new TestShading.hlsl test kernel that exercises the
Shading.hlsli utilities MultiBounceAO and SpecularAOLagarde: create a
[numthreads(1,1,1)] test function that calls MultiBounceAO and SpecularAOLagarde
with boundary inputs (ao = 0 and ao = 1) and typical values (e.g., ao = 0.5) and
with extreme roughness/metalness where applicable; use ASSERT(IsTrue, ...) to
verify outputs are valid (non-negative, within expected [0,1] or
physically-plausible ranges) and check specific invariants (e.g.,
SpecularAOLagarde returns no greater energy than input, MultiBounceAO
decreases/influences occlusion monotonically), so the tests mirror the style in
existing TestColor.hlsl and reference the functions MultiBounceAO and
SpecularAOLagarde for locating the code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8261469a-2096-40fb-8e6f-3b43bad6ca5d

📥 Commits

Reviewing files that changed from the base of the PR and between e57f5cd and d626404.

📒 Files selected for processing (6)

• features/Skylighting/Shaders/Skylighting/Skylighting.hlsli
• package/Shaders/Common/Color.hlsli
• package/Shaders/Common/PBR.hlsli
• package/Shaders/Common/Shading.hlsli
• package/Shaders/DeferredCompositeCS.hlsl
• package/Shaders/Tests/TestColor.hlsl

💤 Files with no reviewable changes (1)

• package/Shaders/Common/Color.hlsli

[github-actions]

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

[alandtse]

Please post the screenshot showing it matches the reference better here. That way it's documented in time and not accidentally lost in the sheets.

[alandtse]

Do we need tests for the new shading functions?

[jiayev]

Do we need tests for the new shading functions?

sure

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

package/Shaders/Tests/TestShading.hlsl (1)

34-38: Consider adding a comment explaining the 1.01 upper bound.

The MultiBounceAO function can return values slightly above 1.0 (by design of the multi-bounce approximation), unlike SpecularAOLagarde which uses saturate() and is bounded to [0, 1]. A brief comment here would clarify why the tolerance differs between the two test functions.

 	float3 bounded = MultiBounceAO(baseColorBright, 0.6f);
 	ASSERT(IsTrue, all(!isnan(bounded)));
 	ASSERT(IsTrue, all(!isinf(bounded)));
 	ASSERT(IsTrue, all(bounded >= 0.6f));
+	// MultiBounceAO can slightly exceed 1.0 by design (multi-bounce energy approximation)
 	ASSERT(IsTrue, all(bounded <= 1.01f));

🤖 Prompt for AI Agents

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

In `@package/Shaders/Tests/TestShading.hlsl` around lines 34 - 38, Add a short
inline comment above the ASSERT(IsTrue, all(bounded <= 1.01f))) explaining that
MultiBounceAO can produce values slightly >1.0 by design (the multi‑bounce
approximation allows small overshoot), and contrast this with SpecularAOLagarde
which uses saturate() and is clamped to [0,1]; place the comment next to the
bounded variable / the ASSERT line referencing MultiBounceAO and the differing
behavior of SpecularAOLagarde for clarity.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@package/Shaders/Tests/TestShading.hlsl`:
- Around line 41-43: The function declaration for TestSpecularAOLagarde has
extra leading indentation causing a formatting inconsistency with
TestMultiBounceAO; edit the TestSpecularAOLagarde declaration so its indentation
matches TestMultiBounceAO (remove the unexpected leading spaces before
"[numthreads(1, 1, 1)] void TestSpecularAOLagarde()") to align formatting with
the rest of the file.

---

Nitpick comments:
In `@package/Shaders/Tests/TestShading.hlsl`:
- Around line 34-38: Add a short inline comment above the ASSERT(IsTrue,
all(bounded <= 1.01f))) explaining that MultiBounceAO can produce values
slightly >1.0 by design (the multi‑bounce approximation allows small overshoot),
and contrast this with SpecularAOLagarde which uses saturate() and is clamped to
[0,1]; place the comment next to the bounded variable / the ASSERT line
referencing MultiBounceAO and the differing behavior of SpecularAOLagarde for
clarity.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 405cffb1-d016-4e4c-8819-0e0ae6c57df4

📥 Commits

Reviewing files that changed from the base of the PR and between d626404 and b2e1d4b.

📒 Files selected for processing (1)

• package/Shaders/Tests/TestShading.hlsl

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Formatting inconsistency: extra indentation before function.

Lines 41-42 have unexpected leading indentation compared to TestMultiBounceAO at line 11-12. The PR notes that pre-commit formatting was applied, so this may have been missed.

-	/// `@tags` shading, ao, specular
-	[numthreads(1, 1, 1)] void TestSpecularAOLagarde()
+/// `@tags` shading, ao, specular
+[numthreads(1, 1, 1)] void TestSpecularAOLagarde()
 {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/// @tags shading, ao, specular
	[numthreads(1, 1, 1)] void TestSpecularAOLagarde()
	{
	/// `@tags` shading, ao, specular
	[numthreads(1, 1, 1)] void TestSpecularAOLagarde()
	{

🤖 Prompt for AI Agents

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

In `@package/Shaders/Tests/TestShading.hlsl` around lines 41 - 43, The function
declaration for TestSpecularAOLagarde has extra leading indentation causing a
formatting inconsistency with TestMultiBounceAO; edit the TestSpecularAOLagarde
declaration so its indentation matches TestMultiBounceAO (remove the unexpected
leading spaces before "[numthreads(1, 1, 1)] void TestSpecularAOLagarde()") to
align formatting with the rest of the file.

[coderabbitai]

🧹 Nitpick comments (4)

package/Shaders/DeferredCompositeCS.hlsl (1)

146-152: Clamp AO input before multi-bounce/sqrt for robustness.

A small saturate on ssgiAo before MultiBounceAO helps avoid unstable values propagating into sqrt(...) if upstream AO ever overshoots.

Proposed change

-	float3 multiBounceSSGIAo = MultiBounceAO(linAlbedo, ssgiAo);
+	float ssgiAoClamped = saturate(ssgiAo);
+	float3 multiBounceSSGIAo = MultiBounceAO(linAlbedo, ssgiAoClamped);

-	linDiffuseColor *= sqrt(multiBounceSSGIAo);
+	linDiffuseColor *= sqrt(max(multiBounceSSGIAo, 0.0));

🤖 Prompt for AI Agents

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

In `@package/Shaders/DeferredCompositeCS.hlsl` around lines 146 - 152, Clamp the
AO input before computing multi-bounce and before sqrt: call MultiBounceAO with
a saturated ssgiAo (e.g., use saturate(ssgiAo)) and also ensure
multiBounceSSGIAo is clamped (saturate or max(0, ...)) before using sqrt to
avoid negative/overshoot values; update the computation around MultiBounceAO,
multiBounceSSGIAo, the sqrt(linDiffuseColor) multiplication, and the final
Color::IrradianceToGamma multiplication that uses directionalAmbientColor to use
the clamped AO value.

package/Shaders/Common/Shading.hlsli (2)

1-2: PR hygiene: add an issue keyword if this maps to a tracked item.

If this PR implements/fixes a tracked issue, add Fixes #..., Closes #..., or Implements #... in the PR description for automatic linkage.

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 `@package/Shaders/Common/Shading.hlsli` around lines 1 - 2, This PR feedback is
asking for PR hygiene: if this change implements or fixes a tracked issue,
update the PR description to include an appropriate GitHub keyword such as
"Fixes #<issue>", "Closes #<issue>", or "Implements #<issue>" so it will
auto-link; locate the relevant change (e.g., the header guarded by
__SHADING_HLSLI__ in Shaders/Common/Shading.hlsli) and, if it corresponds to an
issue, add the keyword and issue number to the PR description before merging.

---

19-22: Consider optional hardening of SpecularOcclusion with clamped inputs.

The current implementation is safe: all call sites (DeferredCompositeCS.hlsl:59, PBR.hlsli:272) saturate NdotV at the source, and material.AO is expected to be in [0, 1]. However, moving the clamping into the helper makes the function more robust to future callers.

Suggested optional improvement

 float SpecularOcclusion(float NdotV, float alpha, float occlusion)
 {
-	return saturate(pow(NdotV + occlusion, alpha) - 1.0 + occlusion);
+	float ao = saturate(occlusion);
+	float base = max(0.0, NdotV + ao);
+	return saturate(pow(base, alpha) - 1.0 + ao);
 }

🤖 Prompt for AI Agents

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

In `@package/Shaders/Common/Shading.hlsli` around lines 19 - 22, SpecularOcclusion
currently assumes callers clamp NdotV and occlusion; make it robust by clamping
NdotV and occlusion inside the function (use saturate on NdotV and occlusion)
and ensure the pow() base is non-negative before raising to alpha (e.g., compute
a clampedBase = max(0, saturatedNdotV + saturatedOcclusion) and use that in
pow), then apply the existing -1.0 + occlusion adjustment (using the saturated
occlusion value) and finally saturate the result before returning to preserve
output in [0,1].

package/Shaders/Tests/TestShading.hlsl (1)

73-118: Add one stress case for out-of-range inputs to catch NaN regressions.

Consider a negative-base/domain stress assertion for SpecularOcclusion (and optionally SpecularAOLagarde) so future changes don’t reintroduce pow-domain instability.

Proposed addition

 [numthreads(1, 1, 1)] void TestSpecularOcclusion() {
+	float stress = SpecularOcclusion(-0.2f, 0.5f, -0.1f);
+	ASSERT(IsTrue, !isnan(stress));
+	ASSERT(IsTrue, !isinf(stress));
+
 	float occlusionZero = SpecularOcclusion(0.35f, 0.5f, 0.0f);
 	ASSERT(IsTrue, abs(occlusionZero) < TestConstants::EXACT_TOLERANCE);

🤖 Prompt for AI Agents

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

In `@package/Shaders/Tests/TestShading.hlsl` around lines 73 - 118, Add a stress
test that calls SpecularOcclusion with an out-of-range/negative-base input
(e.g., negative base or domain that could make the internal pow() see a negative
value) and assert the result is finite and clamped to [0,1]; update the
TestSpecularOcclusion() function to include one extra sample like this and
assert !isnan, !isinf and >=0.0f and <=1.0f for it; optionally add the same
single stress case for SpecularAOLagarde() to catch future pow-domain
regressions.

🤖 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/Common/Shading.hlsli`:
- Around line 1-2: This PR feedback is asking for PR hygiene: if this change
implements or fixes a tracked issue, update the PR description to include an
appropriate GitHub keyword such as "Fixes #<issue>", "Closes #<issue>", or
"Implements #<issue>" so it will auto-link; locate the relevant change (e.g.,
the header guarded by __SHADING_HLSLI__ in Shaders/Common/Shading.hlsli) and, if
it corresponds to an issue, add the keyword and issue number to the PR
description before merging.
- Around line 19-22: SpecularOcclusion currently assumes callers clamp NdotV and
occlusion; make it robust by clamping NdotV and occlusion inside the function
(use saturate on NdotV and occlusion) and ensure the pow() base is non-negative
before raising to alpha (e.g., compute a clampedBase = max(0, saturatedNdotV +
saturatedOcclusion) and use that in pow), then apply the existing -1.0 +
occlusion adjustment (using the saturated occlusion value) and finally saturate
the result before returning to preserve output in [0,1].

In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 146-152: Clamp the AO input before computing multi-bounce and
before sqrt: call MultiBounceAO with a saturated ssgiAo (e.g., use
saturate(ssgiAo)) and also ensure multiBounceSSGIAo is clamped (saturate or
max(0, ...)) before using sqrt to avoid negative/overshoot values; update the
computation around MultiBounceAO, multiBounceSSGIAo, the sqrt(linDiffuseColor)
multiplication, and the final Color::IrradianceToGamma multiplication that uses
directionalAmbientColor to use the clamped AO value.

In `@package/Shaders/Tests/TestShading.hlsl`:
- Around line 73-118: Add a stress test that calls SpecularOcclusion with an
out-of-range/negative-base input (e.g., negative base or domain that could make
the internal pow() see a negative value) and assert the result is finite and
clamped to [0,1]; update the TestSpecularOcclusion() function to include one
extra sample like this and assert !isnan, !isinf and >=0.0f and <=1.0f for it;
optionally add the same single stress case for SpecularAOLagarde() to catch
future pow-domain regressions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cd3a4948-b153-49f5-8205-a0c2afa36b40

📥 Commits

Reviewing files that changed from the base of the PR and between b2e1d4b and ddf5258.

📒 Files selected for processing (4)

• package/Shaders/Common/PBR.hlsli
• package/Shaders/Common/Shading.hlsli
• package/Shaders/DeferredCompositeCS.hlsl
• package/Shaders/Tests/TestShading.hlsl

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

• package/Shaders/Common/PBR.hlsli

[coderabbitai]

🧹 Nitpick comments (1)

package/Shaders/Common/Shading.hlsli (1)

19-22: Consider documenting the alpha parameter meaning.

The alpha parameter represents roughness² (per PBR convention), but this isn't immediately clear from the function signature alone. A brief comment would help future maintainers understand the expected input.

📝 Suggested documentation

+// alpha = roughness^2 (perceptual roughness squared)
 float SpecularOcclusion(float NdotV, float alpha, float occlusion)
 {
 	return saturate(pow(abs(NdotV + occlusion), alpha) - 1.0 + occlusion);
 }

🤖 Prompt for AI Agents

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

In `@package/Shaders/Common/Shading.hlsli` around lines 19 - 22, The
SpecularOcclusion function's parameter alpha isn't documented; add a brief
comment above SpecularOcclusion in Shading.hlsli stating that alpha represents
roughness squared (alpha = roughness^2 per PBR convention), its expected range
(0..1), and any units/assumptions so maintainers know to pass squared roughness
rather than linear roughness when calling SpecularOcclusion(NdotV, alpha,
occlusion).

🤖 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/Common/Shading.hlsli`:
- Around line 19-22: The SpecularOcclusion function's parameter alpha isn't
documented; add a brief comment above SpecularOcclusion in Shading.hlsli stating
that alpha represents roughness squared (alpha = roughness^2 per PBR
convention), its expected range (0..1), and any units/assumptions so maintainers
know to pass squared roughness rather than linear roughness when calling
SpecularOcclusion(NdotV, alpha, occlusion).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cc42ade3-ba49-4004-806d-35f6e9bba13f

📥 Commits

Reviewing files that changed from the base of the PR and between ddf5258 and fa702f1.

📒 Files selected for processing (1)

• package/Shaders/Common/Shading.hlsli

outdated