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

Refactor mx_math files to remove __DECL_GL_MATH_FUNCTIONS__ guard #2014

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ld-kerley
Copy link
Contributor

Refactor mx_math.glsl and mx_math.metal to remove the __DECL_GL_MATH_FUNCTIONS__ guard, and prefix any necessary functions to ensure isolation from other shader generators, such as HdStorm

…FUNCTIONS__ guard, and prefix any necessary functions to ensure isolation from other shader generators.
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.

Although this solution is a practical path forward, one concern I have is that GLSL developers would need to remember that atan must always be written as mx_atan, while other trigonometric functions such as sin and cos would continue to be expressed without any prefix.

To my mind, this represents a potential advantage of @Morteeza's original approach, where the appropriate wrappers for standard GLSL functions were added on the MSL side, allowing common GLSL patterns to be expressed in the most natural way.

Could it be that this change would make MaterialX GLSL code more difficult to write and read, and that we should consider the alternative approach where Storm adds the required preprocessor flag in MSL?

@ld-kerley
Copy link
Contributor Author

@jstone-lucasfilm I totally understand your perspective, and I do concede that it does feel like an additional layer of complexity here.

The first observation I would make is that the problem this PR is trying to address is not unique to storm, but applies to any downstream custom shader generator, some of which may not be publicly known, or open source. I think the project should be trying to provide a positive experience not only for shader writers but integrators of the project.

At a higher level, this problem is really a manifestation of the fact that we don't have documentation (or even better specification) of the requirements for implementing a custom shader generation target. Trying to solve this shortfall in a complete way seems like a significant undertaking, which is why this current PR feels like a more pragmatic approach to me.

I will note that this does not require a GLSL shader author to use mx_atan() if they are only writing GLSL code. The standard function call will still be available if the shader author is working solely in a GLSL environment, the point at which mx_atan() would be necessary only when the shader author is trying to write code that would target both GLSL and MSL. If there existed a new unit test for whatever the new functionality is, then I believe this would be caught as soon as. they were run, and easily changed.

Maybe a different perspective is that the current __DECL_GL_MATH_FUNCTIONS__ guard technique is a way to try and make MSL simulate GLSL. My approach is is a bit more egalitarian and tries to provide a small math API that allows both GLSL and MSL to use a common API, without favoring GLSL over MSL. This feels more scalable in a potential future that we might add other languages that want to share this code.

@jstone-lucasfilm
Copy link
Member

@ld-kerley Just to consider the alternative approach, what if we were to remove the __DECL_GL_MATH_FUNCTIONS__ guard from MSL and make no other changes? Could we rely upon GLSL methods such as atan and inverse being consistently unavailable in MSL, allowing the MaterialX-provided methods to be added without any conflict?

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.

2 participants