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

Improved open_pbr_surface to standard_surface translation #1958

Conversation

mkuo-lucasfilm
Copy link
Contributor

Integrated the logic of coat_darkening from OpenPBR into the calculations of base_color and subsurface_color

Revising base and subsurface color calculations to more closely resemble coat_darkening present in OpenPBR
removing redundant ifgreater node for subsurface color calculation
@mkuo-lucasfilm
Copy link
Contributor Author

Pearl comparisons:

  1. OpenPBR
  2. Standard Surface (original shader translation graph)
  3. Standard Surface (latest shader translation graph)

Generated difference images between the Standard Surface image and the OpenPBR image for both versions. The original average difference was 0.92 (out of 255), and the new average difference is 0.57.
open_pbr_pearl
open_pbr_pearl_ss
open_pbr_pearl_ss_3

@mkuo-lucasfilm
Copy link
Contributor Author

Carpaint comparisons:

  1. OpenPBR
  2. Standard Surface (original shader translation graph)
  3. Standard Surface (latest shader translation graph)

Generated difference images between the Standard Surface image and the OpenPBR image for both versions. The original has an average difference of 1.57 (out of 255), and the new one has an average difference of 1.21.

open_pbr_carpaint
open_pbr_carpaint_ss
open_pbr_carpaint_ss_3

@jstone-lucasfilm jstone-lucasfilm changed the title Improved open_pbr_surface to standard_surface translation Improved open_pbr_surface to standard_surface translation Aug 2, 2024
@portsmouth
Copy link

portsmouth commented Aug 2, 2024

I see, so this bakes in the darkening factor into the base_color (though coat_affect_color should be off I think, for that to work). That will work for an initial translation, though of course if the user decides to remove the coat afterwards (from the Standard Surface), the darkening persists.

The way we translate should depend on what the intended usage of the translated shader is. If it is assumed to be baked and immutable, then this kind of baking of the darkening is fine. But if the user might want to change the parameters after the translation (e.g. remove coat etc.), then it would be better to try to reproduce the structure as well as possible, e.g. keep the base color and just use the coat_affect_color parametrization to approximate the darkening.

I'm not sure what the more appropriate policy would be for these translation graphs.

@jstone-lucasfilm
Copy link
Member

jstone-lucasfilm commented Aug 2, 2024

Thanks for these notes, @portsmouth!

In my own experience, the most important quality of a shader translation graph is its visual parity, since the majority of the artistic work on the material usually occurs before shader translation is applied, and the receiving party is most interested in rendering the original material as accurately as possible.

In situations where additional edits are needed, it's also possible to share the unbaked translated graph, in which the original shading model interface of the material remains available for further editing, though in practice it's more common to bake out textures and constant values in the new shading model.

From this perspective, I would consider @mkuo-lucasfilm's latest work to be a meaningful improvement, since there's a small but measurable improvement to parity for coated materials, and no reduction of parity for other materials.

@jstone-lucasfilm
Copy link
Member

Let's move forward with this refinement to the shader translation graph, and there's still time for additional improvements before the release of MaterialX 1.39.1.

Thanks for your great work on this, @mkuo-lucasfilm!

@jstone-lucasfilm jstone-lucasfilm merged commit 1775a6d into AcademySoftwareFoundation:main Aug 3, 2024
34 checks passed
@mkuo-lucasfilm mkuo-lucasfilm deleted the shader-translation branch August 5, 2024 21:23
JGamache-autodesk added a commit to autodesk-forks/MaterialX that referenced this pull request Nov 5, 2024
…Foundation#2087)

The standard_surface coat parameters are currently affecting the baseColor of UsdPreviewSurface. It should also affect emission color.

Initial translation from `open_pbr_surface` to `standard_surface` (AcademySoftwareFoundation#1949)

- Authored an initial pass at the translation from OpenPBR to standard surface.
- This would help to support OpenPBR in applications using earlier versions of MaterialX, which only have access to Standard Surface, but may want to participate in the OpenPBR ecosystem before they upgrade.

Update `standard_surface` to `open_pbr` translation graph   (AcademySoftwareFoundation#1956)

Replacing coat_affect_roughness with a constant value, since a coating always affects roughness in OpenPBR and is not dependent on the value of coat_darkening.

Improved `open_pbr_surface` to `standard_surface` translation (AcademySoftwareFoundation#1958)

Integrated the logic of coat_darkening from OpenPBR into the calculations of base_color and subsurface_color.

Initial translation from `standard_surface` to `open_pbr_surface` (AcademySoftwareFoundation#1934)

1. Authored an initial pass at the translation graph between standard surface and OpenPBR
2. Added units to standard surface thin film thickness documentation

Update documentation of OpenPBR and Standard Surface translations (AcademySoftwareFoundation#1963)

Clarified documentation where the motivation for the conversion might not be clear - both for the translation graph from OpenPBR to Standard Surface, and the translation graph from Standard Surface to OpenPBR.

Decrement translation graphs minor version (backport)

Consider subsurface color in dielectric albedo
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.

3 participants