1. osl-dev@lists.aswf.io
  2. Messages
   Date   
Re: Review: smarter runflags for lazy eval (issue194117)

aco...@...
 

Re: Review: smarter runflags for lazy eval (issue194117)

cku...@...
 

LGTM2

Can't wait to see the difference on those benchmark cases.

http://codereview.appspot.com/194117/show
Re: Compiling OpenShadingLanguage under Windows

Wormszer <worm...@...>
 

Mcpp integrated pretty easily with the existing structure. I can send you the project I made for it. Unfortunatly I wasn't able to get it to build on vs with cpp syntax. I think it assumed some other things would be around that are installed with gcc.

I was thinking I might send you a patch first and you can apply it to a clean checkout and see if you hit any issues. Then I can submit it to the osl dev.

I think we are a minority on windows and it might be worth trying it on a second windows machine before saying it works.

I will be back at home tomorrow and will send you the project file.

You could look into the print precision issue I mentioned. I'm hoping that's just a simple fix and possibly the last issue.

Jeremy

On Jan 27, 2010, at 3:48 PM, Oleg <ode...@...> wrote:

Hi Jeremy,

I was trying to use the boost.wave preprocessor, but still have some
troubles to integrate it. I was not able to run any tests so far.
However, I'm still trying :-) Maybe it would be useful if you submit
the project file for the mcpp as well.

Regards,
Oleg

On 27 Jan., 06:41, Wormszer <wo...@...> wrote:
I just checked one failed test file output and the error was
ref:
-5.96046e-08
out:
-5.96046e-008
I wonder if this is due to different default precision on cout possibly?
That was the only error so I am not sure.

Any ideas?

I guess the printing is being done in the shader, so maybe ill look there at
the printf operator. It was the only value printed using scientific
notation.

I imagine the others will probably be something similiar.

Jeremy

On Wed, Jan 27, 2010 at 12:31 AM, Wormszer <wo...@...> wrote:
I have made some modifications to runtest.py to allow for the tests to be
run on windows along with some changes to the cmake files.
I have redone the everything from scratch and have made changes to cmake
and the source that should make it easy to build on windows.
I have some extra instructions for setting up some of the dependencies and
then modified the cmake routines to find the external libraries as they come
from installing and building OIIO.
OIIO cmake routines still need manual help and I may go modify them,
instead of having to add a bunch of paths to CMAKE_INSTALL_PREFIX.
I can now run the tests as well but only 35% of them pass.
I was hoping that through including the flex and bison outputs in the
project the need for cygwin could be removed, but the testsuite uses diff so
it maybe here to stay.
To get around having to modify every test, and having to parse the command
lines to pull out the output file names and create a stdout to file pipe, i
decieded to write the command to a batch file and have python call that.
This worked except that my install folder is C:\program files (x86)\... and
of course the space was now giving me issues.
So i was able to parse the command strings and wrap them commands in quotes
and then write them to the batch file and run it.
Now of course that wasn't the end of it. The next issue is that windows
uses cr/lf i guess through > even though the output should be writing just
lf, so diff was puking on that.
So on windows the diff command has to take an extra cmd parameter
--strip-trailing-cr
So now I can run the tests, I will investigate the errors and then should
be ready to submit a patch.
Jeremy
--
You received this message because you are subscribed to the Google Groups "OSL Developers" group.
To post to this group, send email to osl...@....
To unsubscribe from this group, send email to osl...@... .
For more options, visit this group at http://groups.google.com/group/osl-dev?hl=en .
Re: Review: smarter runflags for lazy eval (issue194117)

larry...@...
 

On 2010/01/27 23:22:32, ckulla wrote:
Can't wait to see the difference on those benchmark cases.
OK, I committed it, so give it a try.
This should throw into even more stark relief the waste associated with
sparse runflags. I'll tackle that after "rebind" is done.


http://codereview.appspot.com/194117/show
Review: compiler failed to mark a const as "not written" (issue195084)

larry...@...
 

Reviewers: ,

Description:
Bug fix:

When a function's 'return' statement is not the last statement in the
function body, we use a trick where we wrap the function in a 'do { ...
} while (0)' and the return is like a 'break'. (This is because we
don't have a true jump instruction, which is a pain for SIMD.) We say
'while 0' because we don't want to LOOP, we just want a loop to jump out
of, if you catch my drift.

