fake fur bsdf

This is a fake fur shader based on the paper:
http://www.danbgoldman.com/misc/fakefur/fakefur.pdf

There are 3 closures, two of which essentially re-purpose bsdf_hair.cpp and have additional stuff to account for fur over fur attenuation of light, and another closure that does the same sort of thing for attenuated skin lighting underneath fur.  
Finally, there is an OSL shader which needs to be provided as well - this shader handles a local surface to world space transform for fur tangent vectors and includes yet another fur opacity function for handling the final compositing of fur over skin.
For use in place of full blown hair where medium to low LOD allows...

Re: "SMOOTHSTEP()" - I was informed at one point this is already laying around somewhere, but I haven't found it yet.

[masterblasterofdisaster, 2010-03-04 19:38:10]

update to fakefur bsdf, plus change to ashikhmin velvet fresnel

[aconty, 2010-03-04 21:34:27]

I don't know why I can't put comments on the code, but here is the thing that
confuses me right now. In FakefurSkinClosure::eval_reflect you first set pdf to
cos_pi and then later to 0.0f. I guess you want to remove the latter.

[ckulla, 2010-03-04 21:36:52]

I'm not able to view side-by-side diffs. Are you sure you picked the right
project base?

[aconty, 2010-03-04 21:38:38]

I guess this code:

pdf = cos_pi;
cos_pi = furIllum * furOpac;
pdf = 0.f;

Should be:

pdf = cos_pi;
cos_pi *= furIllum * furOpac; // note *=


Also, are you sure that FakefurSkinClosure transmits? cause eval_transmit is
calling eval_reflect. Shouldn't it be setting eval and pdf to 0.0f?

[masterblasterofdisaster, 2010-03-04 22:21:53]

On 2010/03/04 21:34:27, aconty wrote:
> I don't know why I can't put comments on the code, but here is the thing that
> confuses me right now. In FakefurSkinClosure::eval_reflect you first set pdf
to
> cos_pi and then later to 0.0f. I guess you want to remove the latter.

Looks like a copy and paste oversight.  I'll remove the later.

[masterblasterofdisaster, 2010-03-04 22:23:34]

On 2010/03/04 21:36:52, ckulla wrote:
> I'm not able to view side-by-side diffs. Are you sure you picked the right
> project base?

I svn diffed at OSL_local

[ckulla, 2010-03-04 22:24:33]

The diff is probably fine, but this review is looking at the python svn tree as
a base which breaks side-by-side diffs.

[masterblasterofdisaster, 2010-03-04 22:29:30]

changes to fake fur per alex's comments

[aconty, 2010-03-04 22:32:52]

You still have

cos_pi = furIllum * furOpac;

Is that intentional?

[masterblasterofdisaster, 2010-03-04 22:35:49]

On 2010/03/04 22:32:52, aconty wrote:
> You still have
> 
> cos_pi = furIllum * furOpac;
> 
> Is that intentional?

Yeah.  That's supposed to the the attenuated fur over skin diffuse value.

[aconty, 2010-03-04 22:44:07]

Shouldn't it be?

cos_pi *= furIllum * furOpac;

or something that doesn't just overwrite cos_pi?

[masterblasterofdisaster, 2010-03-04 22:46:45]

On 2010/03/04 22:44:07, aconty wrote:
> Shouldn't it be?
> 
> cos_pi *= furIllum * furOpac;
> 
> or something that doesn't just overwrite cos_pi?

Yes... Thanks, I'm seeing things.

[masterblasterofdisaster, 2010-03-04 22:55:14]

one more update...

[aconty, 2010-03-04 23:01:37]

LGTM

[ckulla, 2010-03-04 23:14:26]

I have a few comments:

http://codereview.appspot.com/224067/diff/3004/11004
File src/liboslexec/bsdf_fakefur.cpp (right):

http://codereview.appspot.com/224067/diff/3004/11004#newcode33
src/liboslexec/bsdf_fakefur.cpp:33: 
Could you include a reference + URL to which paper this is from?

http://codereview.appspot.com/224067/diff/3004/11004#newcode82
src/liboslexec/bsdf_fakefur.cpp:82: CLOSURE_CTOR (FakefurDiffuseClosure) :
BSDFClosure(Both, Labels::DIFFUSE, Both)
Are you sure this closure is "both" sided?

