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

Support transmission in translation from Standard Surface to glTF PBR #2027

Conversation

charo-m
Copy link
Contributor

@charo-m charo-m commented Sep 26, 2024

This corrects the translation of transmission parameters from Standard Surface to glTF PBR.
Issue: #1599

Here's the change in a more readable pseudocode:

if (transmission > 0) {
    if (transmission_depth > 0) {
        attenuation_color = transmission_color
        base_color = white
    } else {
       attenuation color = white
       base_color = transmission_color
    }
} else {
    attenuation_color = white (default)
    base_color = no_transmission_base_color (hasCoatColor ? mixedBaseColor : scaledBaseColor)
}

Before ->
BeforeGraph

After ->
AfterGraph

…to glTF PBR

Signed-off-by: Charlotte Manning <charodev@gmail.com>
Copy link

linux-foundation-easycla bot commented Sep 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@jstone-lucasfilm
Copy link
Member

@charo-m This looks really promising, and to resolve the EasyCLA issue, I'd recommend following up through the support link that the Linux Foundation provides:

https://jira.linuxfoundation.org/servicedesk/customer/portal/4

@charo-m
Copy link
Contributor Author

charo-m commented Sep 26, 2024

thanks @jstone-lucasfilm ! I had a mismatching email address, just managed to get my CLA signed.

@jstone-lucasfilm
Copy link
Member

@charo-m Excellent! Sometimes the LFX needs to bump the EasyCLA check so that it updates to your new email address, so feel free to check with them if it doesn't automatically turn from red to green.

@kwokcb
Copy link
Contributor

kwokcb commented Sep 27, 2024

As a suggestion, it might might be better test to have the graph with the translation node in it to be able to do visual compares as part of the render test suite. Perhaps it be added to the test suite in a new folder resources/Materials/TestSuite/pbrlib/translation. ( @jstone-lucasfilm, I don't see any other translation examples anywhere else ? Others could be added in as desired).

@kwokcb
Copy link
Contributor

kwokcb commented Sep 27, 2024

Add @emackey here to double check the logic.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks @charo-m, and I had just two recommendations for improvements.

source/MaterialXTest/MaterialXGenShader/GenShader.cpp Outdated Show resolved Hide resolved
@jstone-lucasfilm jstone-lucasfilm changed the title correct translation of transmission parameters from Standard Surface to glTF PBR Support transmission in translation from Standard Surface to glTF PBR Sep 28, 2024
…ed mtl to render testsuite

Signed-off-by: Charlotte Manning <charodev@gmail.com>
@charo-m charo-m force-pushed the support_transmission_in_SS_to_glTF_translation_1599 branch from 6d7ddd2 to 3b9f582 Compare September 28, 2024 17:59
@jstone-lucasfilm
Copy link
Member

Thanks for these updates, @charo-m!

For now, I think it's fine to omit the new standard_surface_to_gltf_pbr_glass.mtlx unit test, which is an interesting suggestion for a future improvement to our translation system, but not an update that we need as part of your Dev Days project.

Let's omit this unit test for now, as additional work is needed to integrate shader translation into our test suite.

Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Great work on this improvement, thanks @charo-m!

@jstone-lucasfilm jstone-lucasfilm merged commit 210266b into AcademySoftwareFoundation:main Sep 29, 2024
34 checks passed
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.

3 participants