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

Use CDF for irradiance prefiltering #16010

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Dec 17, 2024

Expanding on the realtime filtering of irradiance, here is the equivalent logic for prefiltering.
If the user specifies generateHarmonics = false, prefilterOnLoad = true and scene.enableIblCdfGenerator() has been called, then an irradianceTexture will be generated using CDF.

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 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 Dec 17, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 2024

@sebavan sebavan requested a review from Popov72 December 17, 2024 18:47
@Popov72
Copy link
Contributor

Popov72 commented Dec 17, 2024

Let us know when you think the PR is ready for review.

@MiiBond
Copy link
Contributor Author

MiiBond commented Dec 17, 2024

I would like a review whenever you're available. The functionality is complete, I think.
Maybe I'll make some visualization tests.

@MiiBond MiiBond force-pushed the mbond/ibl-irradiance-prefilter-using-cdf branch from 1468a13 to 2456de6 Compare December 18, 2024 01:07
@MiiBond
Copy link
Contributor Author

MiiBond commented Dec 18, 2024

I'm getting errors in WebGPU when prefiltering the irradiance. Not sure what's going on yet.
#8R5SSE#481

WebGPU uncaptured error (17): [object GPUValidationError] - Destroyed texture [Texture "BabylonWebGPUDevice5_TextureCube_1024x1024x6_wmips_rgba32float_samples1"] used in a submit.
 - While calling [Queue].Submit([[CommandBuffer from CommandEncoder "upload"], [CommandBuffer from CommandEncoder "render"]])
 - ```

packages/dev/core/src/Materials/Textures/hdrCubeTexture.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Materials/Textures/hdrCubeTexture.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Materials/Textures/hdrCubeTexture.ts Outdated Show resolved Hide resolved
@@ -162,6 +166,10 @@ export class IblCdfGenerator {
if (this._iblSource!.isCube) {
size.width *= 4;
size.height *= 2;
// Force the resolution to be a power of 2 because we rely on the
// auto-mipmap generation for the scaled luminance texture.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this comment, we can generate mipmaps for textures that are not powers of 2?

environmentIrradiance = vEnvironmentIrradiance;
#else
#if defined(USESPHERICALFROMREFLECTIONMAP) || defined(USEIRRADIANCEMAP)
#if !defined(NORMAL) || !defined(USESPHERICALINVERTEX)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to do:

#if (defined(USESPHERICALFROMREFLECTIONMAP) && (!defined(NORMAL) || !defined(USESPHERICALINVERTEX))) || defined(USEIRRADIANCEMAP)

@Popov72
Copy link
Contributor

Popov72 commented Dec 18, 2024

I'm getting errors in WebGPU when prefiltering the irradiance. Not sure what's going on yet. #8R5SSE#481

WebGPU uncaptured error (17): [object GPUValidationError] - Destroyed texture [Texture "BabylonWebGPUDevice5_TextureCube_1024x1024x6_wmips_rgba32float_samples1"] used in a submit.
 - While calling [Queue].Submit([[CommandBuffer from CommandEncoder "upload"], [CommandBuffer from CommandEncoder "render"]])
 - ```

This error means that a texture has been destroyed before a render command/pass using that texture has been completed. The texture is an RGBA32 float 1024x1024 cube texture, with mipmaps, if that helps you find which texture it is.

Your PG is quite complicated because it uses ibl shadows, so it's not easy to find out where the problem comes from... It would help if you could make a simplified reproduction.

You can help debug such problems by providing debugging labels to the textures you create through calls to Engine.createRenderTargetCubeTexture (for example): this label is one of the options passed to the function. This label will be used in the error message, helping to find the culprit texture.

@MiiBond
Copy link
Contributor Author

MiiBond commented Dec 18, 2024

Okay, I've pushed some changes. Notably, I'm creating a new CdfGenerator inside HdrIrradianceFiltering so that we can set the IBL without messing up the one that the user has set on the scene.cdfGenerator.

The WebGPU error seems to be about the loaded, unfiltered cubemap. It happens only when I'm trying to prefilter both irradiance using the CDF maps and radiance. My interpretation of this is that filtering irradiance with the CDF maps causes this texture to be deleted before the radiance filtering runs. Does that sound right?
I'm not sure how or why this would happen. I can filter both without the CDF maps so it certainly seems to be related to something the CDFGenerator is doing.
This playground have shadows disabled and I defined some booleans to let you easily enable and disable irradiance and radiance filtering.
#8R5SSE#487

@Popov72
Copy link
Contributor

Popov72 commented Dec 20, 2024

This playground have shadows disabled and I defined some booleans to let you easily enable and disable irradiance and radiance filtering. #8R5SSE#487

Should this PG crash? It does work for me, with prefilterIrradiance=prefilterRadiance=true. I reloaded it / started it a few times and could not make it crash (Windows 11, Chrome latest version).

@MiiBond
Copy link
Contributor Author

MiiBond commented Dec 20, 2024

No, it shouldn't crash. I used to get the WebGPU errors that I mentioned but, after refactoring the promise logic yesterday, that's gone away.
Everything should be working correctly now.

@sebavan
Copy link
Member

sebavan commented Dec 30, 2024

@Popov72 let me know when you are all good on this one and I ll do a final pass

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.

4 participants