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

KHR_materials_dispersion #2340

Merged
merged 27 commits into from
Feb 5, 2024
Merged

Conversation

bhouston
Copy link
Contributor

A first draft of something we are supporting.

@emackey
Copy link
Member

emackey commented Oct 23, 2023

Hi Ben, this proposal sparked a long conversation in the PBR TSG today. General comments and feedback:

  • This looks like a great candidate, and we think it should have the KHR_ prefix.
  • The Abbe number appears in Enterprise PBR (section 1.9, Dispersion), and in OpenPBR (section 3.3.3), indicating compatibility with other PBR systems, which we like.
  • The draft should be more clear about its dependence on KHR_materials_volume and its effect on transmissive volumetric materials such as gemstones.
  • It would be good to mention (in a non-normative "Implementation Notes" section) that there's a straightforward realtime implementation, for systems that are already processing volumetric IOR, this just adjusts the IOR per color channel.
  • The default value should make no change to the core material. In other words, adding an empty JSON object "KHR_materials_dispersion" : { } should not change the material at all. Perhaps the number 0 indicates no dispersion, and it becomes the default. This is in line with all of our other PBR extensions: The PBR exists outside of the extension (in Blender and in ThreeJS etc), and so the glTF JSON structure has no influence on the material by itself.
  • It would be nice to name references. Maybe we can name the references that OpenPBR names?

Overall this looks like a great start!

@bhouston
Copy link
Contributor Author

Hi @emackey! Thanks for the quick feedback!

This looks like a great candidate, and we think it should have the KHR_ prefix.

Done!

The Abbe number appears in Enterprise PBR (section 1.9, Dispersion), and in OpenPBR (section 3.3.3), indicating compatibility with other PBR systems, which we like.

I've referenced both of those now in the spec and also was clear on their parameter naming. OpenPBR represents dispersion via two separate numbers in order to make it easier for artistic control (Abbe number and scale factor), but they still resolve to a single value in the end, thus I suggest we stick with just Abbe number in the glTF material model.

The draft should be more clear about its dependence on KHR_materials_volume and its effect on transmissive volumetric materials such as gemstones.

I've added an image of a gemstone as well as referencing KHR_material_volume as the extension this is enhancing.

It would be good to mention (in a non-normative "Implementation Notes" section) that there's a straightforward realtime implementation, for systems that are already processing volumetric IOR, this just adjusts the IOR per color channel.

Done.

The default value should make no change to the core material. In other words, adding an empty JSON object "KHR_materials_dispersion" : { } should not change the material at all. Perhaps the number 0 indicates no dispersion, and it becomes the default. This is in line with all of our other PBR extensions: The PBR exists outside of the extension (in Blender and in ThreeJS etc), and so the glTF JSON structure has no influence on the material by itself.

Done! Good idea.

It would be nice to name references. Maybe we can name the references that OpenPBR names?

I've referenced OpenPBR and its references!

Overall this looks like a great start!

Nice! Thank you!

@bhouston bhouston changed the title EXT_materials_dispersion KHR_materials_dispersion Oct 23, 2023
@MiiBond
Copy link
Contributor

MiiBond commented Nov 2, 2023

Very cool! Thanks for defining this.
One thing to note is that the dispersion parameter in OpenPBR, Autodesk Standard Surface and Adobe Standard Material are all varying (i.e. texturable). I know that complicates things but, for the sake of future-proofing, maybe it's worth considering?
Another thing to consider is that the Abbe number doesn't scale in a perceptually-linear way (in terms of the visible amount of dispersion). But the reciprocal of the Abbe number is proportional to the change in the IOR across the spectrum and the angle of refraction varies approximately linearly with the index at these scales. Also, higher Abbe numbers result in less dispersion which might be somewhat counter-intuitive.
I'm not suggesting that being intuitive to the artist is really that important for glTF since most people aren't modifying the glTF parameters directly so take this for what it's worth.
In ASM, we define the dispersion parameter as 20/V so that a value of 1.0 is equivalent to V=20, which is about the lowest Adde number for real materials. Values over 1.0 are still valid for artists that want to exaggerate the effect. Decreasing values lower the amount of dispersion down to 0.0.
Aside from being more intuitive for artists, this mapping also has the added benefit of being more easily defined with a texture.

@bhouston
Copy link
Contributor Author

bhouston commented Nov 3, 2023

@MiiBond I can see us using the 20/V parametrization, it makes sense. I can make that change. I had noticed that in the paper.

