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

Hair material appears to have an importance sampling issue. #121

Closed
etheory opened this issue Mar 8, 2017 · 4 comments
Closed

Hair material appears to have an importance sampling issue. #121

etheory opened this issue Mar 8, 2017 · 4 comments

Comments

@etheory
Copy link

etheory commented Mar 8, 2017

From what I can see, the hair works as follows.

With respect to the longitudinal roughness function:
1.) The theta_o is reflected to produce the required theta_i angle.
2.) This implies that the cuticle is effectively tilting the reflected lobe itself up or down. Since we are reflecting theta_o to produce some theta_r.
Hence theta_r, which is the reflected lobe direction, is theta_r = -theta_o + cuticleAngle
3.) Sampling this lobe must therefore be done by sampling theta_r. If we instead sample -theta_o, and then rotate the resulting angles, we'll get a mismatch between the sampling function and the current pdf evaluation for large cuticle angles.

As this is also effecting my own current implementation of a more advanced version of a similar shader I'm fairly sure this observation is accurate.

The fix would be to modify the shader to rotate theta_o, and not theta_i, both in Sample_f (sample the rotated angle, not rotate the sampled angle) and f.

@Immocat
Copy link
Contributor

Immocat commented Mar 23, 2018

Hi @etheory,

Although it is one year after your post, I find the same problem here recently when exploring the hair shading. I think that pbrt's implementation is not the same as Importance Sampling for Physically-based Hair Fiber Models when importance sampling the longitudinal M function, which I think it is a bug as you mentioned. Also, I think your advice is basically correct. I would like to do more implementation and test, and then write a report for @mmp as soon as possible.

Zejian Wang (@Immocat)


Update: After carefully reading through the d'Eon et al's paper, I am sure that we should tilt then sampling. It also seems that the RECIPROCITY problem caused by unmatched Ap mentioned at the end of the hair chapter could also be solved by doing so. The sampling order should be:

  1. Tilt sinThetaO' = tilt(sinThetaO, scale_alpha)
  2. Calculate all A(sinThetaO' , h) for each p
  3. Sample one p
  4. SampleM(v[p], sinThetaO')

However, as mentioned in d'Eon et al's paper, sampling M is not reciprocal due to the 1/cosThetai as well as the tilted scales. We need a whole new M function if we really want to be totally and mathematically reciprocal, where I think that it is not necessary for now. I will do more test for both pbrt original implementation and my modified version. If everything here is correct, this may lead to a new pull request. I would try to modify as few as possible code in HairBSDF.

@mmp
Copy link
Owner

mmp commented Dec 13, 2019

Fixed now. Thanks to @rgirish28 for the PR, and many thanks to @Immocat and @etheory for the thorough explanations of the underlying error and the appropriate solution.

@mmp mmp closed this as completed Dec 13, 2019
@etheory
Copy link
Author

etheory commented Feb 19, 2020

I know it's a few months ago now, but I've only just seen this. Thanks for finally getting around to it! Really appreciate it. Happy it's resolved now.

@Immocat
Copy link
Contributor

Immocat commented Feb 24, 2020

Actually It's been a long time and I totally forget the details 😆 . Looking forward to pbrt 4 now.

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

No branches or pull requests

3 participants