The bug is that I made the "0" a true constant, but forgot to mark that
the while condition was not written by the while statement itself, and
therefore got a "can't write to a constant" error later on.


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

Affected files:
src/liboslcomp/codegen.cpp


Index: src/liboslcomp/codegen.cpp
===================================================================
--- src/liboslcomp/codegen.cpp (revision 552)
+++ src/liboslcomp/codegen.cpp (working copy)
@@ -1181,7 +1181,8 @@
Symbol *condvar = m_compiler->make_constant (0);
size_t argstart = m_compiler->add_op_args (1, &condvar);
m_compiler->ircode(loop_op).set_args (argstart, 1);
- m_compiler->ircode(loop_op).argread (0, true); // read also
+ m_compiler->ircode(loop_op).argread (0, true); // read
+ m_compiler->ircode(loop_op).argwrite (0, false); // not written
int endlabel = m_compiler->next_op_label ();
m_compiler->ircode(loop_op).set_jump (startlabel, startlabel,
endlabel, endlabel);
Missing sqrtf in beckman's sample function (issue196043)

aco...@...
 

Reviewers: osl-dev_googlegroups.com,

Description:
There was a problem with the sample function in beckman closure. A
missing sqrt for sampling the tangent of theta. It was preventing
reflections from getting the right blur. This patch fixes the problem.

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

Affected files:
src/liboslexec/bsdf_microfacet.cpp


Index: src/liboslexec/bsdf_microfacet.cpp
===================================================================
--- src/liboslexec/bsdf_microfacet.cpp (revision 551)
+++ src/liboslexec/bsdf_microfacet.cpp (working copy)
@@ -374,7 +374,7 @@ public:
// we take advantage of cos(atan(x)) == 1/sqrt(1+x^2)
// and sin(atan(x)) == x/sqrt(1+x^2)
float alpha2 = m_ab * m_ab;
- float tanThetaM = -alpha2 * logf(1 - randu);
+ float tanThetaM = sqrtf(-alpha2 * logf(1 - randu));
float cosThetaM = 1 / sqrtf(1 + tanThetaM * tanThetaM);
float sinThetaM = cosThetaM * tanThetaM;
float phiM = 2 * float(M_PI) * randv;
Re: Missing sqrtf in beckman's sample function (issue196043)

cku...@...
 

Re: Review: compiler failed to mark a const as "not written" (issue195084)

cliffo...@...
 

On this one, I'll have to take your word for it. LGTM!

http://codereview.appspot.com/195084/show
Re: Compiling OpenShadingLanguage under Windows

Oleg <ode...@...>
 

Hi Jeremy,

Sure, you can do it. It would be great.

Oleg

On 28 Jan., 00:23, Wormszer <wo...@...> wrote:
Mcpp integrated pretty easily with the existing structure. I can send  
you the project I made for it. Unfortunatly I wasn't able to get it to  
build on vs with cpp syntax. I think it assumed some other things  
would be around that are installed with gcc.

I was thinking I might send you a patch first and you can apply it to  
a clean checkout and see if you hit any issues. Then I can submit it  
to the osl dev.

I think we are a minority on windows and it might be worth trying it  
on a second windows machine before saying it works.

I will be back at home tomorrow and will send you the project file.

You could look into the print precision issue I mentioned. I'm hoping  
that's just a simple fix and possibly the last issue.

Jeremy

Sent from my iPhone

On Jan 27, 2010, at 3:48 PM, Oleg <od...@...> wrote:

