refactor(hair): enhance hair specular visual, change default hair settings and feature info#1203
Conversation
|
""" WalkthroughThe changes refactor and extend the hair shading system by integrating a BRDF module, updating the hair specular and diffuse lighting models, and adding a transmission parameter. Default parameter values are adjusted, new functions and struct members are introduced, and the user interface is updated to support the new transmission control. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Settings
participant Shader
participant BRDF
UI->>Settings: User adjusts "Glossiness" or "Transmission"
Settings->>Shader: Pass updated Settings (including Transmission)
Shader->>BRDF: Compute specular/diffuse using BRDF functions
Shader->>Shader: Select Scheuermann or Marschner model
Shader-->>UI: Render updated hair shading
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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: 2
📜 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(6 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)src/Features/HairSpecular.cpp(1 hunks)src/Features/HairSpecular.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (10)
package/Shaders/Common/SharedData.hlsli (1)
162-162: Good use of padding space for the new transmission parameter.Replacing the padding field with a meaningful
Transmissionparameter is a clean way to extend functionality without changing the struct size or alignment.src/Features/HairSpecular.cpp (3)
17-18: LGTM! Serialization correctly updated for the new parameter.The serialization macro properly includes the new
Transmissionparameter, maintaining consistency with the struct definition.
23-23: Verify the extended glossiness range is intentional.The maximum glossiness value has been increased from 100.0f to 256.0f, which is a significant change (2.56x). This aligns with the increased default value in the header file but may affect existing user configurations.
Please confirm this range extension is intentional and that the shader calculations handle values up to 256 correctly without precision issues.
30-30: Transmission slider properly implemented.The new transmission slider with range [0.0, 1.0] is appropriately added and matches the default value of 0.5f defined in the header.
src/Features/HairSpecular.h (1)
50-50: New transmission parameter properly defined.The addition of the
Transmissionparameter with a default value of 0.5f is well-integrated into the settings structure.features/Hair Specular/Shaders/Hair/Hair.hlsli (5)
38-38: Function signatures properly updated with vertex normal parameter.The addition of the
VN(vertex normal) parameter to the lighting functions enables proper horizon occlusion calculations, improving the physical accuracy of the hair shading.Also applies to: 181-181
60-60: Correct change from Load to Sample for texture access.Switching from
LoadtoSamplefor the tangent shift texture is the correct approach as it enables texture filtering, providing smoother results for the hair tangent shifts.Also applies to: 199-199, 226-226, 277-277
48-74: Improved Scheuermann implementation with transmission support.The refactored Scheuermann lighting model now includes:
- Proper wrapped diffuse lighting
- Transmission effects with Fresnel-based scattering
- Better integration with the BRDF module
The implementation looks mathematically sound and the addition of transmission enhances the realism of hair rendering.
78-80: Fixed Gaussian distribution calculation.The Hair_g function now correctly uses the two-pi normalization factor
Math::TAUinstead of the hardcoded value, improving mathematical accuracy.
84-147: Enhanced Marschner model with TRT lobe.The Marschner implementation has been significantly improved:
- Added the third specular lobe (TRT) for more accurate hair rendering
- Corrected mathematical formulations
- Proper integration with the transmission parameter
These changes bring the implementation closer to the original Marschner paper.
| float HairGlossiness = 80.0f; | ||
| float SpecularMult = 1.0f; | ||
| float DiffuseMult = 1.0f; | ||
| uint EnableTangentShift = true; | ||
| float PrimaryTangentShift = 0.5f; | ||
| float SecondaryTangentShift = -0.25f; | ||
| float HairSaturation = 1.25f; | ||
| float HairSaturation = 1.0f; | ||
| float SpecularIndirectMult = 1.0f; | ||
| float DiffuseIndirectMult = 1.0f; | ||
| float BaseColorMult = 1.5f; | ||
| float pad; | ||
| float BaseColorMult = 1.0f; | ||
| float Transmission = 0.5f; |
There was a problem hiding this comment.
Default value changes may affect existing users.
The following default values have been significantly modified:
HairGlossiness: 60.0f → 80.0f (33% increase)HairSaturation: 1.25f → 1.0f (20% decrease)BaseColorMult: 1.5f → 1.0f (33% decrease)
While these changes align with the PR objective of maintaining closer resemblance to vanilla appearance, they constitute breaking changes for existing users who haven't customized these values.
Would you like me to generate a migration note or changelog entry to inform users about these default value changes?
🤖 Prompt for AI Agents
In src/Features/HairSpecular.h around lines 40 to 50, the default values for
HairGlossiness, HairSaturation, and BaseColorMult have been changed
significantly, which may break existing user setups. To fix this, create a
migration note or changelog entry clearly documenting these default value
changes and their impact, so users are informed and can adjust their
configurations accordingly.
| #include "Common/Math.hlsli" | ||
|
|
||
| #define MARSCHNER false | ||
| // #define MARSCHNER |
There was a problem hiding this comment.
Default lighting model changed from Marschner to Scheuermann.
By making Marschner opt-in with // #define MARSCHNER, the default hair lighting model has been changed to Scheuermann. This is a significant change that will affect the visual appearance of all hair rendering.
Ensure this change is documented in release notes as it may significantly alter the appearance of hair for existing users.
🤖 Prompt for AI Agents
In features/Hair Specular/Shaders/Hair/Hair.hlsli at line 7, the default hair
lighting model was changed from Marschner to Scheuermann by commenting out the
Marschner define. This change affects hair rendering appearance. Update the
release notes to document this change clearly, explaining that Marschner is now
opt-in and the default is Scheuermann, so users are aware of the visual impact.
|
✅ A pre-release build is available for this PR: |
|
@coderabbitai please propose an appropriate commit given the changes and the github 50 char limit. |
|
Tip For best results, initiate chat on the files or code changes. @alandtse Based on the changes in this PR, here's an appropriate commit message within the 50 character limit: This commit message (46 characters) captures:
Alternative options if you prefer different emphasis:
The first option best summarizes the comprehensive changes while staying well under the character limit. |
2 participants
Now at default settings hair specular would not look overbright, and won't stray too far from vanilla look.
Summary by CodeRabbit
New Features
Improvements