-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix Metal shader errors with MaterialX 1.39 #3519
base: dev
Are you sure you want to change the base?
Conversation
Here is an example USD file, producing the following errors:
I'm a bit confused about how this could have been working at all, as far as I can tell any scene using MaterialX has this problem. |
CC @ld-kerley, since I guess you might want to be aware of this. |
* mx_microfacet.glsl uses functions from mx_math.metal, so the latter must be included first. * Defining atan(y, x) for GLSL compatibility conflicts with mx_math.metal using ::atan(y_over_x). Work around this by tweaking the definition. * textureQueryLevels(u_envRadiance) does not take into account that this texture is wrapped in a MetalTexture class. Signed-off-by: Brecht Van Lommel <[email protected]>
I've asked Lee to take a look, but in the meantime these look good to me. Thanks for catching those, Brecht. Do you know if these work backwards against 1.38 as well? Or do we need to guard accordingly? @tcauchois this would likely need to get in for 25.5 if that is when you plan to switch to 1.39 by default. |
I couldn't figure out if there are OpenUSD tests for MaterialX + Metal. So this is based on Blender tests.
|
Awesome, thanks! |
Filed as internal issue #USD-10652 (This is an automated message. See here for more information.) |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
Firstly, I can confirm this PR appears to resolve the issues. I was poking around at this from both the OpenUSD side and the MaterialX side and if we make some small changes on the MaterialX side then this PR can shrink significantly. With the changes here, then this PR only needs to re-order the |
Thanks! I'm happy to simplify or discard this PR if needed. |
Description of Change(s)
mx_microfacet.glsl
uses functions frommx_math.metal
, so the latter must be included first.textureQueryLevels(u_envRadiance)
did not take into account that this texture is wrapped in aMetalTexture
class.atan(y, x)
for GLSL compatibility conflicts withmx_math.metal
using::atan(y_over_x)
. Work around this by tweaking the definition.Fixes Issue(s)
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)