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

Adds Metal Shader Generation and Rendering Support #1258

Merged
merged 39 commits into from
Mar 16, 2023

Conversation

Morteeza
Copy link
Contributor

Adds Metal Support to MaterialXShaderGen Backend
Adds Metal Support to MaterialX Rendering code for testing generated shaders
Adds Metal Support to MaterialXView Rendering Support (switchable between Metal and OpenGL, default is Metal)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Adds Metal Support to MaterialX Rendering code for testing generated shaders
Adds Metal Support to MaterialXView Rendering Support (switchable between Metal and OpenGL, default is Metal)
Copy link
Contributor

@kwokcb kwokcb left a comment

Choose a reason for hiding this comment

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

Hi @Morteeza , this is a lot of work done here so congrats on getting this done!
I've left some minor points, and future points. For the PR itself, I think it would be good to address:

  1. Target inheritance. Seems that the approach is to use GLSL code snippets as much as possible and tweaking them during code generation. Just making it inherit in the target definition is a small start.
  2. I assume at some point we'd want to check the Python wrappers for this. One reason I am curious to expose this is I think you can use the metal command line to perform compile line compilation testing and we have a python script called generateShader.py which could add in the Metal generator to test with a supplied command line validator.
  3. Along the lines of validation, I'm not sure how you perform unit tests for compilation? (I think I'm just missing something). We could keep what's there are allow an external program to perform validation as is done for OSL. I raise this in case this is seen as a better option ?
  4. There are some comments about the derivation of interfaces, and I only want to bring up if we want to have
    • a "pipeline" interface to extract all backend specifics as opposed to splitting the Viewer class directly.
    • a "texture baker" based interface as there just seems to so much duplicate code there.

Just some "2-cent" stuff to think over :).

libraries/stdlib/genmsl/lib/mx_hsv.metal Outdated Show resolved Hide resolved
libraries/targets/genmsl.mtlx Outdated Show resolved Hide resolved
resources/Lights/envmap_shader.mtlx Show resolved Hide resolved
source/MaterialXRenderMsl/TextureBaker.h Outdated Show resolved Hide resolved
source/MaterialXRenderMsl/TextureBaker.mm Outdated Show resolved Hide resolved
source/MaterialXTest/MaterialXGenMsl/CompileMSL.mm Outdated Show resolved Hide resolved
source/MaterialXTest/MaterialXRenderMsl/RenderMsl.mm Outdated Show resolved Hide resolved
source/MaterialXView/ViewerMSL.mm Outdated Show resolved Hide resolved
Refactorings to address comments on the review (TextureBaker, MetalTexture class, Math-Scalar Operators, MSLProgram, ...)
# Conflicts:
#	source/MaterialXRenderGlsl/TextureBaker.cpp
It causes linker errors for Visual Studio, but in any case exporting template classes is not helpful.
@Morteeza
Copy link
Contributor Author

Morteeza commented Mar 7, 2023

@kwokcb @jstone-lucasfilm This is second revision of the change. I appreciate it if you folks review it and provide me more comments.

Also, I would like to add MSL python bindings, shader graph editor, and render pipeline (viewer refactoring) as a future change.

Looking forward to your comments... and thanks in advance

@Morteeza Morteeza requested a review from kwokcb March 7, 2023 20:34
@Morteeza
Copy link
Contributor Author

Morteeza commented Mar 8, 2023

Now, we have python bindings for Metal Shading Language generator. We can also use xcrun metal --language=metal as validator to compile and validate generated shaders.

@Morteeza Morteeza force-pushed the main branch 2 times, most recently from e2acf59 to ac037ce Compare March 8, 2023 16:14
…ew abstraction call RenderPipeline that takes care of rendering. This change is done with goal of reducing code duplication.
@Morteeza
Copy link
Contributor Author

