chore: pbr consistency changes

[doodlum]

• Fixed first person cubemaps
• Removed multibounce ao
• Fixed energy conservation
• Vanilla specular closer to PBR
• Removed vertex horizon ao

Summary by CodeRabbit

• Bug Fixes

  • Removed horizon occlusion and BRDF-based horizon scaling for consistent specular and ambient AO.
  • Fixed hair diffuse attenuation edge-case to avoid artifacts.
  • Deferred composite now uses screen-space AO in SSGI paths and returns simplified specular irradiance in deferred paths.

• Chores / Refactor

  • Simplified dynamic cubemap/specular interfaces and removed vertex-normal–based horizon paths.
  • Streamlined indirect/coating specular to rely on base F0 and direct AO.
  • Updated gamma conversion exponents, adjusted reflection normalization scale, and added reflection fresnel roughness constant.
  • Kept specular color in irradiance space for grass lighting.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Removed VN/horizon-based occlusion from dynamic cubemaps and indirect/specular paths, changed DynamicCubemaps APIs to drop VN, simplified specular outputs (DEFERRED → 1.0, non-DEFERRED → F0-based), adjusted gamma/exponent and reflection constants, and threaded outputAlbedo into ambient/IBL/skylighting flows.

Changes

Cohort / File(s)	Summary
Dynamic Cubemaps (specular) features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli	Removed horizon occlusion and VN usage; updated public signatures to drop VN; DEFERRED branches return constant 1.0; non-DEFERRED return F0-based irradiance.
PBR / Lighting evaluation package/Shaders/Common/PBR.hlsli, package/Shaders/Common/LightingEval.hlsli, package/Shaders/Common/PBRMath.hlsli	Removed EnvBRDF/horizon-driven specular scaling and horizon occlusion; indirect/specular lobe weights simplified to material.F0; removed VN param from wetness specular; added hair diffuse divide-by-zero guard.
Color / Gamma & reflection constants package/Shaders/Common/Color.hlsli	Gamma exponents changed to 2.2/1/2.2; ReflectionNormalisationScale non-LL branch set to 0.65; added ReflectionFresnelRoughness = 0.7.
Deferred composite / reflectance & AO package/Shaders/DeferredCompositeCS.hlsl	Included BRDF.hlsli; replaced MultiBounceAO with ssgiAo; compute V = -normalize(positionWS.xyz) earlier; apply BRDF::EnvBRDFApproxLazarov to reflectance; use V for specular lobe.
Lighting / ambient & albedo flow package/Shaders/Lighting.hlsl	Smoothed rawRMAOS.w and used it for F0 interpolation; updated DynamicCubemaps call sites to omit vertexNormal; introduced outputAlbedo and applied it to ambient/Skylighting/IBL; consolidated gamma/irradiance conversion steps.
Grass specular handling package/Shaders/RunGrass.hlsl	Removed IrradianceToLinear call on accumulated specularColor; specular remains in irradiance space.

Sequence Diagram(s)

sequenceDiagram
    participant Lighting as Lighting.hlsl
    participant DynCube as DynamicCubemaps.hlsli
    participant BRDF as BRDF.hlsli
    participant Deferred as DeferredCompositeCS.hlsl

    Lighting->>DynCube: GetDynamicCubemapSpecularIrradiance(N, V, roughness, skylightingSH)
    DynCube->>BRDF: compute F0-based irradiance (horizon/VN removed)
    DynCube-->>Lighting: specularIrradiance (F0 * finalIrradiance or 1.0 in DEFERRED)
    Lighting->>BRDF: GetIndirectLobeWeights(material, N, V)
    Lighting->>Lighting: outputAlbedo = indirectLobeWeights.diffuse * vertexColor
    Lighting->>Deferred: pass ambient, outputAlbedo, specular contributions
    Deferred->>BRDF: BRDF::EnvBRDFApproxLazarov applied to reflectance
    Deferred-->>Lighting: composed lighting with ssgiAo and outputAlbedo applied

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• refactor: move BRDFs to separate file #1161 — Introduced/centralized BRDF helpers (BRDF.hlsli), directly related to replacing previous EnvBRDF usage.
• chore(dynamic cubemaps): lessen normalisation darkening #1833 — Modifies ReflectionNormalisationScale in Color.hlsli, overlapping this PR's constant change.
• refactor: restructure lighting #1633 — Removes horizon-occlusion/VN-based specular adjustments and updates DynamicCubemaps/lighting signatures (strong overlap).

Suggested reviewers

• jiayev
• alandtse

Poem

