fix(hair): use indirect normal for deferred marschner hair

[jiayev]

now uses normalize(V- T * dot(V, T)) as screenSpaceNormal for marschner to make indirect diffuse look better

Summary by CodeRabbit

Release Notes

• Performance

  • Optimized hair specular lighting calculations to skip unnecessary computations in certain rendering modes.

• Bug Fixes

  • Improved hair tangent shift and screen-space normal calculations for more consistent hair appearance across different rendering conditions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Two shader files receive conditional guards for HairMode == 1 specular path handling. The first adds an early return in GetHairDynamicCubemapSpecularIrradiance to skip specular irradiance calculations. The second hardens tangent shift logic to avoid applying shifts when HairMode equals 1 and updates screen-space normal handling via ambientNormal recalculation in this mode.

Changes

Cohort / File(s)	Summary
Hair Specular Cubemap Guard features/Hair Specular/Shaders/Hair/Hair.hlsli	Added early return when hairSpecularSettings.HairMode == 1 to bypass specular irradiance computations; existing roughnessPrimary logic unaffected.
Hair Tangent Shift & Normal Handling package/Shaders/Lighting.hlsl	Tightened tangent shift application to exclude HairMode 1; added ambientNormal-based screen-space normal recalculation path for hair mode to ensure normal consistency.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Verify the early return condition in GetHairDynamicCubemapSpecularIrradiance doesn't unintentionally skip necessary initialization
• Confirm tangent shift conditional logic correctly gates the shift application when HairMode == 1
• Review ambientNormal recalculation path and ensure screen-space normal transformation is mathematically correct
• Validate no edge cases exist where the normal-handling fallback causes visual artifacts

Possibly related PRs

• refactor(hair): match marschner and vanilla color #1240 — Directly modifies hair-specular paths for HairMode == 1 with dynamic cubemap roughness and primary tangent shift disabling.
• fix(hair): correct hair indirect normal, marschner by default #1515 — Modifies hair-specular dynamic-cubemap logic for HairMode == 1 scenarios with similar normal/tangent handling adjustments.
• feat(hair): hair self-shadow, finished marschner model and hair flow map support #1225 — Introduced the HairMode switch and Marschner-specific branches that these conditional guards build upon.

Suggested reviewers

• doodlum
• alandtse
• Pentalimbed

Poem

🐰 A guard for the hair, when mode's set to one—
Skip the specular dance, let the tangents run true,
AmbientNormal whispers its screen-space song,
Hopping through shaders, the logic holds strong! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 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.
Title check	✅ Passed	The title 'fix(hair): use indirect normal for deferred marschner hair' accurately describes the main change: replacing the screen-space normal with an indirect normal calculation for the Marschner hair model, which aligns with the PR's stated objective.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

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

---

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]

Using provided base ref: d7675dd
Using base ref: d7675dd
Base commit date: 2025-12-06T02:18:02Z (Saturday, December 06, 2025 02:18 AM)
No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

features/Hair Specular/Shaders/Hair/Hair.hlsli (2)

329-343: Early‑out for HairMode 1 is correct but leaves some now‑dead branches

Short‑circuiting GetHairDynamicCubemapSpecularIrradiance when HairMode == 1 is a safe guard and matches how Marschner is used (no indirect specular from the dynamic cubemap). Given the current call site already skips this function for HairMode == 1, this branch is mainly defensive.

You now have a few HairMode == 1 conditionals in this function that will never be reached if the early‑return triggers, e.g.:

• const float roughnessPrimary = SharedData::hairSpecularSettings.HairMode == 1 ? 1.0 : ...;
• The HairMode == 1 ? 0.0 : SharedData::hairSpecularSettings.PrimaryTangentShift inside ShiftNormal.

If you want to tighten things up, you could simplify those to the non‑Marschner paths (or drop them entirely if the call‑site guard is considered authoritative), but it’s not functionally required.

---

8-14: PR meta: conventional‑commit title and issue reference suggestion

The current title already follows conventional‑commit style but is on the long side. You might consider shortening it to stay within ~50 characters, e.g.:

• fix(hair): improve marschner indirect normal

If this change fully resolves a tracked bug, also consider adding an explicit issue reference in the PR description, for example:

• Fixes #<issue-id> for a complete fix
• Related to #<issue-id> if it’s only part of a larger effort

package/Shaders/Lighting.hlsl (2)

2101-2105: Tangent‑shifted screenSpaceNormal now only applies when hair spec is enabled and HairMode != 1

The extra guards here make sense:

