STORM-1823 - Replace current specular model with Normalized Blinn-Phong specularity | Review Request


Submitter:	Geenz Spad	Reviewers
Branch:		Groups:	viewer
Bugs:	STORM-1823	People:	davep
Change Number:	None	Repository:	viewer-beta

Description:

For a while now, Second Life's deferred renderer has had a somewhat "toonish" looking specular model, as opposed to other platforms which try to go for more physically accurate looking models, such as blinn-phong and similar.

This feature changes the specular model to a technique called normalized blinn-phong. The specular model applies the usual blinn-phong term (assume NdotV to the power of n), multiplied by a normalization function of:
((n + 2) * (n + 4)) / (8 * PI * (powf(2, -n/2) + n))
Where n is the specular exponent.

Gamma correction is also applied to the result to bring the visual results more in line with what you see in high end game engines such as Unreal Engine 3 and Frostbite 2, and stored in a lookup texture.

Test plan can be found in the JIRA.
Repository can be found here: https://bitbucket.org/Geenz/viewer-nbp

Testing Done:

* Tested performance characteristics - both Normalized Blinn-Phong and the previous model seem to have the same performance
* Checked the perceived "brightness" of the new highlights vs. the old ones - new ones seem much brighter at higher shiny values than the old ones, and seem much less "cartoonish" as such
* Checked the impact on different bumpiness values at the test rig at Hippo Hollow - noticeable visual improvement across all objects (though low shiny seems to have a much more subtle light reflectance term - this is expected for normalized blinn-phong)
* Tested different LUT resolutions - best looking seems to be 512x128 with seemingly no measurable decrease in performance

You can find the latest build here: http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/oz_project-8/latest.html

Expand All

Testing Done

Changed from:

* Tested performance characteristics - both Normalized Blinn-Phong and the previous model seem to have the same performance
* Checked the perceived "brightness" of the new highlights vs. the old ones - new ones seem much brighter at higher shiny values than the old ones, and seem much less "cartoonish" as such
* Checked the impact on different bumpiness values at the test rig at Hippo Hollow - noticeable visual improvement across all objects (though low shiny seems to have a much more subtle light reflectance term - this is expected for normalized blinn-phong)
* Tested different LUT resolutions - best looking seems to be 512x128 with seemingly no measurable decrease in performance

Changed to:

* Tested performance characteristics - both Normalized Blinn-Phong and the previous model seem to have the same performance
* Checked the perceived "brightness" of the new highlights vs. the old ones - new ones seem much brighter at higher shiny values than the old ones, and seem much less "cartoonish" as such
* Checked the impact on different bumpiness values at the test rig at Hippo Hollow - noticeable visual improvement across all objects (though low shiny seems to have a much more subtle light reflectance term - this is expected for normalized blinn-phong)
* Tested different LUT resolutions - best looking seems to be 512x128 with seemingly no measurable decrease in performance

You can find the latest build here: http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/oz_project-8/rev/251711/index.html

Description:

Added a link to the latest builds of these changes.

Diff
- added Diff r2

Description:

Updated the diff to generally be cleaner, consolidating all changes into a single diff.

Been running with this patch for about two days with no negative effect. Performance is more or less the same with much improved visuals, exceptionally well in bright lit areas (where previously, the light would almost wash out my avatar in white light)

I'd like to see the mis-named handleReleaseLUTBufferChanged be called handleLUTBufferChanged, and ideally I'd like some comment (probably in the LUT creation) about why the results of lookups should be multiplied by 4 (to avoid saturation in the LUT?).
Otherwise, I reckon this is great.

Oz Linden March 25, 2012, 11:08 a.m. (March 25, 2012, 11:08 a.m.)

I concur with both of these comments, and have asked for a review from Dave.

Geenz Spad March 25, 2012, 2:10 p.m. (March 25, 2012, 2:10 p.m.)

