Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(215)

Issue 207090: add spline op-code (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 1 month ago by clifford.stein
Modified:
13 years, 2 months ago
Reviewers:
dev-osl, larrygritz, osl-dev, ckulla
Base URL:
http://openshadinglanguage.googlecode.com/svn/trunk/
Visibility:
Public.

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.

Patch Set 1 #

Total comments: 4

Patch Set 2 : addressing comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+524 lines, --1 lines) Patch
src/CMakeLists.txt View 1 chunk +1 line, -1 line 0 comments Download
src/liboslcomp/typecheck.cpp View 1 chunk +1 line, -0 lines 0 comments Download
src/liboslexec/CMakeLists.txt View 1 chunk +1 line, -1 line 0 comments Download
src/liboslexec/dual_vec.h View 1 chunk +28 lines, -0 lines 0 comments Download
src/liboslexec/master.cpp View 1 chunk +1 line, -0 lines 0 comments Download
src/liboslexec/opspline.cpp View 1 1 chunk +410 lines, -0 lines 1 comment Download
src/liboslexec/oslops.h View 1 chunk +1 line, -1 line 0 comments Download
testsuite/spline/ref/color.tif View 0 chunks +-1 lines, --1 lines 0 comments Download
testsuite/spline/ref/dcolor.tif View 0 chunks +-1 lines, --1 lines 0 comments Download
testsuite/spline/ref/dfloat.tif View 0 chunks +-1 lines, --1 lines 0 comments Download
testsuite/spline/ref/float.tif View 0 chunks +-1 lines, --1 lines 0 comments Download
testsuite/spline/ref/out.txt View 1 1 chunk +6 lines, -0 lines 0 comments Download
testsuite/spline/run.py View 1 1 chunk +27 lines, -0 lines 0 comments Download
testsuite/spline/test.osl View 1 1 chunk +52 lines, -0 lines 0 comments Download

Messages

Total messages: 12
clifford.stein
14 years, 1 month ago (2010-02-13 00:13:39 UTC) #1
larrygritz
Wow. LGTM. I think you went way above and beyond the call of duty on ...
14 years, 1 month ago (2010-02-13 00:36:46 UTC) #2
ckulla
Have you considered making the test an image instead of text only? Its kind of ...
14 years, 1 month ago (2010-02-13 00:41:24 UTC) #3
lg_larrygritz.com
Good point, Chris. An image would be great here. Might I suggest writing the shader ...
14 years, 1 month ago (2010-02-13 00:44:29 UTC) #4
lg_imageworks.com
I sent from wrong account. Posting again to avoid bounce from SPI alias... Good point, ...
14 years, 1 month ago (2010-02-13 00:46:28 UTC) #5
clifford.stein
On 2010/02/13 00:41:24, ckulla wrote: > Have you considered making the test an image instead ...
14 years, 1 month ago (2010-02-13 00:47:07 UTC) #6
clifford.stein
> Might I suggest writing the shader so that it interpolates the spline > based ...
14 years, 1 month ago (2010-02-13 01:30:31 UTC) #7
lg_imageworks.com
Don't knock yourself out over varying basis (though if easy, please do), I was suggesting ...
14 years, 1 month ago (2010-02-13 01:47:49 UTC) #8
clifford.stein
addressing comments
14 years, 1 month ago (2010-02-16 22:28:05 UTC) #9
clifford.stein
On 2010/02/16 22:28:05, clifford.stein wrote: > addressing comments Here's what I've done: * moved Dual2<float/vec3> ...
14 years, 1 month ago (2010-02-16 22:31:04 UTC) #10
clifford.stein
On 2010/02/16 22:31:04, clifford.stein wrote: > On 2010/02/16 22:28:05, clifford.stein wrote: > > addressing comments ...
14 years, 1 month ago (2010-02-16 22:44:13 UTC) #11
larrygritz
14 years, 1 month ago (2010-02-17 06:25:29 UTC) #12
I like the improvements.

LGTM.

See my comment about how you can simplify the switch statement.

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

http://codereview.appspot.com/207090/diff/1013/1016#newcode376
src/liboslexec/opspline.cpp:376: case (VALU_DERIVS | KNOT_DERIVS | TRIPLES):
This section seems overly complex to me.  Once you know that the result does not
have derivatives, you can completely ignore the derivatives of the knots and
value.  Remember the layout of derivs are that you can always safely treat a
with-derivs variable as a without-derivs variable, if you don't need it.  So it
seems to me that several of these cases can be collapsed.  In fact, from this
point down, you only need two more cases: TRIPLES, or not.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b