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

Add normal to gltf_pbr translation graph #1234

Conversation

rasmusbonnedal
Copy link
Contributor

No description provided.

@@ -17,6 +17,7 @@
<input name="coat_roughness" type="float" value="0.1" />
<input name="emission" type="float" value="0" />
<input name="emission_color" type="color3" value="1, 1, 1" />
<input name="normal" type="vector3" value="0.5, 0.5, 1.0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

The value seems to come from the Standard Surface to UsdPreviewSurface translation graph, and I don't think it is correct in this context. We should use defaultgeomprop="Nworld" instead.

@pablode
Copy link
Contributor

pablode commented Feb 19, 2023

While we're at it, I think we should add support for the tangent input in a similar way!

@jstone-lucasfilm
Copy link
Member

@rasmusbonnedal This looks like a step in the right direction, but as @pablode notes, we'll need to handle the novel situation that both Standard Surface and glTF PBR take world-space normal inputs rather than tangent-space values.

In addition to the default value change that Pablo suggests above, I believe we'd need to refine the logic that optimizes away inputs at their default values during texture baking, adding handling for the defaultgeomprop case as well:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXRender/TextureBaker.inl#L289

@jstone-lucasfilm
Copy link
Member

@rasmusbonnedal Feel free to continue working on this if you have the bandwidth, but I'll close out the original PR for now, since additional work would be needed to make it fully robust.

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