The reasoning for doing this is basically just as you said, to avoid saturation in the LUT since most energy conserving specular models tend to exceed a floating point range of 0 to 1, something that should be accounted for when storing the results in an R8 LUT.
Granted, one could just use a floating point LUT instead, but driver support can be a bit spotty for R16F and R32F.  However, storing in an R16F LUT is something to look into in the future regardless if only to reduce banding.
Scaling by 6 allows us to get a bit more of a higher range (same scale used by RGBM encoding), though whether or not it's really necessary is something potentially worth investigating.

Reviewers (People)
- added davep

Diff
- added Diff r3

Description:

Added more information as to why we multiply in our shaders, and why we're scaling our results by (1.f / 6) in our LUT creation.
Also renamed handleReleaseLUTBufferChanged to handleLUTBufferChanged, per Tofu's request.

Could we see some examples of SL scenes using the two models, particularly with avatars in them, because there have been a number of changes to the SL renderer over the years... and the main effect of increasing the "realism" of the renderer has been to throw the deficiencies of the SL avatar mesh into sharp relief. The current specular model was a deliberate compromise between the older even-toonier renderer and a more "realistic" model that made avatars look horrible.

Geenz Spad March 25, 2012, 3:36 p.m. (March 25, 2012, 3:36 p.m.)

Avatars are only ever effected if they're using attachments that make sure of the shiny attribute.

The diffuse shading on avatars remains unaffected.  I think the "compromise" you're thinking of is the environment map that typically gets applied in classic forward rendering, which was later re-added to the deferred renderer to mitigate content breakage that resulted when it was removed, which hasn't been changed with my modifications.

As I had mentioned previously, there are comparison pictures posted on the JIRA that everyone can see, and I would also very much appreciate it if people could post their own comparisons, and identify any in-world content breakage that relied on the previous model that was used.

4 -> 6 is a bit of a last-minute change... :3  But ship it.

Geenz Spad March 25, 2012, 3:49 p.m. (March 25, 2012, 3:49 p.m.)

Mostly just using an idea I got from reading up on RGBM encoding, and why they scale by 6 as well (though really, in theory you could scale by pretty much any value so long as both the encode and decode use the same scales).  Seems to work quite well, especially since medium shiny seems to have slightly less saturation induced artifacting. :3

indra/newview/pipeline.cpp (Diff revision 3)

Use F_PI here (yes, I know that was in the old code too)

Diff
- added Diff r4

Description:

Added the change that Oz requested, also updated contributions.txt while I was at it.

Diff
- added Diff r5

Description:

Reduce the overall glow amount from the sun to a quarter of its original strength.  This helps prevent specular highlights with glow applied from being too bright when compared to the previous specular model.

Testing Done

Changed from:

* Tested performance characteristics - both Normalized Blinn-Phong and the previous model seem to have the same performance
* Checked the perceived "brightness" of the new highlights vs. the old ones - new ones seem much brighter at higher shiny values than the old ones, and seem much less "cartoonish" as such
* Checked the impact on different bumpiness values at the test rig at Hippo Hollow - noticeable visual improvement across all objects (though low shiny seems to have a much more subtle light reflectance term - this is expected for normalized blinn-phong)
* Tested different LUT resolutions - best looking seems to be 512x128 with seemingly no measurable decrease in performance

You can find the latest build here: http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/oz_project-8/rev/251711/index.html

Changed to:

* Tested performance characteristics - both Normalized Blinn-Phong and the previous model seem to have the same performance
* Checked the perceived "brightness" of the new highlights vs. the old ones - new ones seem much brighter at higher shiny values than the old ones, and seem much less "cartoonish" as such
* Checked the impact on different bumpiness values at the test rig at Hippo Hollow - noticeable visual improvement across all objects (though low shiny seems to have a much more subtle light reflectance term - this is expected for normalized blinn-phong)
* Tested different LUT resolutions - best looking seems to be 512x128 with seemingly no measurable decrease in performance

You can find the latest build here: http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/oz_project-8/latest.html

Description:

Updated build link.

Ship It!

status changed from pending to submitted

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.

STORM-1823 - Replace current specular model with Normalized Blinn-Phong specularity