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

New Rough Radiance Approximation #16063

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

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Jan 14, 2025

The way prefiltered radiance is applied in rasterizers tends not to match the raytraced ground truth very well towards the rough end of the spectrum. I've found that mixing with the irradiance gives a much better result as you can see in the images below.

Here's a playground: http://localhost:1338/#RNO672#1

Raytraced:
Raytraced Roughness

Babylon:
Babylon Roughness

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 14, 2025

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.

@@ -1255,6 +1255,13 @@ export class PBRMaterialPropertyGridComponent extends React.Component<IPBRMateri
propertyName="useHorizonOcclusion"
onPropertyChangedObservable={this.props.onPropertyChangedObservable}
/>
<CheckBoxLineComponent
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Triggering this doesn't change the shader right now. I think I'm missing something to make this property change force the shader to recompile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just replace:

                    if (this._useAlternateRoughRadiance) {
                        defines.ALTERNATE_ROUGH_RADIANCE = this._useAlternateRoughRadiance;
                    }

with

                    defines.ALTERNATE_ROUGH_RADIANCE = this._useAlternateRoughRadiance;

in pbrBaseMaterial.ts.

Copy link
Member

Choose a reason for hiding this comment

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

Is this ok now @MiiBond ?

@MiiBond MiiBond marked this pull request as draft January 14, 2025 00:47
@bjsplat
Copy link
Collaborator

bjsplat commented Jan 14, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 14, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 14, 2025

@MiiBond MiiBond force-pushed the mbond/rough-radiance-improvement branch from 9c5117a to c0bb1e5 Compare January 14, 2025 17:25
@MiiBond MiiBond marked this pull request as ready for review January 14, 2025 17:25
@Popov72
Copy link
Contributor

Popov72 commented Jan 15, 2025

Hey @MiiBond!

Is there a physical argument for lerping radiance to irradiance with higher roughness values, or is it simply because the results look better? Also, have you been able to test with larger scenes / other HDR files to compare with the existing code path?

@MiiBond
Copy link
Contributor Author

MiiBond commented Jan 15, 2025

Is there a physical argument for lerping radiance to irradiance with higher roughness values, or is it simply because the results look better?

So, the short answer is that it's not physically correct to essentially substitute diffuse reflection for specular at roughness levels near 1.0 (they aren't the same thing). However, the reason that it looks more plausible in cases like what I've shown above is that the importance sampling for radiance at roughness near 1.0 really starts to do a very poor job when an IBL has small, strong light sources. Basically, the importance sampling is centered around the reflection direction but doesn't take into account the intensity of the IBL in a given direction. So, on the rough end, you end up with a blurred reflection without the intensity of strong light sources taken into account. This is why we lose the terminators for the strong lights.

We might be able to improve the filtering of radiance at the rough end using the CDF maps but I'm not sure how exactly to do that and I haven't had time to explore it.

Also, have you been able to test with larger scenes / other HDR files to compare with the existing code path?

Yes, I've been using this approximation in Substance Stager for a while now. I can post some more comparisons in a minute.

@MiiBond
Copy link
Contributor Author

MiiBond commented Jan 15, 2025

These include both the improved irradiance sampling (using CDF) and the new roughness approximation:
eclair_bonifacio
old_bonifacio
new_bonifacio

@MiiBond
Copy link
Contributor Author

MiiBond commented Jan 15, 2025

eclair_studio
old_studio
new_studio

@Popov72
Copy link
Contributor

Popov72 commented Jan 16, 2025

Thanks a lot for the explanation and the screenshots!

I will review the PR asap.

@sebavan
Copy link
Member

sebavan commented Jan 16, 2025

Thanks a ton for the explanation !!!

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

Only a few comments!

Also, you should update the visu test and remove the UI, as it takes 1/4 of the picture.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Can we move the new setting in PBRBRDFConfiguration.ts ? as it is really advanced ? Also, should we turn it on by default ? I quite like the visual improvment. We could revert the value to false if we consider it lowering the quality in some given cases ? And finally, is it also valid when relying on an irradiance texture instead of SH as it could have more dynamicity ?

@MiiBond MiiBond force-pushed the mbond/rough-radiance-improvement branch from 793bdad to afeb399 Compare January 18, 2025 06:48
@MiiBond
Copy link
Contributor Author

MiiBond commented Jan 21, 2025

Can we move the new setting in PBRBRDFConfiguration.ts ?

Yes, I really like this idea. It's cleaned up the added code a fair bit and I think it makes sense to enable this by default.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 21, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 21, 2025

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

LGTM

@sebavan sebavan enabled auto-merge (squash) January 21, 2025 20:28
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.

5 participants