@kwokcb @jstone-lucasfilm If you review MaterialXView project files again, you can see that we don't have ViewerCommon, ViewerGL, and ViewerMetal files anymore; but instead I moved the code for rendering to MetalRenderPipeline and GLRenderPipeline class. Please let me know how is that change? We can revert it back or improve it.

Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Copy link
Contributor

@kwokcb kwokcb left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks @Morteeza
There is just one minor test documentation comment that can be looked at after.

python/Scripts/generateshader.py Show resolved Hide resolved
source/MaterialXRenderGlsl/TextureBaker.h Show resolved Hide resolved
source/MaterialXRenderMsl/MslRenderer.mm Show resolved Hide resolved
source/MaterialXView/RenderPipeline.h Show resolved Hide resolved
Copy link
Contributor

@kwokcb kwokcb left a comment

Choose a reason for hiding this comment

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

Sorry for the additional spam but looking at TextureBaker, it seems like the MSL one will replace the GLSL one on Mac. I left a comment for an option for the Python wrappers, but any existing users for this (which includes Usd) would be switching to a "new baker" on Mac so a possible cause concern for integration ?

python/Scripts/baketextures.py Outdated Show resolved Hide resolved
python/Scripts/translateshader.py Outdated Show resolved Hide resolved
/// The structure is populated by directly scanning the program so may not contain
/// some inputs listed on any associated HwShader as those inputs may have been
/// optimized out if they are unused.
struct MX_RENDERMSL_API Input
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Morteeza, What do you think on this comment. Sorry for being nitpicky :).

…both baketextures and translateshader python scripts. The default is left to be "Metal"
@Morteeza
Copy link
Contributor Author

Sorry for the additional spam but looking at TextureBaker, it seems like the MSL one will replace the GLSL one on Mac. I left a comment for an option for the Python wrappers, but any existing users for this (which includes Usd) would be switching to a "new baker" on Mac so a possible cause concern for integration ?

Thanks for the feedback. As I mentioned on previous comments I added the option to use glsl texture baker on Mac.

@Morteeza
Copy link
Contributor Author

Morteeza commented Mar 14, 2023

#1258 (comment)

@kwokcb Not nitpicking at all. The comment still holds. Did I get that correctly?

Morteeza and others added 6 commits March 15, 2023 13:33
@Morteeza
Copy link
Contributor Author

Hi @pablode

Thanks again for checking out MaterialX Metal backend. Today I tried it on macOS 10.15.7 and fixed issues we were facing due to a slight change in API behaviour. It should work fine now, so we only fall back to OpenGL backend for MaterialXView for 10.14 and below, now.

@pablode
Copy link
Contributor

pablode commented Mar 15, 2023

@Morteeza Awesome - and I can confirm that it works! Coincidentally, I did some investigation too, and found some potential improvements (see PR in your repo).

Metal backend improvements

Thanks to @pablode
@Morteeza
Copy link
Contributor Author

@Morteeza Awesome - and I can confirm that it works! Coincidentally, I did some investigation too, and found some potential improvements (see PR in your repo).

Awesome! Thanks for your improvements. I appreciate the effort. I merged all changes in.

This changelist addresses a minor warning flagged by PVS-Studio, where an evaluated expression was always false in the given context.
This changelist clarifies the parenthesis guards of the TEXTURE_NAME and SAMPLER_NAME macros.
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.

Great work on these new features, thanks @Morteeza!

@Morteeza
Copy link
Contributor Author

Great work on these new features, thanks @Morteeza!

Thanks @jstone-lucasfilm for improvements and support 👍

@jstone-lucasfilm jstone-lucasfilm merged commit 6f8b3bd into AcademySoftwareFoundation:main Mar 16, 2023
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…undation#1258)

- Adds Metal Support to MaterialXShaderGen Backend
- Adds Metal Support to MaterialX Rendering code for testing generated shaders
- Adds Metal Support to MaterialXView Rendering Support (switchable between Metal and OpenGL, default is Metal)
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.

4 participants