Fix and refactor sidedness code

[GoogleCodeExporter]

This ticket comes from a mail conversation between Chris and me. I'm
transfering it to a ticket.

I was looking at this function in osl:

Sidedness get_light_side(Sidedness viewing_side) const {
    switch (m_eval_sidedness) {
        case None:  return None; // eval not needed
        case Front: return Sidedness (m_sidedness & viewing_side); // same
side as viewer
        case Back:  return Sidedness (m_sidedness ^ viewing_side); //
opposite side to viewer
        case Both:  return Both; // always sensitive on both sides
        default:    return None;
    }
}

So let's say that sidedness is Front (1) and viewing_side is front too, but
eval_sidedness is Back cause it is refractive. Then you go and return
m_sidedness ^ viewing_side which is 1 ^ 1 = 0 = None. And I don't think
that is the lightside you want to return. And if the viewing_side was
Back(2) then you would be returning 2^1 = 3 = Both.

Original issue reported on code.google.com by aco...@gmail.com on 23 Mar 2010 at 5:34

[GoogleCodeExporter]

Reply from ckulla:

Hmmm, seems you are right, not sure what I thinking here.

I think you need to make a table of all the cases (eval_sidededness, sidedness 
and
view_side) to make sure we don't miss any.

Bonus points if you can setup a testsuite test that verifies that all the cases
render correctly ;)

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:35

[GoogleCodeExporter]

Reply from aconty:

Another issue that I have with sidedness is that those get_light_side  
functions are designed to return None if the closure is singular. But  
that is destroying information, now I find I need to know what is the  
"useful" side of a closure regardless of its singularity. What about  
returning always the real light side even if it can't be evaluated? We  
already check for singularity in the light loop anyway. They are  
orthogonal features.

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:37

[GoogleCodeExporter]

Reply by ckulla:

The original idea was that since eval() always returns 0 for singular  
closures, they don't need to be included in the light loop.

Why do you need to find out light_side for singular closures?

I'm fine with refactoring it though - just let me know what you think  
would work better.

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:38

[GoogleCodeExporter]

Reply by aconty:

For speeding up the regexps and evaluate them only once, I have to  
test with both REFLECT and TRANSMIT labels depending on the light  
side. So I thought I could find out whether a closure is transmitting,  
reflecting or both with the light side and the viewing side. If you  
can think of other alternatives I'm ok. I just thought that it would  
make sense that the light_side were independent from singularity.

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:39

[GoogleCodeExporter]

Reply from ckulla:

I see.

Aside from dielectric() and hair(), are there any closures that are  
both reflect and transmit ?

Maybe we should revisit the whole eval_reflect vs. eval_transmit  
thing, it didn't really work out the way I had hoped initially and it  
doesn't really make sense for the hair closures. We could also make  
the closures just return REFLECT or TRANSMIT like we do for the  
scattering instead of getting that information out of sample.

That would probably let you simplify some of the sidedness logic too.

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:40

[GoogleCodeExporter]

Reply from aconty:

And get rid of dielectric? Might be a good idea. We would then split  
hair into two closures (reflect and transmit, both two sided). But  
that's something we need consensus from everybody.

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:41

[GoogleCodeExporter]

Reply from ckulla:

I was thinking hair closures would just be a single closure that would  
always be classified as "reflect" even when the ray goes "backwards".

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:42

[GoogleCodeExporter]

I don't know if I like that idea. It would work, but I still think that
get_light_side should give the actual light side independently of the eval
capabilities. That would allow the render to guess the reflect/transmit thing 
and we
wouldn't have to change or remove any closures.

Larry? Any thoughts?

Original comment by aco...@gmail.com on 23 Mar 2010 at 5:46

[GoogleCodeExporter]

I agree that the function as written is wrong (or at least leads to unexpected
behavior).   I don't have a strong opinion either way on getting rid of 
dielectric
and/or splitting everything into separate reflect/transmit components.  Does 
that
mean we'd be giving up on the RR, or would we just use RR for all of the 
singular
components and there's no benefit to having any primitive closures that combine
reflection and transmission?

How will this decision seem once we add volumetrics, in which there's just
"scattering"?  We surely won't break volumetric BSDFs into separate forward and 
back
scattering.  Will we still feel that surface boundary scattering makes sense to 
split
even though it breaks symmetry with volumes?

Original comment by larrygr...@gmail.com on 23 Mar 2010 at 6:46

[GoogleCodeExporter]

My proposal doesn't require to split any closure. I think it's the smallest 
change to
get the direction info in the render. But maybe we want to change everything. I 
guess
I can't give any details of the render here, but splitting closures doesn't 
change
too much in the render. Whatever you were doing, you can keep on doing it.

Original comment by aco...@gmail.com on 23 Mar 2010 at 7:01

[GoogleCodeExporter]

I'm ok with the proposals.  Am I missing some grave implication?

Original comment by larrygr...@gmail.com on 23 Mar 2010 at 7:22

[GoogleCodeExporter]

Yeah its true - no need to overcomplicate the changes. So you plan to just skip
closures that are tagged as SINGULAR?

Original comment by cku...@gmail.com on 23 Mar 2010 at 8:22

[GoogleCodeExporter]

A fix for the original issue is under review:

http://codereview.appspot.com/702041/show

Original comment by aco...@gmail.com on 23 Mar 2010 at 8:53

[GoogleCodeExporter]

> So you plan to just skip
> closures that are tagged as SINGULAR?

We already do (I think) but will double check.

Original comment by aco...@gmail.com on 23 Mar 2010 at 8:55