Hi Jeremy,
I was trying to use the boost.wave preprocessor, but still have some
troubles to integrate it. I was not able to run any tests so far.
However, I'm still trying :-) Maybe it would be useful if you submit
the project file for the mcpp as well.
Regards,
Oleg
On 27 Jan., 06:41, Wormszer <wo...@...> wrote:
I just checked one failed test file output and the error was
ref:
-5.96046e-08
out:
-5.96046e-008
I wonder if this is due to different default precision on cout  
possibly?
That was the only error so I am not sure.
Any ideas?
I guess the printing is being done in the shader, so maybe ill look  
there at
the printf operator. It was the only value printed using scientific
notation.
I imagine the others will probably be something similiar.
Jeremy
On Wed, Jan 27, 2010 at 12:31 AM, Wormszer <wo...@...>  
wrote:
I have made some modifications to runtest.py to allow for the  
tests to be
run on windows along with some changes to the cmake files.
I have redone the everything from scratch and have made changes to  
cmake
and the source that should make it easy to build on windows.
I have some extra instructions for setting up some of the  
dependencies and
then modified the cmake routines to find the external libraries as  
they come
from installing and building OIIO.
OIIO cmake routines still need manual help and I may go modify them,
instead of having to add a bunch of paths to CMAKE_INSTALL_PREFIX.
I can now run the tests as well but only 35% of them pass.
I was hoping that through including the flex and bison outputs in  
the
project the need for cygwin could be removed, but the testsuite  
uses diff so
it maybe here to stay.
To get around having to modify every test, and having to parse the  
command
lines to pull out the output file names and create a stdout to  
file pipe, i
decieded to write the command to a batch file and have python call  
that.
This worked except that my install folder is C:\program files  
(x86)\... and
of course the space was now giving me issues.
So i was able to parse the command strings and wrap them commands  
in quotes
and then write them to the batch file and run it.
Now of course that wasn't the end of it. The next issue is that  
windows
uses cr/lf i guess through > even though the output should be  
writing just
lf, so diff was puking on that.
So on windows the diff command has to take an extra cmd parameter
--strip-trailing-cr
So now I can run the tests, I will investigate the errors and then  
should
be ready to submit a patch.
Jeremy
--
You received this message because you are subscribed to the Google  
Groups "OSL Developers" group.
To post to this group, send email to osl...@....
To unsubscribe from this group, send email to osl...@...
.
For more options, visit this group athttp://groups.google.com/group/osl-dev?hl=en
.
Re: Review: compiler failed to mark a const as "not written" (issue195084)

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

Right before checking in, I noticed that the same mistake could happen for ordinary loops:

while (1) { }
or
do { } while(0);

I just fixed in in both places in the identical way.


On Jan 28, 2010, at 8:50 AM, <cliffo...@...> <cliffo...@...> wrote:

On this one, I'll have to take your word for it. LGTM!

http://codereview.appspot.com/195084/show
--
Larry Gritz
l...@...
Review: nan/inf checking (issue194142)

larry...@...
 

Reviewers: ,

Description:
This change allows a debug mode (set by attribute("debugnan",1)) that
checks every value written by each instruction, and if any nan or inf
values are found, issues a warning pointing to exactly which shader
source line it happened on. It's quite expensive, nobody should use it
for ordinary rendering, but it's real helpful if you are trying to debug
NaNs in your renderer, this will quickly narrow it down to the
particular instruction.

(SPI folks: a corresponding renderer-side review will come to you
shortly.)

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

Affected files:
src/liboslexec/exec.cpp
src/liboslexec/oslexec_pvt.h
src/liboslexec/shadingsys.cpp
Re: Review: nan/inf checking (issue194142)

cku...@...
 

How about checking the results of geometry binding as well?

Otherwise LGTM

http://codereview.appspot.com/194142/show
Re: Review: nan/inf checking (issue194142)

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

You mean checking all the inputs? Sure. But let me check this one in first.


On Jan 29, 2010, at 12:20 PM, <cku...@...> wrote:

How about checking the results of geometry binding as well?

Otherwise LGTM

http://codereview.appspot.com/194142/show
--
Larry Gritz
l...@...
Review: more nan/inf checking (issue196066)

larry...@...
 

Reviewers: ,

Description:
At Chris Kulla's suggestion, expand our "debugnan" checking to include
the binding step, to catch any bad values passed into the shading
system.


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

Affected files:
src/liboslexec/exec.cpp
src/liboslexec/oslexec_pvt.h
Thirdparty/Utilties in trunk

Wormszer <worm...@...>
 

Part of the windows build now requires the use of MCPP to do the OSL preprocessing.
There is a binary available but unfortunatly it doesn't support a necessary command line parameter.
 
So I had to build it and they don't provide a project for it.
 
Is there a place in the trunk that I could add a cmakefile and header or instructions that could be used to generate a project.
I don't think we necesarrily need to include the source, but if the whole project switched to mcpp it might be worth it.
Until then you could just download the source, copy the header and cmakefile and generate the project to build the correct version of MCPP to work with OSL.
 
Jeremy
 

 
Re: Compiling OpenShadingLanguage under Windows

Wormszer <worm...@...>
 

I included the project file for mcpp. I plan to generate a cmake file for it.