🐇 I nudged the horizon quiet, let F0 softly gleam,
Cubemaps hum simpler now beneath the moonbeam.
Albedo tucked in warm, ambient finds its tune,
Constants tuned, carrots sparkle — lighting afternoon. 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title 'chore: pbr consistency changes' is directly related to the changeset, which focuses on PBR (physically-based rendering) consistency improvements including linear space conversions, fresnel handling, and cubemap application.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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: 1

🤖 Fix all issues with AI agents

In `@package/Shaders/Common/PBR.hlsli`:
- Around line 246-249: The division 1 / (specularBRDF.x) can produce Inf/NaN
when specularBRDF.x≈0; update the computation around lobeWeights.specular (using
lobeWeights, specularBRDF, material.F0) to guard the reciprocal by using a small
epsilon (e.g., reciprocal = 1.0 / max(specularBRDF.x, kSpecularEpsilon) or
reciprocal = rsqrt(max(specularBRDF.x, kSpecularEpsilon)) depending on desired
precision) and apply the same guard to the coat path (the coat-related weight
calculation around the same area, e.g., coatWeights or coat lobe computation) so
no division uses a near-zero denominator; define a named constant like
kSpecularEpsilon and reuse it for consistency.

🧹 Nitpick comments (2)

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

79-97: Consider using GammaCorrectionValue instead of hardcoded 2.2.

Line 19 defines static float GammaCorrectionValue = 2.2; but these functions hardcode 2.2 directly. Using the variable would keep the gamma exponent in one place and avoid drift if it's ever tuned again.

Note: GammaToTrueLinear / TrueLinearToGamma (lines 99–107) also hardcode 2.2, so they'd remain the canonical "true 2.2" path if you wanted GammaCorrectionValue to be adjustable.

---

111-111: Dead ternary — both branches return 1.0.

ENABLE_LL ? 1.0 : 1.0 is always 1.0. If the non-linear-lighting downscale is intentionally removed, simplify to a plain constant (same applies to ReflectionNormalisationScale on line 114). Keeping the ternary gives the false impression the value is mode-dependent.

Suggested simplification

-	const static float PBRLightingScale = ENABLE_LL ? 1.0 : 1.0;
+	const static float PBRLightingScale = 1.0;
 
 	// Attempt to normalise reflection brightness against DALC
-	const static float ReflectionNormalisationScale = ENABLE_LL ? 1.0 : 1.0;
+	const static float ReflectionNormalisationScale = 1.0;

[coderabbitai]

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (1)

features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (1)

28-29: ⚠️ Potential issue | 🟡 Minor

Horizon occlusion is still cubed here vs. squared in PBR.hlsli.

As noted in the PBR.hlsli review, GetIndirectLobeWeights now uses horizon² while both functions in this file still use horizon³. If this is intentional (e.g., cubemaps need stronger horizon occlusion), a brief comment would help; otherwise, aligning the exponent would improve consistency per the PR's stated goal.

Also applies to: 157-158

🤖 Fix all issues with AI agents

In `@package/Shaders/Common/PBR.hlsli`:
- Around line 266-269: DynamicCubemaps.hlsli currently applies horizon cubed
where other shaders use horizon squared; update the occlusion multiplier in the
DynamicCubemaps code that multiplies lobeWeights.specular (the section using the
local horizon variable) to use horizon * horizon instead of horizon * horizon *
horizon so it matches PBR.hlsli, PBRMath.hlsli, and LightingEval.hlsli; if the
cubemap logic intentionally requires a different exponent, add a clear comment
explaining why and link to the relevant rationale.
- Around line 271-274: The code incorrectly swizzles Color::MultiBounceAO to .y,
losing per-channel AO; change the types from float to float3 for diffuseAO and
specularAO and remove the .y swizzle so you keep the full RGB result from
Color::MultiBounceAO when called with material.BaseColor/material.F0 and AO
inputs (keep using Color::SpecularAOLagarde(NdotV, material.AO,
material.Roughness) as the specular AO input); ensure subsequent uses of
diffuseAO/specularAO handle float3 values (or convert to a luminance scalar
explicitly if a single-channel value is truly needed).

🧹 Nitpick comments (1)

features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (1)

151-151: NoV is now unused after removing EnvBRDF.

NoV on line 151 was only consumed by the removed BRDF::EnvBRDF(roughness, NoV) call. It's now dead code in GetDynamicCubemap.

Suggested removal

 		float3 R = reflect(-V, N);
