fix(pbr): use GTSO, remove ao^2 mult with direct lighting

[soda3000]

Summary by CodeRabbit

• Refactor
  • Updated specular occlusion calculations in shader rendering pipeline.
  • Simplified diffuse lighting calculations for improved efficiency.

[coderabbitai]

📝 Walkthrough

Walkthrough

This change introduces a new specular occlusion function SpecularOcclusionGTSO based on the Jimenez et al. 2016 reference and replaces existing SpecularOcclusion calls with it across shader files. Additionally, diffuse color scaling by multibounce AO is removed from the SSGI sampling path.

Changes

Cohort / File(s)	Summary
New Specular Occlusion Function package/Shaders/Common/Shading.hlsli	Added SpecularOcclusionGTSO(float NdotV, float ao) function that computes saturate((NdotV + ao)² - 1.0 + ao) with Jimenez et al. 2016 reference documentation.
Specular Occlusion Function Replacement package/Shaders/Common/PBR.hlsli, package/Shaders/DeferredCompositeCS.hlsl	Replaced calls to SpecularOcclusion with the new SpecularOcclusionGTSO function, removing explicit roughness/alpha parameter dependency. Removed diffuse color scaling by sqrt(multiBounceSSGIAo) in SSGI path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(ao): better ao calculations #1968: Modifies the same shader areas (PBR GetIndirectLobeWeights and DeferredCompositeCS SampleSSGISpecular) to change how specular/diffuse AO is computed, with this PR replacing its SpecularOcclusion usage.
• fix(pbr): fix improper kD terms #1971: Both PRs modify indirect lobe weighting logic in PBR.hlsli, swapping SpecularOcclusion with SpecularOcclusionGTSO.
• chore: pbr consistency changes #1841: Both PRs modify specular occlusion and ambient-occlusion handling in shader code, changing how AO and multibounce are utilized.

Suggested reviewers

• jiayev
• doodlum
• alandtse

Poem

🐰 A specular hop through Jimenez's way,
New occlusion formula brightens the day,
No roughness to fuss, just NdotV and ao,
The shaders dance lighter, watch specularness glow! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly summarizes the main changes: replacing SpecularOcclusion with GTSO and removing AO squared multiplication from direct lighting calculations across multiple shader files.
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 (2)

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

13-18: PR title and issue linkage can be tightened to match repo conventions.

Consider shortening/lowercasing the title and adding a tracker reference in the PR description.
Suggested title: fix(pbr): use gtso, drop ao2 direct-light mult
If applicable, add: Fixes #<issue> / Closes #<issue>.

As per coding guidelines, use type(scope): description with lowercase style and include GitHub issue keywords when the PR fixes a bug/feature.

🤖 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 13 - 18, Update the PR
title and description to follow repo convention: change the title to the
suggested lowercase typed form (e.g., "fix(pbr): use gtso, drop ao2 direct-light
mult") and add an issue tracker reference in the body using GitHub keywords like
"Fixes #<issue>" or "Closes #<issue>" if this change to SpecularOcclusionGTSO
(function SpecularOcclusionGTSO in Shading.hlsli) resolves a tracked bug; ensure
the PR description includes the scope "pbr", the concise description, and the
issue reference.

---

13-18: Add shader test coverage for SpecularOcclusionGTSO.

Current provided test context (package/Shaders/Tests/TestShading.hlsl:74-109) validates only the legacy alpha-based SpecularOcclusion(...) behavior. Please add direct assertions for the new GTSO path to prevent regressions.

🤖 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 13 - 18, The tests only
cover the legacy SpecularOcclusion path and are missing direct assertions for
the new GTSO implementation; add explicit unit-style test cases in
TestShading.hlsl that call SpecularOcclusionGTSO with representative NdotV and
ao inputs (including edge cases like 0, 1, mid-range and combinations that
exercise the saturate and quadratic behavior) and assert expected outputs (or
delta within epsilon) to prevent regressions; locate the test additions
alongside the existing SpecularOcclusion checks in TestShading.hlsl and use the
same test harness/assert macros so failures surface consistently.

🤖 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 13-18: Update the PR title and description to follow repo
convention: change the title to the suggested lowercase typed form (e.g.,
"fix(pbr): use gtso, drop ao2 direct-light mult") and add an issue tracker
reference in the body using GitHub keywords like "Fixes #<issue>" or "Closes
#<issue>" if this change to SpecularOcclusionGTSO (function
SpecularOcclusionGTSO in Shading.hlsli) resolves a tracked bug; ensure the PR
description includes the scope "pbr", the concise description, and the issue
reference.
- Around line 13-18: The tests only cover the legacy SpecularOcclusion path and
are missing direct assertions for the new GTSO implementation; add explicit
unit-style test cases in TestShading.hlsl that call SpecularOcclusionGTSO with
representative NdotV and ao inputs (including edge cases like 0, 1, mid-range
and combinations that exercise the saturate and quadratic behavior) and assert
expected outputs (or delta within epsilon) to prevent regressions; locate the
test additions alongside the existing SpecularOcclusion checks in
TestShading.hlsl and use the same test harness/assert macros so failures surface
consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dfa9e1ab-9e52-4ebe-b9df-427373ab748c

📥 Commits

Reviewing files that changed from the base of the PR and between 2a3afa2 and e697d75.

📒 Files selected for processing (3)

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

[github-actions]

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