feat(hair): hair self-shadow, finished marschner model and hair flow map support#1225
Conversation
WalkthroughThis update introduces a selectable hair shading model (Kajiya-Kay or Marschner) configurable via UI and shader code. It adds screen-space self-shadowing with tunable parameters. Tangent reorientation and shifting utilities are implemented, and shader lighting functions are refactored to support the new features and parameters, including gamma correction and clamping. A new game constants header is also added for unit conversions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Settings
participant Shader
User->>UI: Select hair mode / adjust self-shadow parameters
UI->>Settings: Update Settings struct
Settings->>Shader: Pass updated parameters (HairMode, SelfShadow, etc.)
Shader->>Shader: Compute tangent shift, reorient tangent
Shader->>Shader: If self-shadow enabled, compute HairSelfShadow
Shader->>Shader: Select lighting model (Kajiya-Kay or Marschner)
Shader-->>UI: Rendered hair with selected model and self-shadowing
Possibly related issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsfeatures/Hair Specular/Shaders/Hair/Hair.hlsli (8)🔇 Additional comments (7)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Features/HairSpecular.cpp (1)
28-44: Well-structured UI with mode-specific controls.The conditional glossiness range based on hair mode is a thoughtful touch. The tooltips provide excellent context for users.
Consider removing the trailing newline in the tooltip text at line 35 for consistency:
- "Note that colors in Marschner mode may appear darker and more saturated.\n" + "Note that colors in Marschner mode may appear darker and more saturated."features/Hair Specular/Shaders/Hair/Hair.hlsli (1)
290-325: Screen-space self-shadowing implementation looks good.The raymarching algorithm is well-structured with proper early exit conditions and boundary checks. The use of noise for jittering helps reduce banding artifacts.
Consider making the step count configurable via shader constants for quality/performance tuning:
- const int stepCount = 4; + const int stepCount = SharedData::hairSpecularSettings.SelfShadowStepCount; // Add to settings
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (4)
features/Hair Specular/Shaders/Hair/Hair.hlsli(7 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)src/Features/HairSpecular.cpp(2 hunks)src/Features/HairSpecular.h(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
package/Shaders/Common/SharedData.hlsli (3)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
src/Features/HairSpecular.cpp (1)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
src/Features/HairSpecular.h (3)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
features/Hair Specular/Shaders/Hair/Hair.hlsli (1)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms (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
🔇 Additional comments (8)
package/Shaders/Common/SharedData.hlsli (1)
163-168: LGTM! Proper struct alignment for GPU constant buffer.The new members are correctly added with appropriate types following the project's convention of using
uintfor boolean flags. Theuint3 padensures proper 16-byte alignment for GPU constant buffers.src/Features/HairSpecular.h (1)
37-57: Structure correctly mirrors shader-side implementation.The Settings struct properly maintains alignment with
alignas(16)and includes matching members for the shader constant buffer. The default values are reasonable for the new hair shading features.src/Features/HairSpecular.cpp (2)
5-23: Serialization correctly extended with new fields.All new settings fields are properly included in the serialization macro.
64-72: Self-shadowing controls properly implemented.The UI section for self-shadowing is well-organized with appropriate parameter ranges and helpful tooltips.
features/Hair Specular/Shaders/Hair/Hair.hlsli (4)
7-19: Utility functions correctly implement tangent manipulation.The
ReorientTangentfunction properly orthogonalizes the tangent vector, and the constants are appropriately defined for hair shading calculations.
44-57: Normal shifting functions correctly implement anisotropic shading.The cross product calculations properly compute shifted normals for anisotropic hair lighting.
161-173: Marschner model implementation follows the paper correctly.The three-lobe computation (R, TT, TRT) is properly implemented with appropriate Fresnel and absorption calculations.
Note that
pow(abs(baseColor), ...)is used in lines 161 and 169. Verify that baseColor is always positive, or document why abs() is necessary here.
216-225: Inconsistent gamma correction between Scheuermann and Marschner branchesOnly the Marschner branch applies
dirDiffuse = Color::LinearToGamma(dirDiffuse);
dirSpecular = Color::LinearToGamma(dirSpecular);
(Hair.hlsli lines 222–223), while the Scheuermann branch returns linear values without any gamma conversion.• Scheuermann output (linear):
features/Hair Specular/Shaders/Hair/Hair.hlsli, GetHairDirectLightScheuermann (lines 61–76)
• Marschner output (gamma-corrected):
features/Hair Specular/Shaders/Hair/Hair.hlsli, call site (lines 219–224)Consider either:
- Applying
Color::LinearToGammato bothdirDiffuseanddirSpecularin the Scheuermann branch for consistency.- Adding a comment explaining why only Marschner requires gamma correction.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/Hair Specular/Shaders/Hair/Hair.hlsli (1)
289-324: Well-implemented screen-space self-shadowing with good performance characteristics.The
HairSelfShadowfunction provides a solid self-shadowing implementation:
- Early exit when disabled improves performance
- 4-step raymarching strikes a good balance between quality and performance
- Noise-based ray offset reduces banding artifacts
- Proper coordinate space transformations and bounds checking
- Exponential falloff with configurable strength provides smooth shadows
Minor suggestion: Consider if the minimum scale clamp of
0.05(line 300) might be too restrictive for certain artistic needs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/Hair Specular/Shaders/Hair/Hair.hlsli(7 hunks)src/Features/HairSpecular.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/HairSpecular.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
features/Hair Specular/Shaders/Hair/Hair.hlsli (8)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Learnt from: soda3000
PR: doodlum/skyrim-community-shaders#1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (9)
features/Hair Specular/Shaders/Hair/Hair.hlsli (9)
7-8: Constants look good for this PR scope.The
HAIR_LIGHTING_MULTIPLIERusingMath::PIis appropriate for compensating the vanilla lighting model. TheGAME_UNIT_TO_CMconstant is fine to keep here given the existing plan in issue #1228 to consolidate game-specific constants into a sharedGame.hlslifile.
14-19: Well-implemented tangent reorientation.The
ReorientTangentfunction correctly implements Gram-Schmidt orthogonalization to ensure the tangent is orthogonal to the normal. The implementation is mathematically sound and follows standard practices.
44-57: Tangent and normal shifting functions are mathematically correct.Both
ShiftNormalandShiftWorldNormalproperly implement tangent shifting and subsequent normal computation using cross products. The texture sampling inShiftWorldNormalwith the -0.5 offset correctly centers the shift range around zero.
61-63: Self-shadow integration is correctly implemented.The addition of the
selfShadowparameter and its early application tolightColorensures consistent shadowing across both diffuse and specular components. The modification maintains the existing function logic while adding the new self-shadowing capability.
173-173: Good addition of clamping to prevent negative values.The
max(R + TT + TRT, 0)clamp ensures the Marschner model doesn't produce negative color values, which is important for stability and physical correctness.
192-192: Consistent clamping in diffuse attenuation.The
max(S, 0)clamp inGetHairDiffuseAttenuationKajiyaKaymaintains consistency with the Marschner model clamping and prevents negative diffuse values.
195-223: Well-integrated Marschner model with proper gamma correction.The
GetHairDirectLightMarschnerfunction properly integrates:
- Lighting multiplier compensation for vanilla lighting compatibility
- Self-shadow application
- Conditional tangent shifting based on settings
- Gamma correction for both diffuse and specular outputs
The gamma correction application is particularly important for the Marschner model to maintain proper color space consistency.
215-224: Excellent unified interface with proper model switching.The
GetHairDirectLightfunction provides a clean abstraction that:
- Branches appropriately between Kajiya-Kay and Marschner models
- Applies gamma correction selectively to Marschner mode only
- Maintains a consistent interface regardless of the underlying model
This design pattern improves maintainability and makes the model selection transparent to calling code.
226-281: Complex but necessary branching for different hair models.The
GetHairIndirectSpecularLobeWeightsfunction properly handles the different requirements of Kajiya-Kay and Marschner models:Marschner path (mode 1):
- Uses
D_Marschnerfor specular calculations- Applies gamma correction for color space consistency
- Early return reduces nesting complexity
Kajiya-Kay path (mode 0):
- Maintains BRDF-based approach with shifted normals
- Proper fresnel and horizon fade calculations
- Consistent tangent shifting support
Both paths handle conditional tangent shifting appropriately. The complexity is justified given the fundamentally different approaches of the two models.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
…ity-shaders#1225) A simple hair self-shadow based on a 4-step screen space raymarch, so the hair won't overglow with transmission. Marschner model is a more physically based hair shading model compared to the default kajiya-kay model used in hair specular. User can now freely choose one of them. This model is dependent on self-shadow as it simulates both reflection and transmission. Added support for hair flow map (or directional map). The texture needs to be put in the slot 7 (starting from 0) in the texture set, and back lighting flag has to be enabled in hair nif. Hair flow map is important for both of the hair models as they are anisotropic. The texture uses the RG channels to record the direction of hair strands in UV space. The direction always points from the hair tip to the root.
2 participants
Basically three new things with no disadvantages compared to before.
Summary by CodeRabbit
New Features
Improvements