I have updated the cmakefiles for osl to hopefully work better on windows.
They assume that there is an thirdpartyhome variable set to the external/dist/windows folder from the OIIO external setup from this page.
 
Other things need to have some envrionment variables setup, and it looks for the OIIO install path location.
I included the document i was creating on the steps I had to do when I redid everything.
 
flex, bison, mcpp all assume to be in your path.
 
There maybe still some issues with the tests and the way they assume where the executables are.
Im not sure if i rolled those into the cmakefiles to handle that. And there is the precision problem.
 
I haven't cheked out the latest source in the last week or so, so hopefully any new commits won't cause any issues.
Let me know what does or doesnt work for you. And then I can fix it and submit a patch.
 
 
Jeremy

 

On Thu, Jan 28, 2010 at 4:00 PM, Oleg <ode...@...> wrote:
Hi Jeremy,

Sure, you can do it. It would be great.

Oleg

On 28 Jan., 00:23, Wormszer <wo...@...> wrote:
> Mcpp integrated pretty easily with the existing structure. I can send  
> you the project I made for it. Unfortunatly I wasn't able to get it to  
> build on vs with cpp syntax. I think it assumed some other things  
> would be around that are installed with gcc.
>
> I was thinking I might send you a patch first and you can apply it to  
> a clean checkout and see if you hit any issues. Then I can submit it  
> to the osl dev.
>
> I think we are a minority on windows and it might be worth trying it  
> on a second windows machine before saying it works.
>
> I will be back at home tomorrow and will send you the project file.
>
> You could look into the print precision issue I mentioned. I'm hoping  
> that's just a simple fix and possibly the last issue.
>
> Jeremy
>
> Sent from my iPhone
>
> On Jan 27, 2010, at 3:48 PM, Oleg <od...@...> wrote:
>
> > Hi Jeremy,
>
> > I was trying to use the boost.wave preprocessor, but still have some
> > troubles to integrate it. I was not able to run any tests so far.
> > However, I'm still trying :-) Maybe it would be useful if you submit
> > the project file for the mcpp as well.
>
> > Regards,
> > Oleg
>
> > On 27 Jan., 06:41, Wormszer <wo...@...> wrote:
> >> I just checked one failed test file output and the error was
> >> ref:
> >> -5.96046e-08
> >> out:
> >> -5.96046e-008
> >> I wonder if this is due to different default precision on cout  
> >> possibly?
> >> That was the only error so I am not sure.
>
> >> Any ideas?
>
> >> I guess the printing is being done in the shader, so maybe ill look  
> >> there at
> >> the printf operator. It was the only value printed using scientific
> >> notation.
>
> >> I imagine the others will probably be something similiar.
>
> >> Jeremy
>
> >> On Wed, Jan 27, 2010 at 12:31 AM, Wormszer <wo...@...>  
> >> wrote:
> >>> I have made some modifications to runtest.py to allow for the  
> >>> tests to be
> >>> run on windows along with some changes to the cmake files.
>
> >>> I have redone the everything from scratch and have made changes to  
> >>> cmake
> >>> and the source that should make it easy to build on windows.
> >>> I have some extra instructions for setting up some of the  
> >>> dependencies and
> >>> then modified the cmake routines to find the external libraries as  
> >>> they come
> >>> from installing and building OIIO.
> >>> OIIO cmake routines still need manual help and I may go modify them,
> >>> instead of having to add a bunch of paths to CMAKE_INSTALL_PREFIX.
>
> >>> I can now run the tests as well but only 35% of them pass.
>
> >>> I was hoping that through including the flex and bison outputs in  
> >>> the
> >>> project the need for cygwin could be removed, but the testsuite  
> >>> uses diff so
> >>> it maybe here to stay.
>
> >>> To get around having to modify every test, and having to parse the  
> >>> command
> >>> lines to pull out the output file names and create a stdout to  
> >>> file pipe, i
> >>> decieded to write the command to a batch file and have python call  
> >>> that.
> >>> This worked except that my install folder is C:\program files  
> >>> (x86)\... and
> >>> of course the space was now giving me issues.
>
> >>> So i was able to parse the command strings and wrap them commands  
> >>> in quotes
> >>> and then write them to the batch file and run it.
>
> >>> Now of course that wasn't the end of it. The next issue is that  
> >>> windows
> >>> uses cr/lf i guess through > even though the output should be  
> >>> writing just
> >>> lf, so diff was puking on that.
> >>> So on windows the diff command has to take an extra cmd parameter
> >>> --strip-trailing-cr
>
> >>> So now I can run the tests, I will investigate the errors and then  
> >>> should
> >>> be ready to submit a patch.
>
> >>> Jeremy
>
> > --
> > You received this message because you are subscribed to the Google  
> > Groups "OSL Developers" group.
> > To post to this group, send email to osl...@....
> > To unsubscribe from this group, send email to osl...@...
> > .
> > For more options, visit this group athttp://groups.google.com/group/osl-dev?hl=en
> > .

