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

Fix issues with environment mapping #33668

Merged

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Nov 17, 2019

Fixes: #33616, fixes #21680, fixes #32125

May help: #19561

This PR addresses a few current issues with environment mapping. Tagged as 3.2. But since it is a relatively large change we may have to save it for 3.2.1. This PR will greatly benefit any project that makes use of the PBR workflow and especially projects that rely heavily on Image Based Lighting (IBL) (e.g. panoramas, HDRIs, etc.)

There are a few major pieces to this PR.

  1. Use proper irradiance map for diffuse IBL
  2. Use mipmaps for smoother radiance map generation
  3. Update generation of mipmapped radiance maps so mobile can properly enjoy IBL

1. Use proper irradiance map for diffuse IBL
Many users were finding that rough dielectrics were looking very shiny. As it turns out, we have been using a mip level of the radiance map instead of a proper irradiance map for diffuse lighting. This led to specular highlights from HDRIs appearing particularly strong I added a proper irradiance map as well as some other related code to the main scene shader in order to make dielectrics materials look proper.

Master

Master

This PR
PR

2. Use mipmaps for smoother radiance map generation
As described in https://chetanjags.wordpress.com/2015/08/26/image-based-lighting/ and https://learnopengl.com/PBR/IBL/Specular-IBL
When computing the radiance map, use the pdf of the distribution function to selectively read from higher mips. This results in substantially smoother radiance maps. I combined this with the old approach to further reduce the artifacts in the radiance map. With this change we can reduce the required number of samples to 512 from 1024, which should speed up radiance map generation on all devices.

3. Update generation of mipmapped radiance maps so mobile can properly enjoy IBL
I found that whenever rendering/quality/reflections/texture_array_reflections was false (default on mobile) the radiance map was all messed up. As it turned out, it was just suffering badly from sampling artifacts. I ported over the approach that was already used for texture arrays and mixed it with the new mipmap approach explained above. Now the mipmapped version of radiance maps looks just as nice as the array version and runs well on mobile. :)

Master (with texture_array_reflections off)
Master-low

This PR(with texture_array_reflections off and low quality ggx)
PR-Low

The Future
I hope that a few people with heavy PBR pipelines can test this out and spot regressions (especially if you make heavy use of IBL) CC @fracteed, @NHodgesVFX, @mysticfall

I will be porting this over to GLES2 shortly. For GLES2, I will likely use spherical harmonics instead of an irradiance map
I won't be adding any of this to GLES2 for now. Because GLES2 cannot handle HDR, the current radiance generation code works very well (even smoother than GLES3). Further, in LDR panoramas the Irradiance map and radiance maps are almost identical. So reading from a higher mip of the radiance map looks almost the same as using proper irradiance. Accordingly, I adjusted the mip level in the GLES2 backend and cleaned up the PBR code, but other than that I will be leaving it. In the future, I will add spherical harmonics for better diffuse irradiance, but that is a new feature and shouldn't be worked added until after the feature freeze ends.

Credit goes to @fracteed for spurning on this work and for providing the very helpful test scene :)

@clayjohn clayjohn added this to the 3.2 milestone Nov 17, 2019
@clayjohn clayjohn force-pushed the Fix_environment_mapping_issues branch from 4299756 to 653b075 Compare November 17, 2019 05:54
@NeoSpark314
Copy link
Contributor

From what you describe this might also fix the second half of #33633 (second level of the environment map mip was black on android). I will try to check this (but I can't compile android on master currently due to the opus update causing linker errors).

@NHodgesVFX
Copy link
Contributor

NHodgesVFX commented Nov 17, 2019

I tested this to see if it improves #33616 unfortunately it doesn't solve that problem. I also checked using fracteed scene, like you did above, and it seems to match your results. I did not test android though.

For #33616 here is a before and after

Godot 3.2 Beta 1:
Godot_3 2Beta1_Rough

