Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Chiang hair BSDF #1968

Conversation

msuzuki-nvidia
Copy link
Contributor

@msuzuki-nvidia msuzuki-nvidia commented Aug 12, 2024

Add Chiang hair BSDF.

This PR provides Chiang hair BSDF model and related nodes proposed by #1973

The nodes to add are:

  • <chiang_hair_bsdf> : The BSDF
  • <chiang_hair_roughness> : User friendly roughness mapping
  • <chiang_hair_absorption_from_color> : Absorption coefficient mapping from user input color
  • <deon_hair_absorption_from_melanin> : Absorption coefficient mapping from melanin parameters

It also include simple_hair_default material as a node graph example.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ld-kerley
Copy link
Contributor

Just to recap concretely the request made in the TSC meeting - it would be great if this PR could also include updates to the PBR spec document reflecting the new nodes being added here

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.PBRSpec.md

@msuzuki-nvidia
Copy link
Contributor Author

Thanks for the recap @ld-kerley. I will update this PR including the PBR spec document sometime soon.

@BrianSharpe
Copy link
Contributor

I think the sqrtPiOver8 factor might be an internal BSDF implementation detail, rather than something that should be done externally. For example, MDL already applies this factor internally.
https://github.com/NVIDIA/MDL-SDK/blob/master/src/mdl/jit/libbsdf/libbsdf_hair.h#L174

@msuzuki-nvidia
Copy link
Contributor Author

It can be internal. Although that part in MDL I'm not sure. MDL applies the π / 8 factor for both longitudinal and azimuthal roughness but the original paper (Appendix A. eq. 12) applies it only for azimuthal roughness, so I followed the paper. Other than that, it works for me.

@BrianSharpe
Copy link
Contributor

MDL applies the π/8 factor for both longitudinal and azimuthal roughness

I don't think it does.
The MDL hair_prepare_roughness() function in that github link I posted only applies it to the Y component.

Given the π/8 factor is in the appendix, rather than in equation 8, makes me think it probably should be part of the internal BSDF implementation rather than in <hair_roughness>. But happy to defer to others for that decision.

@msuzuki-nvidia
Copy link
Contributor Author

The MDL hair_prepare_roughness() function in that github link I posted only applies it to the Y component.

You're right. The factor is applied to only azimuthal!

I also see where your suggestion came from, and I feel the same now. I will include the change in next update.

@BrianSharpe
Copy link
Contributor

BrianSharpe commented Aug 16, 2024

I think <hair_roughness> might need some more changes. In its current form, someone can't tweak the TT and TRT scaling factors. So I'm thinking of two potential changes to accommodate this.

possible change 1) <hair_roughness> returns only a single "vec2 roughness" value (corresponding to roughness_R). This means it's only implementing equations 7 and 8, and any TT and TRT scaling should happen externally.
or
possible change 2) <hair_roughness> takes two additional inputs "tt_scale" and "trt_scale", which default to 0.25 and 4.0 (or should that be 0.5 and 2.0 with them being squared internally?)

Option 1 has less redundancy, but is more work for the user. Option 2 is easier to use, but has redundancy. I'm fine with either option.

@msuzuki-nvidia
Copy link
Contributor Author

I would prefer the second option with pre-squared defaults, 0.5 and 2.0. Indeed these scaling values are somewhat arbitrary but most implementations follow the Marschner's paper so I think it makes sense that the scaling is built-in and exposed in the parameters.

I will make this change as well. Thank you for the suggestion!

msuzuki-nvidia and others added 4 commits August 27, 2024 15:55
Signed-off-by: Jonathan Stone <[email protected]>
msuzuki-nvidia and others added 7 commits September 10, 2024 17:12
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks @msuzuki-nvidia, and I'm CC'ing @fpsunflower and @niklasharrysson for their thoughts from the OSL perspective.

@msuzuki-nvidia
Copy link
Contributor Author

Great to hear that, and thank you for correcting the spelling issues!

@jstone-lucasfilm jstone-lucasfilm merged commit 777826c into AcademySoftwareFoundation:main Sep 19, 2024
34 checks passed
jstone-lucasfilm pushed a commit that referenced this pull request Oct 10, 2024
The current approach for converting GLSL code to MSL doesn't work for the recent Chiang hair PR #1968.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants