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

Shader Translation: Support transmission in Standard Surface to glTF PBR #1599

Closed
kwokcb opened this issue Nov 15, 2023 · 9 comments
Closed

Comments

@kwokcb
Copy link
Contributor

kwokcb commented Nov 15, 2023

Issue

Unlike standard surface and openpbr, base color on gltf pbr affects transmission. This seems
to be the way it's supposed to work but is inconsistent with the other shading models.

For example if you take the glass example which has a base value if 0, this renders as a "clear" material using standard surface and openpbr, but renders opaque black with gltf pbr.

Test

This is the glTF after translation, but can just create a material and set baseColorFactor to 0 and transmissionFactor to non-0.

"materials": [
    {
      "extensions": {
        "KHR_materials_transmission": {
          "transmissionFactor": 1
        },
        "KHR_materials_ior": {
          "ior": 1.5
        }
      },
      "pbrMetallicRoughness": {
        "baseColorFactor": [
          0,
          0,
          0,
          1
        ],
        "metallicFactor": 0,
        "roughnessFactor": 0
      },
      "emissiveFactor": [
        0,
        0,
        0
      ],
      "doubleSided": true,
      "name": "SR_glass"
    }
  ],

Result in model-viewer:
image

Result in MaterialX Graph Editor:
image

Workaround

Modify the base color to be white.

CC'ing @proog128 , @emackey as a possible item to check for future OpenPBR compatibility.

Repro Steps:

  1. Load the glass material into MaterialXVew from the installed resources/Materials/Examples/StandardSurface folder
  2. Open up the advanced settings and set the target shader model to be glt_pbr:
    image
  3. Close the dialog and press 't' (to translate) which will save a new file to disk in the same folder.
  4. Load in the new file. See if it's black :).
@emackey
Copy link

emackey commented Nov 15, 2023

Yes, this was a design decision for glTF transmission: Base color is also the transmission color. When transmission is 1.0 you should use other material systems' transmission color as the glTF base color (default is white typically).

@kwokcb
Copy link
Contributor Author

kwokcb commented Nov 15, 2023

Thanks @emackey,

Then I think the remaining issue lies with the std_surface -> gltf_pbr translation graph that this should be taken into account. Also when a OpenPBR -> gltf_pbr translation graph is created it should use the same logic.

Leaving this issue open for that.

@emackey
Copy link

emackey commented Nov 15, 2023

So it sounds like this is specifically a bug in standard_surface_to_gltf_pbr.mtlx.

Right now SS transmission_color is hooked up exclusively to glTF's volumetric attenuationColor. But reading the SS spec, this should only be true when SS transmission_depth is non-zero. When SS transmission_depth is zero, transmission_color should displace SS base_color, becoming glTF's base color.

And that means yes, when SS transmission is in effect and transmission_depth is nonzero, SS transmission_color becomes glTF attenuationColor, and glTF base color is only ever white in that case.

@jstone-lucasfilm jstone-lucasfilm changed the title glTF PBR base color adversely affects transmission Transmission support in Standard Surface to glTF PBR translation Jan 3, 2024
@jstone-lucasfilm jstone-lucasfilm changed the title Transmission support in Standard Surface to glTF PBR translation Shader Translation: Support transmission in Standard Surface to glTF PBR Sep 23, 2024
@charo-m
Copy link
Contributor

charo-m commented Sep 24, 2024

can I pick this issue for ASF DevDays 2024 ?

@jstone-lucasfilm
Copy link
Member

Done, and thanks @charo-m!

@Avgilles
Copy link

Avgilles commented Sep 24, 2024

can I pick this issue for ASF DevDays 2024 ?

ohh to late for me, good luck !

@kwokcb
Copy link
Contributor Author

kwokcb commented Sep 24, 2024

I've added some repro steps which I hope will help.

For faster setup, it is feasible to download the current release of MaterialX, and modify the file in libraries/bxdf/translation to avoid having to build everything. You need to restart MaterialXView after modification.

Of course to check in you'd want to run against a proper development build.

@charo-m
Copy link
Contributor

charo-m commented Sep 26, 2024

For the repro steps, I wasn't able to get MaterialXView working (I'm on an Intel macbook, and couldn't use the prebuilt binary as it was arm64 only - and the one that I built gets into a bad state when I try to load a different material). So instead, I added some lines to write out to disk the translated material (removed before submitting PR) to the unit test which translates SS glass to glTF PBR, then inspected that in MaterialXGraphEditor which works fine.

PR: #2027

jstone-lucasfilm pushed a commit that referenced this issue Sep 29, 2024
…#2027)

This corrects the translation of transmission parameters from Standard Surface to glTF PBR.
Issue: #1599
@jstone-lucasfilm
Copy link
Member

Thanks to @kwokcb for this original report, and to @charo-m for addressing this in #2027!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants