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

Specular Energy Conservation fix #16476

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

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Apr 10, 2025

Okay, I was starting to work on supporting the OpenPBR specular model (coloured specular right out to F90 and then, after that, texturable IOR and the F82 model for conductors). However, right away, I noticed that the current diffuse/specular logic isn't correct for dielectrics. We multiply the diffuse part by F0, leaving too much energy at glancing angles.

Here's a furnace test where metallic increases from left to right and metallicF0Factor increases from bottom to top.
image

This PR eliminates this completely.
image

I've only touched the metallic workflow (I think) as this is all that's applicable to OpenPBR compatibility (and that's my current focus). Before I continue further, I'd like to hear some feedback as I've made some other changes.

  1. In the metallic workflow, reflectivityColor is used to store metallic, roughness and ior while metallicReflectanceFactors is used to store the reflectivity colour. I basically just swapped these as it makes a lot for sense to me. Please let me know if this is a problem.
  2. I'm passing the full reflectivity colour into the shader now rather than pre-multiplying by F0. This gives us more control in the shader and will be necessary later for coloured F90 and texturable IOR.
  3. In order for blending between dielectric and metal lobes to maintain energy conservation, it needs to be done after the lobes are calculated rather than as we're calculating each property. I'm doing this in pbrBlockFinalLitComponents.
    https://playground.babylonjs.com/?snapshot=refs/pull/16476/merge#KQYNYS#10

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 10, 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 Apr 10, 2025

Outside of a furnace test, the change can be pretty subtle but you can see the difference around edges:
Before:
image
After:
image

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 10, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 10, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16476/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 10, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16476/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16476/merge/?snapshot=refs/pull/16476/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 11, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 11, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 11, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16476/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 11, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16476/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16476/merge/?snapshot=refs/pull/16476/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 11, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 11, 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.

2 participants