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

Update subsurface color types #220

Merged

Conversation

jstone-lucasfilm
Copy link
Member

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.

  • Change subsurface_radius_scale from a vector3 to a color3 in the specification, aligning with the MaterialX implementation of OpenPBR.
  • Change the radius input of subsurface_bsdf from a vector3 to a color3 in the MaterialX implementation, aligning with the current definition of the subsurface_bsdf node in MaterialX 1.39.

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.

- Change `subsurface_radius_scale` from a `vector3` to a `color3` in the specification, aligning with the MaterialX implementation of OpenPBR.
- Change the `radius` input of `subsurface_bsdf` from a `vector3` to a `color3` in the MaterialX implementation, aligning with the current definition of the `subsurface_bsdf` node in MaterialX 1.39.
@portsmouth
Copy link
Contributor

This accords with what I proposed in #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.).

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.

I haven't followed the discussion on this topic, but the proposed change looks good to me.

@portsmouth
Copy link
Contributor

portsmouth commented Jun 27, 2024

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
Copy link
Member Author

@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

@jstone-lucasfilm jstone-lucasfilm merged commit 5bcea36 into AcademySoftwareFoundation:dev_1.1 Jun 27, 2024
1 check passed
@portsmouth
Copy link
Contributor

@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.

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.

None yet

3 participants