Date   

Re: add spline op-code (issue207090)

cliffo...@...
 

Might I suggest writing the shader so that it interpolates the spline
based on u, and varies the interpolation type based on v (e.g.
"linear" if v < 0.2, "b-spline" if 0.2 <= v < 0.4, etc.).
That would make the basis "varying", which the code currently doesn't
allow, but that can be changed. I'll make the change -- it shouldn't be
tough to do.



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


Re: add spline op-code (issue207090)

cliffo...@...
 

On 2010/02/13 00:41:24, ckulla wrote:
Have you considered making the test an image instead of text only?
Its kind of hard to know if this is working correctly or not by just
looking at
the numbers.
During testing, I did plot the output (w/ 1000 points) of the various
bases with gnuplot and they all looked pretty good. But I can try an
image.


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


Re: add spline op-code (issue207090)

Larry Gritz <l...@...>
 

I sent from wrong account. Posting again to avoid bounce from SPI alias...


Good point, Chris. An image would be great here.

Might I suggest writing the shader so that it interpolates the spline based on u, and varies the interpolation type based on v (e.g. "linear" if v < 0.2, "b-spline" if 0.2 <= v < 0.4, etc.).

Also, I think in this case you probably should have tests for both float and color splines, just to be sure.


On Feb 12, 2010, at 4:41 PM, cku...@... wrote:

Have you considered making the test an image instead of text only?

Its kind of hard to know if this is working correctly or not by just
looking at the numbers.

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


--
Larry Gritz
l...@...


Re: add spline op-code (issue207090)

cku...@...
 

Have you considered making the test an image instead of text only?

Its kind of hard to know if this is working correctly or not by just
looking at the numbers.

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


Re: add spline op-code (issue207090)

larry...@...
 

Wow. LGTM. I think you went way above and beyond the call of duty on
this, Cliff. Ignoring knot derivs would probably have been fine. In
fact, I would go so far as to say perhaps you should have a #define that
allows knot derivs to be ignored so in the future we can see how much
this costs us. Or maybe that should be the default for now, considering
that we currently have derivs (unnecessarily) on everything.




http://codereview.appspot.com/207090/diff/1/4
File src/liboslexec/opspline.cpp (right):

http://codereview.appspot.com/207090/diff/1/4#newcode113
src/liboslexec/opspline.cpp:113: template<class T> T removeDerivatives
(T x) { return x; }
const T?

http://codereview.appspot.com/207090/diff/1/4#newcode114
src/liboslexec/opspline.cpp:114: template<class T> T removeDerivatives
(const Dual2<T> x) { return x.val(); }
const Dual2<T> &x ? ref?

http://codereview.appspot.com/207090/diff/1/4#newcode139
src/liboslexec/opspline.cpp:139: }
I wonder if these functions above shouldn't be in dual_vec.h? May come
in handy if they are needed elsewhere.

http://codereview.appspot.com/207090/diff/1/4#newcode158
src/liboslexec/opspline.cpp:158: // We need to know explicitly whether
the knots have
Golly, Cliff, that's more than I would've done. Sure, we need derivs
with respect to the interpolant, but I'm not sure in what circumstance
the knots themselves need derivs.

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


add spline op-code (issue207090)

cliffo...@...
 

Reviewers: dev-osl_imageworks.com, osl-dev_googlegroups.com,

Description:
This adds 'spline' to OSL. This op-code is a bit on the heavy side
w.r.t. templates and that's due to the variety of ways this function can
be called. I had a version which was less dependent on templates, but
it resulted in more code duplication.

This op-code takes in an array of knots. If we think it's useful to
specify an extra argument which contains the number of knots (so
potentially the remained of the knot array is empty), I can do that.



Please review this at http://codereview.appspot.com/207090/show

Affected files:
src/CMakeLists.txt
src/liboslcomp/typecheck.cpp
src/liboslexec/CMakeLists.txt
src/liboslexec/master.cpp
src/liboslexec/opspline.cpp
src/liboslexec/oslops.h
testsuite/spline/ref/out.txt
testsuite/spline/run.py
testsuite/spline/test.osl


Re: hair sidedness bug (issue206064)

aco...@...
 

