> On Dec. 2, 2012, 9:23 a.m., Celierra Darling wrote:
> > indra/newview/app_settings/settings.xml, line 7823
> > <http://codereview.secondlife.com/r/612/diff/1/?file=8132#file8132line7823>
> >
> >     I still think it might be valuable to attenuate (HSV) saturation, but 
> > since it'd been stuck at 1.0 since forever and it doesn't look like it'll 
> > be exposed in Environment Settings anytime soon... *shrug*
> >     
> >     Used like this, RenderSSAOEffect might be entirely redundant with 
> > RenderSSAOFactor, if I remember correctly?

I don't think HSV-modulating according to SSAO is valuable (either 
aesthetically or physically) - some sort of colour-grading function would be 
grand but it shouldn't be tied to SSAO.  Only the 'V' part has ever been 
enabled (as released) as you say...
RenderSSAOEffect and RenderSSAOFactor are unrelated.


> On Dec. 2, 2012, 9:23 a.m., Celierra Darling wrote:
> > indra/newview/app_settings/shaders/class1/deferred/softenLightF.glsl, line 
> > 214
> > <http://codereview.secondlife.com/r/612/diff/1/?file=8134#file8134line214>
> >
> >     Where's the code where ssao_effect_mat had been constructed? (If it's 
> > not being used, it should probably be taken out too.)

It was being constructed in pipeline.cpp, and the patch includes removal of 
that part.


> On Dec. 2, 2012, 9:23 a.m., Celierra Darling wrote:
> > indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl, line 
> > 127
> > <http://codereview.secondlife.com/r/612/diff/1/?file=8137#file8137line127>
> >
> >     The 'if' here might be problematic.. I remember some cards were choking 
> > on branches, thus the convoluted lines that looked like new = old + 
> > int(conditional)*value.  (same for class1)
> >     
> >     Also, I could have sworn that there used to be comments here specifying 
> > what the non-mangled math originally was (a la the old 
> > softenLightF.glsl:206-214)....

Our shaders are really branchy, that's really not a problem (on the target 
class).  I don't strictly remember removing comments on the original maths, but 
the weighting (the only mathy part of this affair really) has changed radically 
at least twice since the original inline explanation, and should be simple 
enough to follow procedurally lately. :)


> On Dec. 2, 2012, 9:23 a.m., Celierra Darling wrote:
> > indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl, line 
> > 130
> > <http://codereview.secondlife.com/r/612/diff/1/?file=8137#file8137line130>
> >
> >     Won't using norm.xyz*diff raw like this cause false positives (from 
> > self-occlusion)?  IIRC, that was why the old code used 
> > dot(samp-0.05*norm-pos, norm).  (todo for self: render this for one sample 
> > to check...)  (same for class1)

I'm not sure I follow - but a sample candidate co-incident with the sample 
origin is disregarded.


> On Dec. 2, 2012, 9:23 a.m., Celierra Darling wrote:
> > indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl, line 
> > 132
> > <http://codereview.secondlife.com/r/612/diff/1/?file=8137#file8137line132>
> >
> >     Out of curiosity, why a minimum, instead of ex. "(angrel>0) ? distrel : 
> > 0.0" ?  Seems odd in cases of 0 < angrel < distrel.  (No longer using the 
> > sphere assumption?)

The reasoning was that as a sample candidate draws nearer to the sample origin, 
the relevancy of its relative angle becomes overshadowed (so to speak :)), 
given that it's not really a point but a sampling of something (i.e. the 
sphere) with size whose extents are also increasingly blocking off the local 
area.  Conversely, all distances being equal, the occlusion of the sample 
origin is proportional to the directness with which the sample candidate blocks 
its normal (the direction from which it *would* otherwise be drawing the most 
light).
So the two relevancies modulate either other, the 'correct' mixing of the two 
in my head would be sqrt(angrel*distrel) but max(angrel,distrel) seemed a touch 
more pleasing and probably quicker.


- Tofu


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/612/#review1288
-----------------------------------------------------------


On Dec. 2, 2012, 3:49 a.m., Tofu Buzzard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/612/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2012, 3:49 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Description
> -------
> 
> Use a different scheme for weighting SSAO samples, apply SSAO before fog is 
> applied, fix a bug in the screen-space shadow/ssao smoothing offset where the 
> 'checkerboard' stipple had been refactored incorrectly, change some default 
> settings in line with the resulting visual changes.  Also, improve comments a 
> bit. :3
> 
> 
> This addresses bug VWR-29083.
>     http://jira.secondlife.com/browse/VWR-29083
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt UNKNOWN 
>   indra/llrender/llshadermgr.h UNKNOWN 
>   indra/llrender/llshadermgr.cpp UNKNOWN 
>   indra/newview/app_settings/settings.xml UNKNOWN 
>   indra/newview/app_settings/shaders/class1/deferred/blurLightF.glsl UNKNOWN 
>   indra/newview/app_settings/shaders/class1/deferred/softenLightF.glsl 
> UNKNOWN 
>   indra/newview/app_settings/shaders/class1/deferred/sunLightSSAOF.glsl 
> UNKNOWN 
>   indra/newview/app_settings/shaders/class2/deferred/softenLightF.glsl 
> UNKNOWN 
>   indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl 
> UNKNOWN 
>   indra/newview/pipeline.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/612/diff/
> 
> 
> Testing
> -------
> 
> Been running with this for months on an assortment of nvidia hardware, linux 
> only.
> 
> 
> Thanks,
> 
> Tofu Buzzard
> 
>

_______________________________________________
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges

Reply via email to