AcademySoftwareFoundation / OpenPBR

Specification and reference implementation for the OpenPBR Surface shading model
Apache License 2.0
459 stars 18 forks source link

Update subsurface color types #220

Closed jstone-lucasfilm closed 3 months ago

jstone-lucasfilm commented 3 months ago

This changelist updates the types associated with physical color values for subsurface scattering in OpenPBR, aligning with the conclusions of recent threads on ASWF Slack channels.

portsmouth commented 3 months ago

This accords with what I proposed in https://github.com/AcademySoftwareFoundation/OpenPBR/issues/144.

As we discussed on Slack, there is still some work to do to understand how/whether color management of the subsurface_radius_scale can be made to work better, as due to the non-linearity of the relationship between the radius (which is the RGB mean-free-path) and the observed color, the render appearance can change significantly on changing color space.

But it still makes more sense for this to be a color than a vector3, as it needs some kind of color management (and is picked as a color, driven by RGB texture, etc.).

portsmouth commented 3 months ago

Just to note, we also don't currently make it very clear in the OpenPBR spec what it means that a quantity is a color3 versus a vector3. We should for example probably say explicitly that we assume that the color3 quantities are color managed in the standard way (which will lead to some issues for the subsurface as noted, but on balance it's even worse if not color managing the subsurface radius).

I'm not sure we even have logic for this color management in e.g. Maya, e.g. if an OpenPBR (or Standard Surface) shader is brought in with some asset, do we check the associated color space of the parameters (using what metadata?) and do some transformation in the DCC/renderer to account for it? What if the parameters are linked to textures, is the color space of the texels accounted for? Possibly, but i'm not aware of the details (@AdrienHerubel will be).

jstone-lucasfilm commented 3 months ago

@portsmouth In this situation, we can just fall back on the specified behavior of color spaces in MaterialX, which is inherited by OpenUSD and NanoColor as well:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.Specification.md#color-spaces-and-color-management-systems

portsmouth commented 3 months ago

@jstone-lucasfilm sure, though it deserves at least a mention in the OpenPBR spec, that we expect some such logic to be applied by the host application.