--
You received this message because you are subscribed to the Google Groups "OSL Developers" group.
To post to this group, send email to osl...@....
To unsubscribe from this group, send email to osl...@....
For more options, visit this group at http://groups.google.com/group/osl-dev?hl=en.


?
oiio updates for osl win32.patch
?
mcpp_vs.vcproj
?
noconfig.H
?
osl patch win32.patch
?
Open Shading Language build instructions for windows.docx

Fixes in westin_sheen closure (issue194147)

aco...@...
 

Reviewers: osl-dev_googlegroups.com,

Description:
We were forgetting to set the pdf in the eval function, plus we were
using the wrong cosine in its sampler.

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

Affected files:
src/liboslexec/bsdf_westin.cpp


Index: src/liboslexec/bsdf_westin.cpp
===================================================================
--- src/liboslexec/bsdf_westin.cpp (revision 556)
+++ src/liboslexec/bsdf_westin.cpp (working copy)
@@ -150,7 +150,8 @@ public:
float cosNI = normal_sign * m_N.dot(omega_in);
if (cosNO > 0 && cosNI > 0) {
float sinNO2 = 1 - cosNO * cosNO;
- float westin = sinNO2 > 0 ? powf(sinNO2, 0.5f * m_edginess) * cosNI * float(M_1_PI) : 0;
+ pdf = cosNI * float(M_1_PI);
+ float westin = sinNO2 > 0 ? powf(sinNO2, 0.5f * m_edginess) * pdf : 0;
return Color3 (westin, westin, westin);
}
return Color3 (0, 0, 0);
@@ -174,7 +175,7 @@ public:
sample_cos_hemisphere (Nf, omega_out, randu, randv, omega_in, pdf);
if (Ngf.dot(omega_in) > 0) {
// TODO: account for sheen when sampling
- float cosNO = float(M_PI) * pdf;
+ float cosNO = Nf.dot(omega_out);
float sinNO2 = 1 - cosNO * cosNO;
float westin = sinNO2 > 0 ? powf(sinNO2, 0.5f * m_edginess) * pdf : 0;
eval.setValue(westin, westin, westin);
Re: Fixes in westin_sheen closure (issue194147)

cku...@...
 

Re: Review: more nan/inf checking (issue196066)

cku...@...
 

http://codereview.appspot.com/196066/diff/1/3
File src/liboslexec/exec.cpp (right):

http://codereview.appspot.com/196066/diff/1/3#newcode345
src/liboslexec/exec.cpp:345: if (debugnan && check_nan (sym, NULL, 0,
m_npoints-1, badval))
Can you pass the original run flags here? Otherwise you are checking
points that might not be valid.

http://codereview.appspot.com/196066/diff/1/3#newcode384
src/liboslexec/exec.cpp:384: if (!get_renderer_userdata (m_npoints,
wants_derivatives,
It would be good to check the results here too.

http://codereview.appspot.com/196066/show
Re: Review: more nan/inf checking (issue196066)

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

Good call on both counts. I will upload an updated review shortly.

-- lg


On Feb 1, 2010, at 8:26 AM, <cku...@...> <cku...@...> wrote:


http://codereview.appspot.com/196066/diff/1/3
File src/liboslexec/exec.cpp (right):

http://codereview.appspot.com/196066/diff/1/3#newcode345
src/liboslexec/exec.cpp:345: if (debugnan && check_nan (sym, NULL, 0,
m_npoints-1, badval))
Can you pass the original run flags here? Otherwise you are checking
points that might not be valid.

http://codereview.appspot.com/196066/diff/1/3#newcode384
src/liboslexec/exec.cpp:384: if (!get_renderer_userdata (m_npoints,
wants_derivatives,
It would be good to check the results here too.

http://codereview.appspot.com/196066/show
--
Larry Gritz
l...@...
141 - 160 of 4975