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_subsurface #1928

Closed

Conversation

bsdorra
Copy link
Contributor

@bsdorra bsdorra commented Jan 11, 2021

The subsurface scattering part of KHR_materials_volume extracted to its own extension. Please note, this extension drops the parameterization via subsurfaceColor in favor of a parameterization which is semantically more detached from the absorption definition in KHR_materials_volume. New parameters are scatterColor and scatterDistance, from which we can directly derive the scattering coefficient as defined in the readme.

@Reon90
Copy link
Contributor

Reon90 commented Jun 3, 2021

Is it possible to add this one to Extension List in main repo?

@donmccurdy
Copy link
Contributor

@Reon90 — if you mean this list in the extension registry, note that it has several sections, including Khronos extensions that are (a) already ratified, or (b) in progress and actively moving toward ratification. At the moment, this PR is just a PR and nothing more.

I'm guessing that your comment is meant as a +1 for supporting SSS in glTF in the future? If so, noted! If not, please let us know if you have any other preferences or concerns here. 🙂

@donmccurdy
Copy link
Contributor

(Related: I think something like #1970 may help to communicate the status of PRs more clearly)

@hypnosnhendricks
Copy link

Looking at the Khronos Sample Viewer implementation for transmission and volume, I was wondering what the shader code would look like just for the SSS portion of Volume. Our team wants to implement SSS and I wanted to setup support for this extension so they could see the results, but I'm not sure how to detangle the SSS code from the volume code in 'ibl.glsl' located here: https://github.com/KhronosGroup/glTF-Sample-Viewer/blob/master/source/Renderer/shaders/ibl.glsl

Could someone help? For these type of extensions have the sample shader source is really, really useful for us devs.

"properties": {
"scatterDistance": {
"type": "number",
"minimum": 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

scatterDistance should declare the minimum as an exclusiveMinimum. See the schema for KHR_materials_volume

Replace pseudo equations with latex math
Wording
@donmccurdy
Copy link
Contributor

Apologies in advance if this goes into bike-shedding territory, but to me the KHR_materials_sss name feels inconsistent with other extensions... perhaps KHR_materials_subsurface? Blender's UI just uses the term "Subsurface" for example, omitting the "scattering" term outside of documentation.

@bsdorra
Copy link
Contributor Author

bsdorra commented Jun 27, 2022

Apologies in advance if this goes into bike-shedding territory, but to me the KHR_materials_sss name feels inconsistent with other extensions... perhaps KHR_materials_subsurface? Blender's UI just uses the term "Subsurface" for example, omitting the "scattering" term outside of documentation.

I've no strong opinion here. Agreed that KHR_materials_sss naming feels somewhat inconsistent. On the other hand, the term SSS is almost being used like a name in itself, it's short and certainly triggers the right associations for many people.
Consistency to blender sounds useful. Not sure if I like that idea of dropping the scattering part from the name though, as that's what the extensions technically defines. But KHR_materials_subsurface_scattering is too long. :)

@emackey
Copy link
Member

emackey commented Jun 27, 2022

No strong opinion from me either, but I notice the name "scatter" is already in the parameters (scatterColor, scatterDistance) so maybe it's fine to have the name of the extension say only subsurface. Both names will be seen together in a finished glTF file, so it will be clear this is subsurface scattering.

"KHR_materials_subsurface" : {
    "scatterColor": [ 0.5, 0.0, 0.0 ]
}

@aidinabedi
Copy link
Contributor

aidinabedi commented Oct 4, 2022

Hello all! Just to chip in if I may 😄

In Unreal, we also use the shortcut subsurface to denote our SSS shading model.

Additionally, we also allow the scatterColor and scatterDistance (although we call it "density" and it goes from 0 to 1) to have texture inputs. Would that be feasible to add in the extension? i.e.: scatterColorTexture and scatterDistanceTexture?

@bsdorra
Copy link
Contributor Author

bsdorra commented Oct 6, 2022

@aidinabedi, thanks for the input. We explicitely defined the current extension proposal without texturable parameters. With the new glTF PBR extensions we decided to strictly separate surface from volume effects. Texturing volumetric parameters with a surface texture will change the results of the volume evaluation based on the volume entry point. This may be perfectly fine for a rasterizer that uses a surface-based approximation to volume integration, e.g. diffusion approximation. But for path-tracers with bi-directional integration methods such behavior will destroy reciprocity. So you end up with inconsitent results for different rendering techniques, which is something we were trying to prevent by design.

We recently added a diffuseTransmissionColorFactor to the KHR_materials_diffuse_transmission PR. Is this something that may help with the requirement for a textureable scatterColor?

Not saying that the parameterization is set in stone though, we're still discussing alternatives back and forth. If you have example scenarios and you're allowed to share, we' re more than happy to look at them!

@emackey
Copy link
Member

emackey commented Nov 21, 2024

Further discussion should take place in #2453, not here. Closing.

@emackey emackey closed this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants