-
Notifications
You must be signed in to change notification settings - Fork 352
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
Refactor mx_math files to remove GLSL guard #2014
Conversation
…FUNCTIONS__ guard, and prefix any necessary functions to ensure isolation from other shader generators.
There was a problem hiding this 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?
@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 Maybe a different perspective is that the current |
@ld-kerley Just to consider the alternative approach, what if we were to remove the |
@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 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? |
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 ? |
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:
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? |
…mmetry with atan() prefixing required.
Slack conversation pertaining to this work is here. |
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
__DECL_GL_MATH_FUNCTIONS__
guardSigned-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]>
cb04ef7
into
AcademySoftwareFoundation:main
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