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 GLSL guard #2014

Merged

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?

@jstone-lucasfilm
Copy link
Member

@ld-kerley There's a recent discussion on the ASWF Slack about extending shader generation to support HLSL in addition to the existing support for GLSL and MSL, and I think this provides another useful perspective on this proposal.

Following the current approach in MaterialX, developers would continue to author hardware shading nodes using native GLSL, and MaterialX would convert these features to their HLSL and MSL equivalents during shader generation.

In contrast, this proposal would take us along the path of providing mx_ wrappers around a growing set of native GLSL functions, and developers would be required to use these when writing hardware shading nodes (otherwise their code would work only on a single platform). In a sense, we would be defining a new shading language in MaterialX, and hardware shading nodes would be expressed in this language rather than an existing open shading language such as GLSL, HLSL, or MSL.

Although that's a valid path forward, to my mind it would make the authoring of hardware shading nodes more complex for developers (especially those working on new BSDF nodes with complex logic), without providing any benefits for artists authoring assets using MaterialX.

Given that context, perhaps it's preferable to follow our current approach, where GLSL is treated as the "neutral" hardware shading language in which our nodes are defined, and this code is converted to all other hardware shading languages during the shader generation process?

@ld-kerley
Copy link
Contributor Author

I want to preface these thoughts with the caveat that most of my shader writing experience is offline (OSL) and not realtime....

I saw the HLSL discussion, and it made me think of this PR again too.... though I think it made me think this PR was even more valid :). As I feel you've said before in previous meetings its good for MaterialX not to "pick favorites" when it comes to shading languages, and opting for native GLSL as the defacto language and requiring MSL and potentially now HLSL to conform to GLSL feels potentially a little inhibiting. I feel this PR proposes a somewhat more democratic approach.

There may well be concepts/features in MSL/HLSL that we want to leverage that do not have natural correspondences in GLSL.

Also the original intent of this PR was to make things more robust and stable for downstream integrators creating their own subclasses of these shader generators. HdStorm being the most commonly known of these, but I expect there are others. We don't control what additional code these code generators are creating, and we also do not have any documentation to declare or describe what the constraints these code generators need to conform to (I think such documentation would potentially impose harder restrictions on MaterialX once it was written than we might be happy with). There were one or two issues that needed to be resolved, with the interplay between MaterialX and HdStorm recently, and given the two projects have very different development velocities and release schedules, it seemed like a good idea to me to try and fix a robust solution moving forwards.

If we don't move forward with something similar to what is proposed in this PR, do you have any alternate ideas on how we can make the development landscape more robust for these downstream code generator integrations of MaterialX ?

@jstone-lucasfilm
Copy link
Member

These are great points, @ld-kerley, and I fully agree that there are two valid paths forward, each of which provides a sufficient set of tools to build and maintain hardware shading code across languages.

Here are the two paths forward that I see:

  1. Following the approach started by @Morteeza in his original MSL implementation, we would continue using GLSL as our canonical hardware shading language, with cross-compilation techniques used to generate shaders in HLSL, MSL, and other hardware languages in the future. This set of cross-compilation techniques would include preprocessor definitions, string replacement, and other future improvements that are needed to generate robust shading code in each target language.
  2. Following the approach that you propose here, we would create mx-prefixed macros around each concept that differs across hardware shading languages, and authors of hardware shading nodes would write in our "meta-language" rather than in a common language such as GLSL. Over time, our set of macros would likely include vector and matrix data types (e.g. mx_float3, mx_matrix44), trigonometric functions (e.g. mx_cos, mx_atan), and potentially even a common syntax for data structures and argument passing. This approach is similar in spirit to the Three.js Shading Language, which hides the differences between popular web shading languages such as GLSL and WGSL.

Each of these paths forward has its own strengths and weaknesses, with option (1) potentially being the clearest for readers and writers of MaterialX shading code, and option (2) potentially providing the greatest degree of future flexibility.

Perhaps this would make a good topic for an upcoming MaterialX TSC meeting?

@ld-kerley
Copy link
Contributor Author

Slack conversation pertaining to this work is here.

@jstone-lucasfilm jstone-lucasfilm changed the title Refactor mx_math files to remove __DECL_GL_MATH_FUNCTIONS__ guard Refactor mx_math files to remove GLSL guard Oct 9, 2024
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
@jstone-lucasfilm jstone-lucasfilm merged commit cb04ef7 into AcademySoftwareFoundation:main Oct 10, 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.

2 participants