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

Separate CDF Renderer from IBL Shadows and use for realtime filtering #15878

Merged

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Nov 22, 2024

I'd like to improve the realtime filtering and prefiltering of IBL's using the CDF maps that I'm using for IBL shadows. To do this, I'm separating the CDF renderer out from the IBL Shadows Pipeline.
Then, as a first step, I'm using these maps to improve the realtime filtering of irradiance.

As a test, here's an extreme IBL with a single sunlight rendered in a raytracer.
extreme_light_raytraced

Here's how it looks with spherical harmonics:
extreme_light_sh

Here's the old real-time filtering:
extreme_light_rt_old

And the new real-time filtering:
extreme_light_rt_new

I still may not have the math quite right, especially for the weighting, but the early results are very promising. I haven't changed the number of samples yet but we should be able to lower them from what they currently are.

TODO:

  1. Using importance sampling with the CDF maps should be an option for prefiltering IBL's as well.
  2. Should using these CDF maps be a separate option and not directly replace the existing filtering? The main downside is that it requires the CDF maps to be generated first. After that, the speed/quality should always be a win.
  3. I haven't modified the WebGPU irradiance function yet because it doesn't seem to be used. What I mean is that, when I modify the irradiance() function in the glsl shaders, it seems to affect the WebGPU rendering as well. I really don't understand why.
  4. I haven't tested anything other than diffuse irradiance yet.

@MiiBond MiiBond marked this pull request as draft November 22, 2024 18:08
@MiiBond MiiBond force-pushed the mbond/ibl-importance-sampling-irradiance branch from 8f33457 to ea008bf Compare November 22, 2024 18:13
@bjsplat
Copy link
Collaborator

bjsplat commented Nov 22, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@Popov72
Copy link
Contributor

Popov72 commented Nov 26, 2024

If we can improve performance AND the quality of real-time filtering, I'm all for it!

However, I think it should be a new path that we can enable/disable, because replacing what we currently have would be a breaking change, and I know that for the commerce sandbox, we should be sure that the new code doesn't break anything => or, at least, we should discuss this internally and decide if we can allow a breaking change here.

I'm also wondering if we could pre-calculate the two CDF textures and integrate them into an .env file? I think we have a version number in the .env files, so it would be easy to support a new format, where these two textures would have been precalculated. This way, the new code path will be automatically selected at runtime when we load these .env files, and we won't have to suffer the generation time.

cc @sebavan

One thing about your PR: is it possible to undo all the formatting changes in the shader code? A lot of the changes only concern formatting (undoing them would simplify reviewing), and I also find the new formatting difficult to read, with types appearing on the next line...

@MiiBond MiiBond force-pushed the mbond/ibl-importance-sampling-irradiance branch from ea008bf to 534693e Compare November 26, 2024 17:00
@bjsplat
Copy link
Collaborator

bjsplat commented Nov 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@MiiBond
Copy link
Contributor Author

MiiBond commented Nov 26, 2024

Agreed. I've made it its own code path now. Set scene.useEnvironmentCDFMaps = true

Yes, the CDF maps could absolutely be part of the .env file, along with irradiance. If the prefiltered maps are already created, however, the CDF maps aren't needed anymore except for other features like IBL shadowing.

About the shader formatting, VSCode keeps doing that and I need to remember to disable "format on save" while working on shaders. Do you guys use different settings to avoid this?

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@MiiBond
Copy link
Contributor Author

MiiBond commented Nov 26, 2024

I'm still having problems getting the WebGPU version to actually load the WGSL shaders. If I purposely type in bad code in packages\dev\core\src\ShadersWGSL\pbr.fragment.fx, it will still run fine. Putting the bad code in the glsl version results in compile errors.
Is there something special that needs to be done to make Babylon load the WGSL shaders?

@MiiBond MiiBond force-pushed the mbond/ibl-importance-sampling-irradiance branch from 0a7f97a to 981b168 Compare November 26, 2024 19:55
@bjsplat
Copy link
Collaborator

bjsplat commented Nov 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

2 similar comments
@bjsplat
Copy link
Collaborator

bjsplat commented Nov 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 27, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@MiiBond MiiBond marked this pull request as ready for review November 27, 2024 18:07
@bjsplat
Copy link
Collaborator

bjsplat commented Nov 27, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 27, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 27, 2024

Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

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

Fantastic work! Thanks a ton!

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 27, 2024

@MiiBond MiiBond force-pushed the mbond/ibl-importance-sampling-irradiance branch from 237ded8 to 8e58ce3 Compare December 2, 2024 23:57
@sebavan sebavan merged commit 5727959 into BabylonJS:master Dec 3, 2024
13 checks passed
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.

6 participants