@MiiBond does OpenPBR/ASS/ASM allow for ior to vary around a material and do they allow for absorption to vary across the surface of a material? glTF has not adopted these textures in part because it the surface parameterization is hard to extrapolate through the volume correctly and clearly. I know one could technically use a generalized Barycentric coordinates (e.g. Warren coordinates.). I figured we were staying away from this use case. If you want to make the case that we should vary Dispersion on the surface of the material via a map unlike Absorption and IOR, please do.

@emackey
Copy link
Member

emackey commented Nov 6, 2023

@MiiBond to confirm, if we invert the value (20/V), does that make it correspond more linearly to the visual effect of dispersion? For example, if the user supplied a value of 0.1 it shows visually very small amounts of dispersion, but a value of 0.9 or 1.0 shows a larger visual separation of colors? If so then yes, @bhouston if you don't mind adjusting this draft to use this parameterization, that would be the PBR TSG's preference. In the JSON schema, The default and minimum could be 0.0 (with 20 / 0.0 being the "infinite" Abbe number), and maximum could be omitted since there's no hard stop at 20 / 1.

On the PBR call today we also talked about the possibility of texturing this parameter, but decided against, at least for now. We previously ratified a non-texturable IOR setting on the idea that it's volumetric and not a surface-texture property, and it wouldn't make sense to reverse that decision here for dispersion.

@emackey
Copy link
Member

emackey commented Nov 6, 2023

Sample model time. This one presumes the 20/V change, where dispersion ranges from zero to one.

DispersionIorTest.zip

@MiiBond
Copy link
Contributor

MiiBond commented Nov 7, 2023

Sorry, I've been incapacitated by the flu since Thursday.
@emackey, yes, that's correct. 0.0 is no-effect and 1.0 would be the maximum, real-world amount of dispersion.
@bhouston, IOR and dispersion are varying in OpenPBR but I believe that it's still on the TODO list to define what that means for volumetric properties. They're also varying in Standard Surface, I think, so maybe that documentation has some insights. In ASM, we made both IOR and dispersion uniform to avoid this issue.

As you guys said, though, it probably doesn't make sense to change things in glTF at this point.

@MiiBond
Copy link
Contributor

MiiBond commented Dec 8, 2023

My Babylon.js implementation of this extension has been merged.
BabylonJS/Babylon.js#14501

@emackey
Copy link
Member

emackey commented Dec 11, 2023

New sample model available! I've switched from gems to prisms, and updated the background cloth. Also, the range of values goes up to 5.0 in the model now.

Here's the GLB:

DispersionIorTest_v2.zip

And some screenshots, rendered by https://sandbox.babylonjs.com/ using Mike's newly-merged PR. I'm using the "Studio" IBL with default settings here.

If someone could path-trace this model for comparison, that would be much appreciated!

Dispersion_screenshot1

Dispersion_screenshot2

This didn't appear in VSCode's markdown preview, but we'll test GitHub.
@emackey emackey merged commit 4b870a0 into KhronosGroup:main Feb 5, 2024
1 check passed
@BeRo1985
Copy link

BeRo1985 commented Feb 9, 2024

I've implemented support of KHR_material_dispersion also in my PasVulkan project now:

Screenshot_20240210_000201

@donmccurdy
Copy link
Contributor

Adding read/write/edit support in glTF Transform as well:

@zeux
Copy link
Contributor

zeux commented Feb 16, 2024

gltfpack support:

abbaswasim pushed a commit to abbaswasim/glTF that referenced this pull request Sep 7, 2024
* draft of EXT_materials_dispersion

* incorporating feedback

* use inline latex

* fix typo

* specify defaults and range in extension.

* make clear that it is an enhancement on top of KHR_materials_dispersion

* make clear the names used in other material models.

* add references from OpenPBR.

* change to storing 20/V

* improve equations

* wording cleanup.

* split equations apart.

* test

* improve the json to match dispersion spec

* address @lexaknyazev's feedback

* remove mention of texture

* Some updates based on review (GitHub and PBR TSG) feedback.

* Update visible light wording

Co-authored-by: Bastian Sdorra <[email protected]>

* Change $V$ to $V_d$ per review, add note about internal reflections

* Update to reference Cauchy's equation

Co-authored-by: Bastian Sdorra <[email protected]>

* Fix blank line

* Add note box per review.

This didn't appear in VSCode's markdown preview, but we'll test GitHub.

* Lambda fix per review

* Tweak

* Status changed from "Draft" to "RC" in an official PBR TSG vote today.

* Contributor list update

* Clarify Fraunhofer spectral lines and define lambda symbols

Co-authored-by: Bastian Sdorra <[email protected]>

---------

Co-authored-by: Ed Mackey <[email protected]>
Co-authored-by: Bastian Sdorra <[email protected]>
Co-authored-by: Alexey Knyazev <[email protected]>
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.

9 participants