woven cloth

specular cloth closure - filtered diffuse and cloth weave pattern generation code is now all in OSL.  No need to review the original bsdf_cloth.cpp since this is scheduled for removal.

[masterblasterofdisaster, 2010-01-28 18:08:13]

http://codereview.appspot.com/195082/diff/1/6
File src/liboslexec/bsdf_cloth_specular.cpp (right):

http://codereview.appspot.com/195082/diff/1/6#newcode266
src/liboslexec/bsdf_cloth_specular.cpp:266: if(m_btf_interp < 1.f){
I'm still on the fence as to whether the BTF should be separate.

[ckulla, 2010-02-03 18:12:14]

Did you plan on removing the old cloth closure as a separate commit?

How will the osl support code for pattern generation be shipped?

http://codereview.appspot.com/195082/diff/1/6
File src/liboslexec/bsdf_cloth_specular.cpp (right):

http://codereview.appspot.com/195082/diff/1/6#newcode152
src/liboslexec/bsdf_cloth_specular.cpp:152: float cosNI = m_N.dot(omega_in);
Both these lines need to multiply by normal_sign (see other BSDFs)

In fact - everywhere you use m_N inside eval_reflect - you should be using
(normal_sign * m_N). You might want to make a local variable for this instead.

This is important to handle sidedness correctly.

http://codereview.appspot.com/195082/diff/1/6#newcode158
src/liboslexec/bsdf_cloth_specular.cpp:158: Point2 rect[4];
Declare these variables where they will be used.

http://codereview.appspot.com/195082/diff/1/6#newcode170
src/liboslexec/bsdf_cloth_specular.cpp:170: if(m_R0[LONGWARP] ==
m_R0[LONGWEFT]){
Testing for this is probably slower than just computing both.

It also keeps the code shorter.

http://codereview.appspot.com/195082/diff/1/6#newcode178
src/liboslexec/bsdf_cloth_specular.cpp:178: float I, G;
Don't declare variables until you need them.

http://codereview.appspot.com/195082/diff/1/6#newcode264
src/liboslexec/bsdf_cloth_specular.cpp:264: G = computeG_Smith(m_N, H,
omega_out, cosNI, cosNO);
You could declare G down here.

http://codereview.appspot.com/195082/diff/1/6#newcode309
src/liboslexec/bsdf_cloth_specular.cpp:309: if (faceforward (omega_out, Ng, m_N,
Ngf, Nf)) {
Why? If you are not implementing sample just use:

{
  return Labels::Reflect;
}

http://codereview.appspot.com/195082/diff/1/3
File src/liboslexec/oslops.h (right):

http://codereview.appspot.com/195082/diff/1/3#newcode768
src/liboslexec/oslops.h:768: fetch_value_array<typeof(v[0]), N>(v, argidx, idx,
exec, nargs, args)
Is typeof required here? The compiler should be able to deduce the template
argument on its own.

[masterblasterofdisaster, 2010-02-03 19:00:48]

"Did you plan on removing the old cloth closure as a separate commit?

Yep, just need to coordinate removing the original cloth closure w/ the others.

"How will the osl support code for pattern generation be shipped?

Not sure, though we had discussed providing it as a an OSL .h file.

http://codereview.appspot.com/195082/diff/1/6
File src/liboslexec/bsdf_cloth_specular.cpp (right):

http://codereview.appspot.com/195082/diff/1/6#newcode309
src/liboslexec/bsdf_cloth_specular.cpp:309: if (faceforward (omega_out, Ng, m_N,
Ngf, Nf)) {
On 2010/02/03 18:12:14, ckulla wrote:
> Why? If you are not implementing sample just use:
> 
> {
>   return Labels::Reflect;
> }

Left it there for messing around, forgot to remove.

http://codereview.appspot.com/195082/diff/1/3
File src/liboslexec/oslops.h (right):

http://codereview.appspot.com/195082/diff/1/3#newcode768
src/liboslexec/oslops.h:768: fetch_value_array<typeof(v[0]), N>(v, argidx, idx,
exec, nargs, args)
On 2010/02/03 18:12:14, ckulla wrote:
> Is typeof required here? The compiler should be able to deduce the template
> argument on its own.

It should, but I was having no such luck and ended up having to use typeof.

[masterblasterofdisaster, 2010-02-04 00:35:56]

applied changes per Chris' review

[ckulla, 2010-02-04 00:44:26]

LGTM

[masterblasterofdisaster, 2010-02-05 19:11:26]

switched from Schlick Fresnel approximation to fresnel_dielectric() in
bsdf_cloth_specular.cpp

[ckulla, 2010-02-05 19:15:40]

On 2010/02/05 19:11:26, masterblasterofdisaster wrote:
> switched from Schlick Fresnel approximation to fresnel_dielectric() in
> bsdf_cloth_specular.cpp

Did you upload the right patch? The code looks the same.

[masterblasterofdisaster, 2010-02-05 19:17:16]

one more try:  switched from Schlick Fresnel approximation to
fresnel_dielectric() in bsdf_cloth_spe

[masterblasterofdisaster, 2010-02-05 19:25:28]

again: switched from Schlick Fresnel approximation to fresnel_dielectric()

[ckulla, 2010-02-05 21:55:02]

On 2010/02/05 19:25:28, masterblasterofdisaster wrote:
> again: switched from Schlick Fresnel approximation to fresnel_dielectric()

LGTM