http://codereview.appspot.com/224067/diff/3004/11004#newcode120
src/liboslexec/bsdf_fakefur.cpp:120: float cosNI = m_N.dot(omega_in);
Need to multiply by normal_sign.

http://codereview.appspot.com/224067/diff/3004/11004#newcode127
src/liboslexec/bsdf_fakefur.cpp:127: // fur over fur opacity
http://monkeyisland.lylemcdonald.com/showthread.php?t=42929
This link points to a forum you have to register for. Is it possible to
summarize the contents of the post you are referencing here?

http://codereview.appspot.com/224067/diff/3004/11004#newcode135
src/liboslexec/bsdf_fakefur.cpp:135: pdf = (float) 1 / (4 * M_PI);
Shouldn't this be 1 / 2pi?

http://codereview.appspot.com/224067/diff/3004/11004#newcode141
src/liboslexec/bsdf_fakefur.cpp:141: return eval_reflect(omega_out, omega_in,
normal_sign, pdf);
Shouldn't this be 0?

This is an approximation for fur on surfaces, it should be impossible for light
to be transmitted from the backside.

http://codereview.appspot.com/224067/diff/3004/11004#newcode160
src/liboslexec/bsdf_fakefur.cpp:160: eval.setValue(sinTheta, sinTheta,
sinTheta);
This doesn't match the eval function at all.

http://codereview.appspot.com/224067/diff/3004/11004#newcode242
src/liboslexec/bsdf_fakefur.cpp:242: float cosNI = m_N.dot(omega_in);
Here too.

http://codereview.appspot.com/224067/diff/3004/11004#newcode279
src/liboslexec/bsdf_fakefur.cpp:279: return eval_reflect(omega_out, omega_in,
normal_sign, pdf);
Same comment - are you sure you want this?

http://codereview.appspot.com/224067/diff/3004/11004#newcode314
src/liboslexec/bsdf_fakefur.cpp:314: CLOSURE_CTOR (FakefurSkinClosure) :
BSDFClosure(Both, Labels::DIFFUSE)
Are you sure you want this closure to be both sided?

http://codereview.appspot.com/224067/diff/3004/11004#newcode349
src/liboslexec/bsdf_fakefur.cpp:349: float sigmaDir  = (1.f+kappa)/2.f *
m_fur_reflectivity;
Multiply by 0.5f instead of dividing by 2.f

http://codereview.appspot.com/224067/diff/3004/11004#newcode352
src/liboslexec/bsdf_fakefur.cpp:352: float cosNI = m_N.dot(omega_in);
normal_sign here too.

http://codereview.appspot.com/224067/diff/3004/11004#newcode354
src/liboslexec/bsdf_fakefur.cpp:354: float sigmaSurface = 1.f +
m_fur_attenuation * sigmaSurfaceA-1.f;
1 + atten * sigma - 1

Is this +1-1 intentional?

http://codereview.appspot.com/224067/diff/3004/11004#newcode388
src/liboslexec/bsdf_fakefur.cpp:388: eval.setValue(pdf, pdf, pdf);
This does not match eval. Are you sure this is a good approximation?

[aconty, 2010-03-04 23:28:16]

Wait a minute, if as Chris says this is to be mapped on a regular surface and be
single sided, what is the tangent vector doing here?

I was thinking all the time that it was going to be applied to ribbons. Or is it
and we just avoid the transparency? In that case sampling a hemisphere on the
ribbon's normal will be wrong too.

[masterblasterofdisaster, 2010-03-04 23:30:41]

On 2010/03/04 23:28:16, aconty wrote:
> Wait a minute, if as Chris says this is to be mapped on a regular surface and
be
> single sided, what is the tangent vector doing here?
> 
> I was thinking all the time that it was going to be applied to ribbons. Or is
it
> and we just avoid the transparency? In that case sampling a hemisphere on the
> ribbon's normal will be wrong too.

The tangent vector is passed in from a map that is derived from the hair comb.

[aconty, 2010-03-04 23:37:44]

Then chris is right, everything should be single sided and you should sample a
hemisphere. Which I guess will be on the surface normal.

[larrygritz, 2010-03-05 00:10:30]

http://codereview.appspot.com/224067/diff/3004/11004
File src/liboslexec/bsdf_fakefur.cpp (right):