Godot 3.2 This PR:
Godot_3 2PR_Rough

Thank you for working on this :)

@NeoSpark314
Copy link
Contributor

I cherry picked the commit; but just have seen that it is not yet ported to GLES2; once it's there for GLES2 I can test for #33633 on android (oculus quest).

@clayjohn
Copy link
Member Author

@NHodgesVFX I wonder if your issue and #19561 are connected. As you can see even in fracteed's scene the reflections on the edge of the rings are too strong. Unfortunately, it may be a completely different issue. Have you tried your head model in GLES2? From what I can see the specular reflections are a lot more subtle there. If it is better in GLES2 then it will make it a lot easier to identify the problem.

@NeoSpark314 to test now you can test that issue with a panorama sky and make sure "mipmaps" are selected on import. Otherwise I should have an update soonish. (I still think your precision highp PR is needed though)

@clayjohn clayjohn force-pushed the Fix_environment_mapping_issues branch from 653b075 to 91b9624 Compare November 17, 2019 23:34
@clayjohn
Copy link
Member Author

@NeoSpark314 I realized that the changes aren't really needed on GLES2, so I didn't force mipmap generation on PanoramaSkies. This PR won't help #33633 anymore. Sorry. :(

@clayjohn clayjohn marked this pull request as ready for review November 17, 2019 23:39
@clayjohn clayjohn requested a review from reduz as a code owner November 17, 2019 23:39
@clayjohn clayjohn force-pushed the Fix_environment_mapping_issues branch from 91b9624 to 2f57bc4 Compare November 18, 2019 06:08
@NeoSpark314
Copy link
Contributor

@clayjohn no worries; thanks for the update; I'll try and see if I can find the cause of the black mip map level in #33633.

@fracteed
Copy link

Thanks for your work on this @clayjohn it is looking like a great improvement! I have been having a play with your branch, and it looks good to me. Here is a comparison test using a more moderate hdri ,with the current master on left and your fix on the right. The dielectrics look much more correct and realistic.

comparison_4

I also did a few tests between eevee and your fix (eevee on left, godot fix on right) using 2 other hdri's in the following 2 images. It is generally looking a bit nicer than eevee imo, and very close to substance painter. There is one oddity and that is the hard line on the left hand side of mesh. It is very visible with the existing demo hdri but it is still there with any hdri. Would be good to get rid of this if possible?

comparison_1

comparison_3

The halo effect we previously discussed is very subtle using other hdri's so I don't think it is a concern.
Have also tried it with my game and am happy with the result!

@reduz
Copy link
Member

reduz commented Nov 19, 2019

This looks good, the line may an arctifact related to using dual parabolloid. Would be interesting if you port this to Master branch and test there too.

@clayjohn
Copy link
Member Author

@reduz, this is on master. But I will port this to Vulkan as well to see the benefits there. :)

@fracteed @reduz I didn't notice that line while testing. It does indeed look like a bug from using dual paraboloid. When you are seeing that line is it only when using orthogonal projection? Admittedly, while testing I only looked at it in orthogonal from one direction. And when I looked around from other directions I switched to perspective.

@fracteed
Copy link

fracteed commented Nov 19, 2019

@clayjohn yes it shows up in perspective as well.

@clayjohn clayjohn force-pushed the Fix_environment_mapping_issues branch from 2f57bc4 to cd40154 Compare November 20, 2019 06:31
@clayjohn
Copy link
Member Author

@fracteed @reduz The bright line was my mistake. It is fixed now. :)

I was accidentally allowing the irradiance texture to repeat when it should have been set to clamp_to_edge.

@akien-mga
Copy link
Member

As @reduz seems overall positive about the change (but he's in holidays so I don't expect a thorough review), let's merge and get wider testing in 3.2 beta 2.

@akien-mga akien-mga merged commit 3be6e76 into godotengine:master Nov 20, 2019
@akien-mga
Copy link
Member

Thanks a ton!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants