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

Unhide inversesqrt() function in mx_math.metal #2010

Conversation

ld-kerley
Copy link
Contributor

inversesqrt() is currently inside of a #ifdef __DECL_GL_MATH_FUNCTIONS__ guard, which means it is not visible inside of OpenUSD's Storm hdMetal backend.

This PR moves it outside of the guard, and renames the function to mx_inversesqrt() to avoid possible name conflicts in other shader generators.

…ename function to mx_inversesqrt() to ensure no name clash with other shader generators. This is important for the Metal shdaer generation in Storms hdMetal backend.
… the metal code for now, to keep the types supported in line with each other.
@shill-lucasfilm
Copy link
Contributor

Just for my own understanding, what is different about inversesqrt() here versus the other functions inside the #ifdef __DECL_GL_MATH_FUNCTIONS__ guard? I'm guessing it has something to do with where it's being used, but that's not clear to me.

@ld-kerley
Copy link
Contributor Author

TL;DR - I think inversesqrt is not currently defined by other shader code added by hydra - but it may be added in the future.

I don't claim to fully understand the backstory here - but the MaterialX code generator in Hydra storm for Metal does not set __DECL_GL_MATH_FUNCTIONS__. It also introduces other shader pre-amble before the materialX code generation. There was a previous case (mod I think) where functions were being defined both in the preamble and by the shader code generated by MaterialX, so there the path forwards was to give the function a MaterialX specific prefix. I'm actually wondering if we should do that for all the functions inside of the __DECL_GL_MATH_FUNCTIONS__ guard, and then that means not using the builtin GLSL versions of those functions but instead calling the mx_ prefixed wrappers.

@jstone-lucasfilm - what do you think? Should we refactor the guard away completely, and just move to using mx_ prefixed function calls that wrap the GLSL and MSL implementations, where they differ?

@jstone-lucasfilm
Copy link
Member

jstone-lucasfilm commented Sep 12, 2024

@ld-kerley I believe the current design choice, using the __DECL_GL_MATH_FUNCTIONS__ guard around MSL implementations of common GLSL functions, was part of the original implementation of Metal support in MaterialX from @Morteeza.

If Storm is not currently setting this flag in Metal builds, then I can imagine two potential paths forward:

  1. Update Storm to consistently set __DECL_GL_MATH_FUNCTIONS__ in Metal builds.
  2. Remove this guard entirely from MaterialX, defining mx_ prefixed helper methods that are implemented in both GLSL and MSL.

How challenging would it be to implement approach (1), where the guard is set in Storm? If that's not a practical solution, then I'm completely fine with pivoting to approach (2).

@ld-kerley
Copy link
Contributor Author

I don't think it would be terribly hard to do (1), though getting the PR merged with Pixar can take some time sometimes.

I think I'd vote for (2) though, as it will isolate us from potential future conflicts. Like the previous mod() conflict here.

I think it would be good to merge this PR for now, so that anyone who happens to be trying to render OpenPBR, with Metal in Storm will get the fix, and I can put up another PR soon refactoring out __DECL_GL_MATH_FUNCTIONS__.

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 approach sounds good to me, @ld-kerley, and let's move forward with this change, since it's the pattern that we intend to apply to other cross-platform functions in the future.

@jstone-lucasfilm jstone-lucasfilm merged commit 34907af into AcademySoftwareFoundation:main Sep 12, 2024
34 checks passed
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