Hey Tofu! :3

indra/newview/app_settings/settings.xml (Diff revision 1)

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?

1. Tofu Buzzard Jan. 11, 2013, 12:39 p.m. (Jan. 11, 2013, 12:39 p.m.)

   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.

2. Celierra Darling Jan. 11, 2013, 2:55 p.m. (Jan. 11, 2013, 2:55 p.m.)

   Hmm, there is some effect where saturation (seems to) decrease on AO'd areas, but that can be more illusion than physical.  Tweaking it here would be either cancelling it or exaggerating it, and I can see the argument for avoiding it.  Though there are real cases of artificial narrow-spectrum direct lighting + fuller-spectrum ambient light, like gas-discharge streetlamps at dusk or the such - thus my selfish lust for it getting exposed in environment settings... :p
   
   Looking at your comment, I think -Factor might be being used for something different from when I was using it, so never mind that.

indra/newview/app_settings/shaders/class1/deferred/softenLightF.glsl (Diff revision 1)

Where's the code where ssao_effect_mat had been constructed? (If it's not being used, it should probably be taken out too.)

1. Tofu Buzzard Jan. 11, 2013, 12:39 p.m. (Jan. 11, 2013, 12:39 p.m.)

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

2. Celierra Darling Jan. 11, 2013, 2:55 p.m. (Jan. 11, 2013, 2:55 p.m.)

   Whoops, thanks; not sure how I missed it.

indra/newview/app_settings/shaders/class1/deferred/sunLightSSAOF.glsl (Diff revision 1)

This line can probably go too. (It got removed in class2 but not class1; maybe check for more?)

indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl (Diff revision 1)

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)....

1. Tofu Buzzard Jan. 11, 2013, 12:39 p.m. (Jan. 11, 2013, 12:39 p.m.)

   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. :)

2. Geenz Spad Jan. 11, 2013, 12:51 p.m. (Jan. 11, 2013, 12:51 p.m.)

   However, a general rule of thumb is if you can save a branch in a shader, then you should do so.  My personal preference is to try and keep it to branches that have defined ARB instructions that a vendor's GLSL compiler will likely optimize to (which is limited to less than and greater than unfortunately).  Though you're right, it's not much of a problem on class 2 hardware that can handle deferred.
   
   That said, is there a good reason for diff.z != 0.0 to *not* be diff.z > 0.0?

3. Tofu Buzzard Jan. 11, 2013, 1:24 p.m. (Jan. 11, 2013, 1:24 p.m.)

   Added comments which explain diff.z != 0.0 somewhat!

4. Tofu Buzzard Jan. 11, 2013, 1:25 p.m. (Jan. 11, 2013, 1:25 p.m.)

   And thanks for taking a look!

5. Celierra Darling Jan. 11, 2013, 2:55 p.m. (Jan. 11, 2013, 2:55 p.m.)

   I see I should have been paying closer attention to the changes! :P Sorry if I'm struggling to catch up a bit.
   
   The same branch is also in class 1 too, FYI.  I'm not sure what the definitions of the classes are these days....

indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl (Diff revision 1)

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)

1. Tofu Buzzard Jan. 11, 2013, 12:39 p.m. (Jan. 11, 2013, 12:39 p.m.)

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

2. Celierra Darling Jan. 11, 2013, 2:55 p.m. (Jan. 11, 2013, 2:55 p.m.)

   'angrel' and the larger sampling radius seem to have taken care of it.  This was referring to the case where, for example, a sample pixel is picked that's 0.01 m away from the origin on the same triangle, and with a little rounding error, the sample ends up looking like a huge occluder.  The old code has nothing analogous to 'angrel': there's an assumption that incoming ambient light is falling onto the sample origin equally from all directions. 

indra/newview/app_settings/shaders/class2/deferred/sunLightSSAOF.glsl (Diff revision 1)

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?)

1. Tofu Buzzard Jan. 11, 2013, 12:39 p.m. (Jan. 11, 2013, 12:39 p.m.)

   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.

2. Celierra Darling Jan. 11, 2013, 2:55 p.m. (Jan. 11, 2013, 2:55 p.m.)

   Thanks!  You may want to put in that you're not presuming uniform incoming ambient light like the old code was.  (Also, you forgot the sqrt in your comment if you were intending it to say the same thing as you said here?)

3. Celierra Darling Jan. 11, 2013, 3:08 p.m. (Jan. 11, 2013, 3:08 p.m.)

   Er, to be clearer, the old presumption was that ambient light would be falling onto the sample origin equally from all directions if not for the occluders that were sampled - thus no use of angrel.  In hindsight, that seems a little dumb - to me, the way you're doing it is like using a better prior in the Bayesian sense.