Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to prefiltered environment maps #1420

Merged

Conversation

ApoorvaJ
Copy link
Contributor

As discussed with @jstone-lucasfilm, MaterialXView's pre-integrated IBL path (i.e. when FIS is turned off) is broken. This PR attempts to fix this issue.

image

Highlights of this PR

  1. In mx_environment_prefilter.glsl, we've moved from the more accurate Fresnel term to Schlick's approximation. We do this because the split-sum approximation for IBL relies on it:

image
Source: Moving Frostbite to PBR (PDF)

  1. Before this PR, MaterialXView uses the default mip-mapping filter on the environment radiance map. This also needs to be changed for correct IBL. In this PR, I pre-convolve the environment radiance map when it is first requested, to generate the correct LD term. I do this for various roughness values and store the results in different mip levels. I hold on to the non-preconvolved map, which is needed when FIS is on, which results in numerical integration. The bulk of the pre-convolution shader code is in mx_pre_convolve_environment.glsl.

Request for review

I'd like to specifically request review of the following:

  1. mx_pre_convolve_environment.glsl - There might be opportunities here to use some bits of existing code, instead of writing stuff from scratch. E.g. get_smith_joint_ggx_part_lambda_v() might be able to be formulated using existing code.
  2. I've tested with MaterialXView + Metal on Mac, MaterialXView + OpenGL on Mac, and MaterialXView + OpenGL on Windows. I haven't tested other potential backends and MaterialXGraphEditor.
  3. Image.cpp - I think the getMaxMipCount() function was wrong, and it caused a crash in my code. I believe I've fixed it, but worth a double-check.
  4. MetalTextureHandler.mm - I needed mipmaps on a renderable texture. I don't know if this change has any deeper repercussions.
  5. RenderPipelineGL.cpp, RenderPipelineMetal.mm - I had some trouble with your API abstraction layer, so I might be doing it wrong, or using the API directly.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 19, 2023

@ApoorvaJ ApoorvaJ changed the title Preconvolve IBL Improve rough reflections in Image Based Lighting Jul 19, 2023
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an excellent proposal, @ApoorvaJ, and my main recommendation would be to leverage existing GLSL helper functions where possible, so that we don't end up with multiple implementations to optimize and maintain going forward.

As one example, I see this new implementation of height-correlated GGX/Smith masking shadowing:
https://github.com/AcademySoftwareFoundation/MaterialX/pull/1420/files#diff-eb403937b2a487a7fdf5a66ef7476114dd764cebd1cf209cebc77b5f59f1f4f8R88

And if it's possible to leverage (or extend) our existing implementation, that would be better:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/pbrlib/genglsl/lib/mx_microfacet_specular.glsl#L99

Similarly, I see this new implementation of GGX NDF sampling:
https://github.com/AcademySoftwareFoundation/MaterialX/pull/1420/files#diff-eb403937b2a487a7fdf5a66ef7476114dd764cebd1cf209cebc77b5f59f1f4f8R54

And ideally we'd want to leverage (or extend) the existing GGX VNDF sampling that is used elsewhere in the MaterialX GLSL codebase:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/pbrlib/genglsl/lib/mx_microfacet_specular.glsl#L65

In the latter example, we might see improved performance and convergence as well, if the existing VNDF sampling function can be used, but let me know if I'm missing any key differences in the new use case that would require a second helper method.

@jstone-lucasfilm jstone-lucasfilm changed the title Improve rough reflections in Image Based Lighting Improvements to prefiltered environment maps Dec 29, 2023
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this great work, @ApoorvaJ, and I believe this is now ready to merge. My own recent changes have been focused on aligning the GGX importance sampling with our existing approach in MaterialX, using the VNDF distribution and filtered samples.

@jstone-lucasfilm jstone-lucasfilm merged commit 3580bb7 into AcademySoftwareFoundation:main Dec 29, 2023
31 checks passed
jstone-lucasfilm pushed a commit that referenced this pull request Dec 29, 2023
This changelist adds support for prefiltering environment maps on the GPU, using filtered VNDF sampling for efficiency.

The MaterialX Viewer can be used to test the new functionality by unchecking the Environment FIS option in Advanced Settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants