Skip to content

WGSL/WebGPU GLSL ShaderGenerator#2407

Merged
jstone-lucasfilm merged 30 commits into
AcademySoftwareFoundation:mainfrom
scotbrew:brews/feature/wgsl
Jun 9, 2025
Merged

WGSL/WebGPU GLSL ShaderGenerator#2407
jstone-lucasfilm merged 30 commits into
AcademySoftwareFoundation:mainfrom
scotbrew:brews/feature/wgsl

Conversation

@scotbrew

@scotbrew scotbrew commented May 22, 2025

Copy link
Copy Markdown
Contributor

Adding WgslShaderGenerator and its support classes to generate WGSL-compliant GLSL code. This is targeted to allow USD to render MaterialX shaders with the WebGPU implementation.

Main changes:

  1. LightData.type renamed to LightData.light_type -- "type" is a reserved word in WGSL. The change only affects the WGSL flavor of GLSL.
  2. Changing the function signature and calls for all functions that use (sampler2D) to (texture2D, sampler), as WGSL GLSL is more restrictive and does not support sampler2D.

Pull Request Sections:

  1. WgslShaderGenerator and its support classes
  2. WGSL-compliant GLSL shaders (filenames: *_wgsl.glsl) (using #define HW_SEPARATE_SAMPLERS in glsl shader code)
  3. Small modifications to base classes or refactoring a few .glsl files. and adding 2 baseclass virtual functions.

Issues Resolved:
This PR addresses the following logged issues.

scotbrew added 7 commits May 16, 2025 13:42
…erator.

* Derive WgslShaderGenerator and similar classes from VkShaderGenerator.
* Create virtual function under ShaderGenerator.h to specify member variable name of LightData.type. (shifting to LightData.light_type for WGSL)
…r2D.

* Refactor mx_microfacet_specular.glsl to extract out mx_latlong_map_lookup.
* Add shader code: mx_image*_wgsl.glsl, mx_environment*_wgsl.glsl, and mx_hextiled*_wgsl.glsl
* Update stdlib_genglsl_impl.mtlx to add genglsl_wgsl implementations.
* Add "glsl_wgsl" target type.
@linux-foundation-easycla

linux-foundation-easycla Bot commented May 22, 2025

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

Comment thread libraries/pbrlib/genglsl/lib/mx_microfacet_specular.glsl
Comment thread libraries/pbrlib/genglsl/lib/mx_latlong.glsl Outdated
Comment thread libraries/pbrlib/genglsl/lib/mx_microfacet.glsl Outdated
Comment thread libraries/targets/genglsl_wgsl.mtlx Outdated
Comment thread resources/Materials/TestSuite/_options.mtlx Outdated
@scotbrew

Copy link
Copy Markdown
Contributor Author

No testing done with Js currently. Likely something would need to change around the LightData struct so it can use LightData.light_type instead of LightData.type to have Js work with WebGPU/WGSL.

Comment thread source/MaterialXGenGlsl/GlslShaderGenerator.cpp Outdated
Comment thread source/MaterialXGenGlsl/Nodes/LightSamplerNodeGlsl.cpp Outdated
Comment thread source/MaterialXGenGlsl/WgslResourceBindingContext.cpp
Comment thread source/MaterialXGenGlsl/WgslResourceBindingContext.cpp
Comment thread source/MaterialXGenGlsl/WgslResourceBindingContext.h Outdated
Comment thread source/MaterialXGenGlsl/WgslShaderGenerator.cpp
Comment thread source/MaterialXGenGlsl/WgslShaderGenerator.cpp Outdated
Comment thread source/MaterialXGenGlsl/WgslShaderGenerator.cpp Outdated
Comment thread source/MaterialXGenMsl/Nodes/LightSamplerNodeMsl.cpp Outdated
Comment thread source/MaterialXGenShader/HwShaderGenerator.cpp Outdated
Comment thread source/MaterialXGenShader/Nodes/CompoundNode.cpp Outdated
Comment thread source/MaterialXTest/MaterialXGenGlsl/GenGlsl.cpp
@scotbrew scotbrew marked this pull request as ready for review May 22, 2025 18:49
@ashwinbhat

Copy link
Copy Markdown
Contributor

Hi @scotbrew greatly appreciate you getting this work started 👍🏽 I briefly looked at the changes and think you are addressing all the key areas wrt the sampler split. However, I see that we will have to duplicate major blocks of shader code for minor differences in function call.
While having separate _wgsl.glsl and .glsl allows us to tweak the wgsl version later, I'm wondering for the initial release if we could consolidate mx_environment_* and hex_tile by using a compile time macro. So something like this:
#define VULKAN 100
#if VULKAN > 100
mx_latlong_map_lookup(Lw, $envMatrix, lod, $envRadiance_texture, $envRadiance_sampler)
#else
mx_latlong_map_lookup(Lw, $envMatrix, lod, $envRadiance)
#endif

Could we introduce a hwSeparateSamplers option in https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenShader/GenOptions.h that allows this branching and emits the required #define. This will also reduce the need to create a separate WgslResourceBindingContext since Vulkan ResourceBindingContext could simply choose to sperate or combine based on hwSeparateSamplers option setting.

Based on suggestion from Niklas slack I think we want to use genwgsl instead of genglsl_wgsl so this will allow underlying implementation to change and remove the glsl notion in future.

@scotbrew

scotbrew commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

@ashwinbhat , the #define is a great idea for a way to minimize the branching of the code.

I was not seeing a way for the WgslShaderGenerator to specify the GenOptions from the context in the constructor. We can leave adding a GenOption as a phase2 if wanting to pull it back to Vulkan.

void WgslShaderGenerator::emitDirectives(GenContext& context, ShaderStage& stage) const
{
    VkShaderGenerator::emitDirectives(context, stage);
    if (genContext.getOptions(). hwSeparateSamplers) {
        emitLine("#define HW_SEPARATE_SAMPLERS");
    }
}

@kwokcb kwokcb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. i was hoping we could virtualize or modularize the lookups more but the raw code is pretty ugly. Even though there is duplication I'm not really a big fan of compiler directives in code, but I guess that's good enough for now. I was hoping that the image functions at least had emitted headers to branch on but that does not seem to be the case.

  2. I'd prefer virtualization instead of introducing tests for a specific generator.

  3. It should be pretty straight-forward to add in genwgsl test and that should give you pretty good base-line unit test coverate. If there is way to validate the code, then we can look at adding something to CI later on via the generateshader.py script. If you something that users can run to perform any validation would be good to document this.

Comment thread resources/Materials/TestSuite/_options.mtlx Outdated
Comment thread source/MaterialXGenGlsl/WgslResourceBindingContext.cpp
Comment thread source/MaterialXGenShader/Nodes/CompoundNode.cpp Outdated
Replace constructed string with static for virtual function getLightDataTypevarString().  Change return type to string&.

This more closely follows the MaterialX class conventions.
@scotbrew

Copy link
Copy Markdown
Contributor Author

I've combed over the changes to streamline a bit further. PR should be ready for final review.

@kwokcb kwokcb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great with some minor items:

  1. It would be good to expose the environment texture / sampler uniform names as a courtesy to implementors so they know what uniforms to use for binding.

I can't think of any other way to make the source code cleaner than what's done here with the preprocessor directive.

  1. I think the sampler gradient call syntax is incorrect ?

Thanks.

Comment thread libraries/pbrlib/genglsl/lib/mx_environment_fis.glsl
Comment thread libraries/pbrlib/genglsl/lib/mx_environment_fis.glsl
Comment thread libraries/stdlib/genglsl/mx_hextilednormalmap.glsl
scotbrew added 2 commits May 24, 2025 23:32
* Creating T_ENV_RADIANCE_TEXTURE and T_ENV_RADIANCE_SAMPLER
* Creating HW::ENV_RADIANCE_TEXTURE and HW::ENV_RADIANCE_SAMPLER
* Updating _tokenSubstitutions() for new strings.

@scotbrew scotbrew left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all comments and suggestions.

Comment thread source/MaterialXGenGlsl/WgslSyntax.cpp Outdated

@JGamache-autodesk JGamache-autodesk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from missing WGSL forbidden keywords in the Syntax, everything else looks fine to me.

Comment thread libraries/stdlib/genglsl/mx_image_color3.glsl
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
@jstone-lucasfilm

Copy link
Copy Markdown
Member

@scotbrew Oh, I just noticed that our render validation tests are returning all-black renders with this PR!

I've confirmed the same results locally on my Windows machine, and here are the current renders from the Viewer and Graph Editor with this PR:

Viewer:

Viewer

Graph Editor:

GraphEditor

@jstone-lucasfilm

Copy link
Copy Markdown
Member

Looking back through recent GitHub Actions runs, here's the first one I see where the render validation tests are returning black renders, in case that helps to track down the cause:

https://github.com/AcademySoftwareFoundation/MaterialX/actions/runs/15331741009

@scotbrew

scotbrew commented Jun 7, 2025

Copy link
Copy Markdown
Contributor Author

That is definitely one to catch and fix before fully integrating.

Updates:

  1. I'll take a look to repro it on my end and compare shader code to identify the fix.
  2. Verified locally that the latest MaterialXView with this PR has black renders. Tracking down the introduced issue.
  3. The issue has been isolated to a refactoring change for the PR in the commit "WGSL: Refactor shader code and CompountNode shader gen" (SHA: 131d322 ).
  4. Fixed in the latest commit. The refactoring of ShaderGenerator::emitFunctionDefinitionParameter() needed to treat input and output ShaderPorts slightly differently.

Fix issue introduced when refactoring code to use ShaderGenerator::emitFunctionDefinitionParameter().

OutputPorts need to use _syntax->getOutputTypeName() instead of _syntax->getTypeName().

Fix commit from "WGSL: Refactor shader code and CompountNode shader gen" (sha: 131d322)
@scotbrew

scotbrew commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

The identified render issue regression has been addressed. Here are the latest images from the Linux test runners.
GraphEditor_MarbleSolid Viewer_BrassAverage Viewer_CarpaintTranslated

@scotbrew

scotbrew commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

For code spot-checking, a comparison was done between the standard_surface_look_brass_tiled shader code generated by the latest PR2407 code and latest main branch. Everything looks as expected.

  • VS: Identical
  • PS: Diff adding only code scoped by #ifdef HW_SEPARATE_SAMPLERS. (see attached *-DIFF.txt)
  • MDL: Identical

standard_surface_look_brass_tiled_ps-DIFF.txt

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this contribution, @scotbrew, and I believe this looks ready to go!

Further in the future, I'd be interested in trying additional strategies to reduce the interface duplication in the hardware implementations of our image nodes, but the benefits of this initial implementation greatly outweigh those minor issues.

@jstone-lucasfilm jstone-lucasfilm merged commit 8eaf80c into AcademySoftwareFoundation:main Jun 9, 2025
32 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.

5 participants