http://codereview.appspot.com/224067/diff/3004/11004#newcode34
src/liboslexec/bsdf_fakefur.cpp:34: inline float SMOOTHSTEP(const float &edge0,
const float &edge1, float &x) {
No reason for ALL CAPS.  I would prefer moving this to an appropriate header and
also turning it into a template so it will work with any type.

http://codereview.appspot.com/224067/diff/3004/11004#newcode45
src/liboslexec/bsdf_fakefur.cpp:45: inline float furOpacity(float &cosNI, float
&cosTI,
There's no point making a parameter reference if you aren't modifying it, unless
it's something big enough that you want to avoid unnecessary copies (in this
case it is not: a 4-byte float is smaller than an 8-byte pointer/reference).

[masterblasterofdisaster, 2010-03-05 00:29:57]

On 2010/03/05 00:10:30, larrygritz wrote:
> http://codereview.appspot.com/224067/diff/3004/11004
> File src/liboslexec/bsdf_fakefur.cpp (right):
> 
> http://codereview.appspot.com/224067/diff/3004/11004#newcode34
> src/liboslexec/bsdf_fakefur.cpp:34: inline float SMOOTHSTEP(const float
&edge0,
> const float &edge1, float &x) {
> No reason for ALL CAPS.  I would prefer moving this to an appropriate header
and
> also turning it into a template so it will work with any type.
> 

Yeah, won't compile when I use a lowercase smoothstep, left it LOUD as a
reminder... I can't find where it's coming from. 

I'd be happy to set things right - which header, then?

[lg_imageworks.com, 2010-03-05 00:41:51]

If it won't compile with lowercase, that probably means it's already defined
somewhere!

Hmmm... seems that somebody already implemented smoothstep in
bsdf_cloth_fncs.cpp.  That might be part of the problem.  Let's move it from
there and turn it into a template in, I dunno, how about oslops.h?

(I'm almost inclined to put it in OIIO's fmath.h, right next to its friend
clamp(), but it's not very necessary since nothing else in OIIO uses
smoothstep.)



On Mar 4, 2010, at 4:29 PM, <masterblasterofdisaster@gmail.com> wrote:

> On 2010/03/05 00:10:30, larrygritz wrote:
>> http://codereview.appspot.com/224067/diff/3004/11004
>> File src/liboslexec/bsdf_fakefur.cpp (right):
> 
>> http://codereview.appspot.com/224067/diff/3004/11004#newcode34
>> src/liboslexec/bsdf_fakefur.cpp:34: inline float SMOOTHSTEP(const
> float &edge0,
>> const float &edge1, float &x) {
>> No reason for ALL CAPS.  I would prefer moving this to an appropriate
> header and
>> also turning it into a template so it will work with any type.
> 
> 
> Yeah, won't compile when I use a lowercase smoothstep, left it LOUD as a
> reminder... I can't find where it's coming from.
> 
> I'd be happy to set things right - which header, then?
> 
> 
> 
> 
> 
> 
> 
> http://codereview.appspot.com/224067/show
> 

--
Larry Gritz
lg@imageworks.com

[aconty, 2010-03-05 00:59:01]

I'm not sure if oiio should be a basic library for everything. Well, that's not
true, I'm sure it shouldn't :) The place for that should be IMath I guess, but
we don't control it. I suggest putting it somewhere inside osl.

[lg_imageworks.com, 2010-03-05 01:13:07]

I totally agree, that's why I suggested oslops.h.  I was just commenting that
there's a "clamp" in fmath.h, and I always thought the clamp and smoothstep were
neighbors.

We should, in fact, send the smoothstep to the Imath folks (some of them are
probably reading this) and maybe someday it'll make its way into the library. 
(And maybe someday after that, our site will be on a modern enough OpenEXR
version to use it. :-)


On Mar 4, 2010, at 4:59 PM, <aconty@gmail.com> wrote:

> I'm not sure if oiio should be a basic library for everything. Well,
> that's not true, I'm sure it shouldn't :) The place for that should be
> IMath I guess, but we don't control it. I suggest putting it somewhere
> inside osl.
> 
> http://codereview.appspot.com/224067/show
> 

--
Larry Gritz
lg@imageworks.com

[jreynolds_imageworks.com, 2010-03-05 01:26:17]

Larry Gritz wrote:
> If it won't compile with lowercase, that probably means it's already defined
somewhere!
>   
Yeah, that's what I mean, I couldn't find . -exec grep ...blah, blah 
where it's coming from.

Chris pointed me to a version in dual.h that looks like it might be the one.

> Hmmm... seems that somebody already implemented smoothstep in
bsdf_cloth_fncs.cpp.  That might be part of the problem.  Let's move it from
there and turn it into a template in, I dunno, how about oslops.h?
>
> (I'm almost inclined to put it in OIIO's fmath.h, right next to its friend
clamp(), but it's not very necessary since nothing else in OIIO uses
smoothstep.)
>
>
>
> On Mar 4, 2010, at 4:29 PM, <masterblasterofdisaster@gmail.com> wrote:
>
>   
>> On 2010/03/05 00:10:30, larrygritz wrote:
>>     
>>> http://codereview.appspot.com/224067/diff/3004/11004
>>> File src/liboslexec/bsdf_fakefur.cpp (right):
>>>       
>>> http://codereview.appspot.com/224067/diff/3004/11004#newcode34
>>> src/liboslexec/bsdf_fakefur.cpp:34: inline float SMOOTHSTEP(const
>>>       
>> float &edge0,
>>     
>>> const float &edge1, float &x) {
>>> No reason for ALL CAPS.  I would prefer moving this to an appropriate
>>>       
>> header and
>>     
>>> also turning it into a template so it will work with any type.
>>>       
>> Yeah, won't compile when I use a lowercase smoothstep, left it LOUD as a
>> reminder... I can't find where it's coming from.
>>
>> I'd be happy to set things right - which header, then?
>>
>>
>>
>>
>>
>>
>>
>> http://codereview.appspot.com/224067/show
>>
>>     
>
> --
> Larry Gritz
> lg@imageworks.com
>
>
>
>   

[masterblasterofdisaster, 2010-03-05 23:31:32]

updated per yesterdays review comments

[masterblasterofdisaster, 2010-03-05 23:39:44]

correction to patch set...

[ckulla, 2010-03-05 23:59:01]

http://codereview.appspot.com/224067/diff/35001/18005
File src/liboslexec/bsdf_fakefur.cpp (right):

http://codereview.appspot.com/224067/diff/35001/18005#newcode88
src/liboslexec/bsdf_fakefur.cpp:88: CLOSURE_CTOR (FakefurDiffuseClosure) :
BSDFClosure(side, Labels::DIFFUSE, Both)
You need to get rid of the other "Both" here. Otherwise you are indicating that
the surface gathers light from behind.

http://codereview.appspot.com/224067/diff/35001/18005#newcode160
src/liboslexec/bsdf_fakefur.cpp:160: sample_cos_hemisphere (Nf, omega_out,
randu, randv, omega_in, pdf);
You are sampling using a cosine distribution here, but the eval function is
applying a uniform PDF.

http://codereview.appspot.com/224067/diff/35001/18005#newcode220
src/liboslexec/bsdf_fakefur.cpp:220: CLOSURE_CTOR (FakefurSpecularClosure) :
BSDFClosure(side, Labels::GLOSSY, Both)
Same comment here (no Both)

[aconty, 2010-03-06 00:14:36]

http://codereview.appspot.com/224067/diff/35001/18005
File src/liboslexec/bsdf_fakefur.cpp (right):

http://codereview.appspot.com/224067/diff/35001/18005#newcode160
src/liboslexec/bsdf_fakefur.cpp:160: sample_cos_hemisphere (Nf, omega_out,
randu, randv, omega_in, pdf);
Which means you have to change it in the eval_reflect function. Just set the
correct pdf cos/pi

[masterblasterofdisaster, 2010-03-06 00:27:20]

...

[ckulla, 2010-03-06 00:31:08]

http://codereview.appspot.com/224067/diff/40001/32004
File src/liboslexec/bsdf_fakefur.cpp (right):

http://codereview.appspot.com/224067/diff/40001/32004#newcode88
src/liboslexec/bsdf_fakefur.cpp:88: CLOSURE_CTOR (FakefurDiffuseClosure) :
BSDFClosure(side, Labels::DIFFUSE, side)
The third argument just needs to be removed. Look at bsdf_phong.cpp for an
example.

http://codereview.appspot.com/224067/diff/40001/32004#newcode222
src/liboslexec/bsdf_fakefur.cpp:222: CLOSURE_CTOR (FakefurSpecularClosure) :
BSDFClosure(side, Labels::GLOSSY, side)
Same here.

[masterblasterofdisaster, 2010-03-09 16:46:05]

update

[ckulla, 2010-03-10 18:06:10]

LGTM