Whatever we come up with needs to be coordinated to your other fix.
This change
is only about fixing the hair.
No problem, go ahead. As I said in the previous reply we moved singular
out of the light loop some time ago, so my code won't break anything.

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


Re: hair sidedness bug (issue206064)

cku...@...
 

On 2010/02/10 21:49:56, aconty wrote:
That's right, but if you commit that how are we going to fix it?
We could have get_light_side return front/back but then skip the eval
loop for singular closures for instance.

Whatever we come up with needs to be coordinated to your other fix. This
change is only about fixing the hair.


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


Re: hair sidedness bug (issue206064)

aco...@...
 

No, wait a minute. Singular had already been taken out of MIS for
optimization. This is not going to break anything. I still would change
it, but it is safe to commit. Go ahead.

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


Re: hair sidedness bug (issue206064)

aco...@...
 

That's right, but if you commit that how are we going to fix it?

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


Re: hair sidedness bug (issue206064)

cku...@...
 

Right - what I meant is that its already broken in your other review,
regardless of this change.

Let's discuss this more in the other code review.

Since the behavior doesn't change for now - can I commit this?

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


Re: hair sidedness bug (issue206064)

aco...@...
 

Is not broken now, because we don't use the sidedness to discard lights
from the light loop. We use it to discard direct sampling, but not when
sampling the closures.

But now, with my added extra check for sidedness in the light
estimation, this will prevent the light to arrive to both parts of the
MIS.

Plus I think it is confusing to mix two concepts, sidedness and
singularity. Even if it didn't break anything, it would do sooner or
later.

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


Re: hair sidedness bug (issue206064)

cku...@...
 

But the behavior for singular closures does not change in this review.
So it sounds like something is already broken.


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


Re: hair sidedness bug (issue206064)

aco...@...
 

It breaks with my other review, so nothing that you could actually
reproduce now. But the case is this. Think of a reflective plane that is
reflecting a sphere light. That reflection comes from the indirect part
of the MIS, but if you set the sidedness to None, my other patch will
put the light out of the loop and therefore won't be accounted for (no
reflection)


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


Re: hair sidedness bug (issue206064)

cku...@...
 

I'm not sure I understand - we were already returning None from
get_light_side() for singular closures before.

Can you setup a test case for me to look at?

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


Re: hair sidedness bug (issue206064)

aco...@...
 

Now it looks clear. The only thing I don't like is using eval_sidedness
of None for singular closures. Now I'm using sidedness to choose visible
lights and this is going to break it. Can you avoid setting None for
singular closures? We really need that information for MIS. Singularity
is already reflected on the scattering label.

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


hair sidedness bug (issue206064)

cku...@...
 

Reviewers: dev-osl_imageworks.com, osl-dev_googlegroups.com,

Description:
I forgot one important case in the get_light_side function - some
closures receive light from both sides simultaneously (like hair).

This patch makes the 4 cases explicit:
None: surface doesn't need eval
Front: surface is sensitive to light on the same side as the viewer
Back: surface is sensitive to light on the opposite side of the
viewer
Both: surface is sensitive to light on both sides

This fixes backlighting on hair - previously only eval_reflect was being
called because get_light_side was filtering out lights coming from
behind.


Please review this at http://codereview.appspot.com/206064/show

Affected files:
src/include/oslclosure.h
src/liboslexec/bsdf_diffuse.cpp
src/liboslexec/bsdf_hair.cpp
src/liboslexec/bsdf_microfacet.cpp
src/liboslexec/bsdf_reflection.cpp
src/liboslexec/bsdf_refraction.cpp
src/liboslexec/bsdf_transparent.cpp
src/liboslexec/closure_test.cpp


Re: array assign can nuke derivatives (issue205043)

aco...@...
 


Re: array assign can nuke derivatives (issue205043)

cliffo...@...
 

On 2010/02/09 21:40:51, ckulla wrote:
On 2010/02/08 19:54:50, ckulla wrote:
Any comments on this?
LGTM

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


Re: array assign can nuke derivatives (issue205043)

cku...@...
 

On 2010/02/08 19:54:50, ckulla wrote:


Any comments on this?

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

4861 - 4880 of 5098