Skip to content

Diffuse Roughness support #16253

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

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

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Mar 5, 2025

This PR replaces #16183

This PR implements the base_diffuse_roughness parameter from the OpenPBR specification.

The diffuse roughness is implemented for analytic lights, realtime-filtered IBL, prefiltered IBL and spherical harmonics.
I've added a flag to a material to choose between Lambert, Burley and the new Energy Conserving Oren-Nayar (EON) model used by OpenPBR. The default is OpenPBR's EON model.

The previous behaviour was to use Burley diffuse for analytical lights and Lambert for IBL. Also, previously, specular roughness was applied to diffuse roughness for analytical lights while IBL didn't use it at all (because it was simply Lambertian). So, the new default slightly changes existing projects that used analytical lights but I question how noticeable that will be.

Analytical Light:
https://playground.babylonjs.com/?snapshot=refs/pull/16253/merge#MXACV7#3
roughnessCompare

Realtime IBL:
https://playground.babylonjs.com/?snapshot=refs/pull/16253/merge#MXACV7#5
image

The diffuse roughness models are heavily dependent on the light direction and view direction and are therefore difficult to handle with a prefiltered IBL. I came up with two methods for approximating roughness with prefiltered IBL's. The first, if we prefiltered using CDF, we generate a dominant light direction to use in the BRDF calculations. It works reasonably well.

Prefiltered IBL with CDF:
https://playground.babylonjs.com/?snapshot=refs/pull/16253/merge#MXACV7#9
image

The second approach, if you don't use CDF, is by approximating roughness by bending the surface normal towards the camera to add some of the retro-reflective behaviour that you get with EON. Because of this, you'll notice that Burley and EON are identical and the shadow terminator appears to move as diffuse roughness increases. In practice, however, this example uses an extreme IBL with a bright sunlight. With other IBL's, the effect tends to be more convincing.

Prefiltered IBL without CDF
https://playground.babylonjs.com/?snapshot=refs/pull/16253/merge#MXACV7#10
image

The default IBL lighting in Sandbox uses spherical harmonics so we need to approximate diffuse roughness with this as well. I'm using the same bent normal technique as with prefiltered IBL without CDF.

Spherical Harmonics IBL
https://playground.babylonjs.com/?snapshot=refs/pull/16253/merge#MXACV7#11
image

@MiiBond MiiBond marked this pull request as draft March 5, 2025 22:03
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 5, 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.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 5, 2025

1 similar comment
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 6, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 6, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 6, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 6, 2025

@MiiBond MiiBond force-pushed the openpbr/diffuse_roughness branch from 3580a82 to 55f34a0 Compare March 7, 2025 04:29
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 7, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 7, 2025

1 similar comment
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 7, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 7, 2025

Copy link
Contributor

@virtualzavie virtualzavie left a comment

Choose a reason for hiding this comment

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

This is only a partial review with a couple of comments.

Copy link
Contributor

@virtualzavie virtualzavie left a comment

Choose a reason for hiding this comment

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

Continued review.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 10, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 10, 2025

1 similar comment
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 10, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 10, 2025

@MiiBond MiiBond force-pushed the openpbr/diffuse_roughness branch from 203a6cf to 5ee9410 Compare March 11, 2025 23:57
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@MiiBond MiiBond marked this pull request as ready for review March 12, 2025 18:25
@MiiBond MiiBond force-pushed the openpbr/diffuse_roughness branch from 6b0f7a4 to 2f1cf0e Compare March 12, 2025 18:37
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@MiiBond MiiBond force-pushed the openpbr/diffuse_roughness branch from 1acd8bf to d4278a2 Compare May 5, 2025 16:10
@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 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.

@MiiBond
Copy link
Contributor Author

MiiBond commented May 5, 2025

@Popov72 @sebavan
The linter in the CI is giving me this error but I don't get it locally:

error @typescript-eslint/naming-convention: Interface name `GLTFLoaderExtensionOptions` must have one of the following prefixes: I

I'll make this change but I wanted to call this out.

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 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.

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@sebavan
Copy link
Member

sebavan commented May 5, 2025

@Popov72 @sebavan The linter in the CI is giving me this error but I don't get it locally:

error @typescript-eslint/naming-convention: Interface name `GLTFLoaderExtensionOptions` must have one of the following prefixes: I

I'll make this change but I wanted to call this out.

I do not think we should change it

cc @RaananW

@RaananW
Copy link
Member

RaananW commented May 5, 2025

@Popov72 @sebavan The linter in the CI is giving me this error but I don't get it locally:

error @typescript-eslint/naming-convention: Interface name `GLTFLoaderExtensionOptions` must have one of the following prefixes: I

I'll make this change but I wanted to call this out.

No need to replace the entire interface, you will need to add eslint-disable-next-line @typescript-eslint/naming-convention before the line using it (where you used it).

Fetch from master and run npm install to be sure you are up to date with the newest eslint rules

@deltakosh deltakosh enabled auto-merge (squash) May 5, 2025 18:21
auto-merge was automatically disabled May 5, 2025 18:23

Head branch was pushed to by a user without write access

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented May 5, 2025

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.

8 participants