fix(pbr): fix improper kD terms

[doodlum]

Summary by CodeRabbit

• Bug Fixes

  • Separated diffuse and specular contributions for more physically consistent lighting.
  • Applied consistent non-metallic (kD) attenuation across diffuse, subsurface transmission, and indirect lighting.
  • Fixed lighting color for fine-fuzz/glint so detailed light color is used for accurate highlights.

• Refactor

  • Reworked multi-layer coat weighting and attenuation for improved layered material appearance.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors PBR lighting in package/Shaders/Common/PBR.hlsli to separate specular computation, introduce kD = 1 - F attenuation across diffuse, subsurface, coat, and indirect paths, and rename/update coat-related intermediates; replaces context light color usage for fuzz lighting with detailedLightColor.

Changes

Cohort / File(s)	Summary
PBR Shader Core package/Shaders/Common/PBR.hlsli	Rework direct/specular accumulation to compute specular via microfacet functions, introduce kD = 1 - F for diffuse attenuation, propagate kD into subsurface transmission and two-layer coat paths, rename coatSpecularLobeWeight → coatSpecularLobeSpecular, adjust coat diffuse/specular attenuation and lobe weights, and switch fuzz lighting to use detailedLightColor.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• chore: pbr consistency changes #1841: Overlapping edits to PBR.hlsli around diffuse/specular separation and coat attenuation.
• refactor: restructure lighting #1633: Modifies the same PBR lighting codepaths and lobe weight logic.
• fix(pbr): fix inconsistencies #1526: Related changes to diffuse/specular/AO and lobe handling in PBR.hlsli.

Suggested reviewers

• jiayev

Poem

🐰 I hopped through code at break of day,
Kneading kD where speculars play,
Coats and subsurfaces now align,
Fuzz lit by detail, colors fine,
A tiny hop — the shader's gay.

🚥 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 'fix(pbr): fix improper kD terms' directly relates to the main changes in the PR, which involve correcting how kD (diffuse attenuation) terms are applied throughout the PBR shader calculations.
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
• Commit unit tests in branch pbr-fixes-3

📝 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.

[github-actions]

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

[coderabbitai]

🧹 Nitpick comments (2)

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

162-165: Clamp attenuation terms to avoid negative lobe energy in edge cases.

1 - F/1 - specular style terms can go slightly negative with out-of-range material inputs or approximation error. Clamping these attenuation factors makes the shader more robust.

Proposed robustness diff

-			float3 kD = 1 - F;
+			float3 kD = saturate(1 - F);
...
-			float3 kD = 1 - lobeWeights.specular;
+			float3 kD = saturate(1 - lobeWeights.specular);
...
-				float3 layerAttenuation = 1 - coatSpecularLobeSpecular * material.CoatStrength;
+				float3 layerAttenuation = saturate(1 - coatSpecularLobeSpecular * material.CoatStrength);
...
-					float3 coatDiffuseLobeWeight = material.CoatColor * (1 - coatSpecularLobeSpecular);
+					float3 coatDiffuseLobeWeight = material.CoatColor * saturate(1 - coatSpecularLobeSpecular);

Also applies to: 245-246, 254-255, 260-260

🤖 Prompt for AI Agents

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

In `@package/Shaders/Common/PBR.hlsli` around lines 162 - 165, Clamp the
diffuse/specular attenuation factors to prevent negative energy: replace direct
uses of expressions like kD = 1 - F and any 1 - specular terms with a clamped
value (e.g., kD = saturate(1 - F) or equivalent) before they are multiplied into
lightingOutput.diffuse/specular; update occurrences around the kD assignment and
the lines that add to lightingOutput.diffuse/specular (the lines using
BRDF::Diffuse_Lambert(), satNdotL, detailedLightColor and specular), and apply
the same clamping to the other listed locations (around lines handling 1 -
specular at the other occurrences) so all attenuation factors are bounded
non-negative.

---

1-1: Consider adding an issue-closing keyword in the PR description (if applicable).

If this bug fix maps to a tracked issue, add Fixes #<id> or Closes #<id> for traceability.

As per coding guidelines, bug-fix PRs should include GitHub issue keywords like Fixes #123`` when applicable.

🤖 Prompt for AI Agents

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

In `@package/Shaders/Common/PBR.hlsli` at line 1, Summary: PR description is
missing an issue-closing keyword; update the PR description to include a GitHub
keyword like "Fixes #<id>" or "Closes #<id>" if this change addresses a tracked
issue. Edit the pull request description (not code) to append the appropriate
keyword referencing the issue number so the PR will automatically close the
issue when merged; ensure the keyword is on its own line and references the
correct issue ID.

🤖 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/PBR.hlsli`:
- Around line 162-165: Clamp the diffuse/specular attenuation factors to prevent
negative energy: replace direct uses of expressions like kD = 1 - F and any 1 -
specular terms with a clamped value (e.g., kD = saturate(1 - F) or equivalent)
before they are multiplied into lightingOutput.diffuse/specular; update
occurrences around the kD assignment and the lines that add to
lightingOutput.diffuse/specular (the lines using BRDF::Diffuse_Lambert(),
satNdotL, detailedLightColor and specular), and apply the same clamping to the
other listed locations (around lines handling 1 - specular at the other
occurrences) so all attenuation factors are bounded non-negative.
- Line 1: Summary: PR description is missing an issue-closing keyword; update
the PR description to include a GitHub keyword like "Fixes #<id>" or "Closes
#<id>" if this change addresses a tracked issue. Edit the pull request
description (not code) to append the appropriate keyword referencing the issue
number so the PR will automatically close the issue when merged; ensure the
keyword is on its own line and references the correct issue ID.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ab37298d-6f9c-47f0-86c1-ae3d66d26852

📥 Commits

Reviewing files that changed from the base of the PR and between 73c91ec and 3b38b90.

📒 Files selected for processing (1)

• package/Shaders/Common/PBR.hlsli