• hairSpecularSettings.Enabled ensures we don’t pay the cost of sampling and shifting normals when the hair specular feature is off.
• HairMode != 1 avoids re‑orienting the normal when running in Marschner mode, which now has its own ambientNormal handling below.

One subtle point: because screenSpaceNormal is later reassigned from ambientNormal in the HAIR/CS_HAIR block (Line 2726), this shifted normal no longer reaches the final GBuffer normal for hair; it only matters for any logic that reads screenSpaceNormal between this block and that later override.

If you still want Kajiya–Kay hair (HairMode 0) to expose a tangent‑shifted normal to deferred consumers, you may want to revisit that later assignment; otherwise this block may be effectively dead outside of any intermediate use.

---

2722-2727: ambientNormal → screenSpaceNormal override changes GBuffer normals for all hair modes

Here you always set:

float3 ambientNormal = worldNormal;
#if defined(HAIR) && defined(CS_HAIR)
if (SharedData::hairSpecularSettings.Enabled && SharedData::hairSpecularSettings.HairMode == 1)
    ambientNormal = normalize(viewDirection - hairT * dot(viewDirection, hairT));
screenSpaceNormal = normalize(FrameBuffer::WorldToView(ambientNormal, false, eyeIndex));
#endif

Implications:

• For Marschner (HairMode == 1), screenSpaceNormal now uses the new normalize(V - T * dot(V, T))-based direction, which matches the PR goal.
• For non‑Marschner hair (e.g. Kajiya–Kay) and for cases where hair spec is disabled, ambientNormal stays equal to worldNormal, and this line overwrites any tangent‑shifted screenSpaceNormal computed earlier. So the final GBuffer normal for hair is always worldNormal (or the Marschner vector), regardless of EnableTangentShift.

If your intent was to change the screen‑space normal only for Marschner, consider gating the assignment to that mode, e.g.:

-#if defined(HAIR) && defined(CS_HAIR)
-if (SharedData::hairSpecularSettings.Enabled && SharedData::hairSpecularSettings.HairMode == 1)
-    ambientNormal = normalize(viewDirection - hairT * dot(viewDirection, hairT));
-    screenSpaceNormal = normalize(FrameBuffer::WorldToView(ambientNormal, false, eyeIndex));
-#endif
+#if defined(HAIR) && defined(CS_HAIR)
+if (SharedData::hairSpecularSettings.Enabled && SharedData::hairSpecularSettings.HairMode == 1) {
+    ambientNormal = normalize(viewDirection - hairT * dot(viewDirection, hairT));
+    screenSpaceNormal = normalize(FrameBuffer::WorldToView(ambientNormal, false, eyeIndex));
+}
+#endif

That would preserve the existing tangent‑shifted screenSpaceNormal behavior for Kajiya–Kay while still giving Marschner the new approximation. If the broader change (always tying screenSpaceNormal to ambientNormal for hair) is intentional, it’d be good to confirm and maybe leave a brief comment in the code to document it.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7675dd and 240ba80.

📒 Files selected for processing (2)

• features/Hair Specular/Shaders/Hair/Hair.hlsli (1 hunks)
• package/Shaders/Lighting.hlsl (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

• features/Hair Specular/Shaders/Hair/Hair.hlsli
• package/Shaders/Lighting.hlsl

features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)

Files:

• features/Hair Specular/Shaders/Hair/Hair.hlsli

**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

1. Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
   if the existing title does not describe the code changes):
   Format: type(scope): description
   Length: 50 characters limit for title, 72 for body
   Style: lowercase description, no ending period
   Examples:

   • feat(vr): add cross-eye sampling
   • fix(water): resolve flowmap bug
   • docs: update shader documentation

2. Issue References (if PR fixes bugs or implements features):
   Suggest adding appropriate GitHub keywords:

   • "Fixes #123" or "Closes #123" for bug fixes
   • "Implements #123" or "Addresses #123" for features
   • "Related to #123" for partial implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

• features/Hair Specular/Shaders/Hair/Hair.hlsli
• package/Shaders/Lighting.hlsl

🧠 Learnings (5)

📓 Common learnings

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.

📚 Learning: 2025-08-17T18:37:35.839Z

Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)

Applied to files:

• package/Shaders/Lighting.hlsl

📚 Learning: 2025-08-03T18:37:19.690Z

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.

Applied to files:

• package/Shaders/Lighting.hlsl

📚 Learning: 2025-08-17T18:37:35.839Z

Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/

Applied to files:

• package/Shaders/Lighting.hlsl

📚 Learning: 2025-06-17T05:40:22.785Z

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.

Applied to files:

• package/Shaders/Lighting.hlsl

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
• GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
• GitHub Check: Build plugin and addons

[github-actions]

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