-		float NoV = saturate(dot(N, V));
 
 		float level = roughness * 7.0;

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@package/Shaders/Lighting.hlsl`:
- Around line 2102-2108: The calculation for rawRMAOS.w is using material.F0
which is still 0, so rawRMAOS.w always becomes 0 and removes dielectric Fresnel;
update the code so rawRMAOS.w is derived from the original specular/sample value
(e.g., the texture sample variable that was assigned to rawRMAOS before F0 was
initialized, or a new sampledSpecular variable) or use a proper dielectric F0
(e.g., Color::GammaToTrueLinear(baseColor.xyz) or a 0.04 constant) instead of
material.F0; ensure the lerp/smoothstep call that writes rawRMAOS.w references
that original sampled specular/dielectricF0 and then keep the later material.F0
assignment logic (the lerp into material.F0) unchanged, touching rawRMAOS.w,
material.F0, smoothstep, and the branch on
SharedData::linearLightingSettings.enableLinearLighting.

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (1)

package/Shaders/Lighting.hlsl (1)

3016-3019: ⚠️ Potential issue | 🟠 Major

Non-dynamic-cubemap specular fallback uses albedo-modulated directionalAmbientColor.

After the changes above, directionalAmbientColor has been multiplied by outputAlbedo (line 2984). So the fallback indirectLobeWeights.specular * directionalAmbientColor now includes an albedo modulation that wasn't there before. For specular reflections, the incident light (ambient) shouldn't be pre-multiplied by diffuse albedo — specular response is governed by F0/Fresnel, not diffuse color. This could darken specular highlights on dark materials or tint them incorrectly in the non-dynamic-cubemap path.

🧹 Nitpick comments (1)

package/Shaders/Lighting.hlsl (1)

2103-2104: lerp(0.0, 1.0, x) simplifies to just x.

lerp(0.0, 1.0, smoothstep(0.04, 1.0, rawRMAOS.w)) is mathematically identical to smoothstep(0.04, 1.0, rawRMAOS.w). The lerp is a no-op here.

Simplification

-	rawRMAOS.w = lerp(0.0, 1.0, smoothstep(0.04, 1.0, rawRMAOS.w));
+	rawRMAOS.w = smoothstep(0.04, 1.0, rawRMAOS.w);

[github-actions]

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

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@package/Shaders/Common/Color.hlsli`:
- Around line 116-117: Fix the typo in the comment above the
ReflectionFresnelRoughness constant: change "Roughnss" to "Roughness" in the
comment that documents const static float ReflectionFresnelRoughness = 0.7; so
the comment correctly reads "Roughness value for fresnel reflections".

In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 162-164: The N·V sign is inverted because V =
normalize(positionWS.xyz) points camera→surface; update the Fresnel call so
NdotV uses the view direction from surface→camera (i.e. negate V) before
saturating: compute viewDirection = -normalize(positionWS.xyz) (or use -V) and
pass saturate(dot(normalWS, viewDirection)) into BRDF::EnvBRDFApproxLazarov when
computing reflectance.xyz with Color::ReflectionFresnelRoughness to restore
correct angular variation.

🧹 Nitpick comments (2)

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

79-97: Gamma functions now duplicate GammaToTrueLinear/TrueLinearToGamma.

With the exponent changed to hardcoded 2.2, GammaToLinear and LinearToGamma are now functionally identical to GammaToTrueLinear and TrueLinearToGamma (lines 99–107). Also, GammaCorrectionValue on line 19 is now unused. Consider removing the duplicates or the dead constant in a follow-up.

package/Shaders/Lighting.hlsl (1)

2102-2112: rawRMAOS.w Fresnel reduction for non-metals looks correct now (past issue resolved).

The smoothstep mapping on rawRMAOS.w zeroes out Fresnel for specular levels below 0.04 (typical dielectric F0) and ramps to full for metallic values. The subsequent lerp with material.Metallic properly blends between dielectric and metallic F0.

Minor simplification: lerp(0.0, 1.0, smoothstep(...)) is identity — it equals smoothstep(0.04, 1.0, rawRMAOS.w).

Optional simplification

-	rawRMAOS.w = lerp(0.0, 1.0, smoothstep(0.04, 1.0, rawRMAOS.w));
+	rawRMAOS.w = smoothstep(0.04, 1.0, rawRMAOS.w);

[addididi]

Testing it out with my textures it does help resolve the issue with metal inconsistency, but the tradeoff is visually leaving PBR in a much worse state overall. The textures look much flatter and a huge amount of quality from the previous PBR rendering is lost, it basically removes any advantage PBR has quality wise and leaves only workflow advantages. Which from a user side of things means PBR basically has no purpose over CM.

meh

[jiayev]

this also

[jiayev]

this