Cycles: Use faster and exact GGX VNDF sampling algorithm
ClosedPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Tue, Jan 24, 4:19 AM.

Details

Reviewers

Brecht Van Lommel (brecht)
Weizhen Huang (weizhen)

Commits

rBe308b891c882: Cycles: Use faster and exact GGX VNDF sampling algorithm

Summary

Based on "Sampling the GGX Distribution of Visible Normals" by Eric Heitz (https://jcgt.org/published/0007/04/01/).

Also, this removes the lambdaI computation from the Beckmann sampling code and just recomputes it below. We already need to recompute for two other cases (GGX and clearcoat), so this makes the code more consistent.
In terms of performance, I don't expect a notable impact since the earlier computation also was non-trivial, and while it probably was slightly more accurate, I'd argue that being consistent between evaluation and sampling is more important than absolute numerical accuracy anyways.

Note that this depends on (and includes) D17099, but since Phabricator doesn't handle multi-commit patches well I'm uploading it like this.

Diff Detail

Repository

rB Blender

Branch

microfacet-cleanup-2 (branched from master)

Build Status

Buildable 25534
Build 25534: arc lint + arc unit

Event Timeline

Lukas Stockner (lukasstockner97) requested review of this revision.Tue, Jan 24, 4:19 AM

Lukas Stockner (lukasstockner97) created this revision.

Harbormaster completed remote builds in B25534: Diff 59848.Tue, Jan 24, 4:19 AM

Looks all good to me, a few minor remarks:

intern/cycles/kernel/closure/bsdf_microfacet.h
68	Could we make utility functions out of this? Seem to be used often, see `sin_from_cos` in tree.h and `cos_from_sin` in bsdf_hair_principled.h
156	We have `inversesqrtf()`
215	You mean the paper Understanding the Masking-Shadowing Function in Microfacet-Based BRDFs, Eric Heitz, Journal of Computer Graphics Techniques 3.2 (2014)?
217	In the paper just below Eq 69 (and Eq 72) there is a = 1/(alpha tan), so I think it's fine not to comment on this? Just a minor remark from me, doesn't matter much.

Updated based on review.

Harbormaster completed remote builds in B25542: Diff 59866.Tue, Jan 24, 3:41 PM

Looks good, thank you! Please separate the part using the new sin_from_cos() in the other files as another commit.

This revision is now accepted and ready to land.Tue, Jan 24, 5:29 PM

Lukas Stockner (lukasstockner97) added inline comments.Tue, Jan 24, 5:30 PM

intern/cycles/kernel/closure/bsdf_microfacet.h
68	Good point, I moved them to `math.h` and replaced the obvious uses I could find. I kept both `sin_from_cos` and `cos_from_sin` - it's the same operation of course, but I couldn't come up with a good generic name. `convert_sin_cos` maybe? Also, there's a few cases where the code already has the squared value, I kept those as-is for now instead of adding `sin_from_cos2` and `cos_from_sin2` as well.
215	Ah, yeah, I had moved the comment from below but I mixed the papers up. This reference is supposed to go with the Beckmann sampling.
217	Yeah, this is quite obvious I guess. I'll remove it.