-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix LOD selection for image-based lighting #12070
Conversation
Thank you for the pull request, @jjhembd! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @jjhembd! Just a few questions from me.
@@ -68,7 +68,7 @@ vec3 czm_sampleOctahedralProjectionWithFiltering(sampler2D projectedMap, vec2 te | |||
* @returns {vec3} The color of the cube map at the direction. | |||
*/ | |||
vec3 czm_sampleOctahedralProjection(sampler2D projectedMap, vec2 textureSize, vec3 direction, float lod, float maxLod) { | |||
float currentLod = floor(lod + 0.5); | |||
float currentLod = floor(lod); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the fix is resulting in better appearances for sure. But any idea why there was a 0.5
increment in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this dates back to before we were interpolating the mip level. In this previous version we were simply rounding to the nearest mip, which required the 0.5
increment. 006d859 added trilinear filtering but neglected to remove the 0.5
when selecting the "currentLod".
this._maximumMipmapLevel = length - 1; | ||
const cubeMaps = (this._cubeMaps = new Array(length)); | ||
const mipTextures = (this._mipTextures = new Array(length)); | ||
const mipLevels = cubeMapBuffers.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth capping this to a higher number than 6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few values with mixed results. My choice of no cap was based on 2 factors:
- The higher-numbered LODs are the lower-resolution ones, so including them adds very little memory overhead.
- If a user tries the same environment map at different resolutions, they would expect to see differences only in the high-res details of the reflections (mip level
0
). However, if we cap the max LOD, different input resolutions will result in a different mip being used for the background low-res part of the reflection.
@ggetz I think I answered your questions. Let me know if any further clarification is needed. |
Quick check of the Water Bottle sample model. Note the double reflection for the sun. This is caused by #12027 |
@ggetz These are clipping artifacts where the I tried setting I think these issues will be resolved when we address #12027. |
Got it. Thanks @jjhembd! |
Description
This PR fixes two errors in LOD sampling of environment maps for image-based lighting:
czm_sampleOctahedralProjection
, the interpolation of mip levels was sometimes starting from the wrong lower-bound mip level.OctahedralProjectedCubeMap
, the number of mip levels was limited to 6. This limit meant that the lowest-resolution versions of the environment map were not used. For rough materials lit by high-res environment maps, this could sometimes result in specular reflections with too much detail.The improvement in LOD sampling from this PR is most visible on the Barn Lamp sample model, which has a lot of variation in the material roughness.
Before this PR:
After fixing mip level interpolation:
After allowing more mip levels:
Clearcoat Wicker model, before this PR:
After fixing mip level interpolation:
After allowing more mip levels:
Note how the specular reflection is slightly broader and brighter when an appropriately low-res version of the environment map is used.
Issue number and link
Fixes #12069.
Testing plan
Load the glTF PBR Extensions Sandcastle and compare the results to the same Sandcastle in main. Most models should show some improvement, but the effect is most dramatic on the "Barn Lamp" model.
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change