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

Fix Metal shader errors with MaterialX 1.39 #3519

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

brechtvl
Copy link
Contributor

@brechtvl brechtvl commented Feb 5, 2025

Description of Change(s)

  • mx_microfacet.glsl uses functions from mx_math.metal, so the latter must be included first.
  • textureQueryLevels(u_envRadiance) did not take into account that this texture is wrapped in a MetalTexture class.
  • Defining atan(y, x) for GLSL compatibility conflicts with mx_math.metal using ::atan(y_over_x). Work around this by tweaking the definition.

Fixes Issue(s)

Checklist

@brechtvl
Copy link
Contributor Author

brechtvl commented Feb 5, 2025

Here is an example USD file, producing the following errors:

Warning: in _ValidateCompilation at line 212 of usd/src/external_usd/pxr/imaging/hdSt/glslProgram.cpp -- Failed to compile shader (FRAGMENT_SHADER): program_source:2656:17:
    return vec3(mx_cos(phi) * sinTheta,
                ^
program_source:2657:17: error: use of undeclared identifier 'mx_sin'
                mx_sin(phi) * sinTheta,
                ^
program_source:2667:17: error: use of undeclared identifier 'mx_cos'
    return vec3(mx_cos(phi) * sinTheta,
                ^
program_source:2668:17: error: use of undeclared identifier 'mx_sin'
                mx_sin(phi) * sinTheta,
                ^
program_source:2785:12: error: no matching function for call to 'atan'
    return ::atan(y_over_x);
           ^~~~~~
program_source:153:3: note: candidate function template not viable: requires 2 arguments, but 1 was provided
T atan(T y, T x) { return atan2(y, x); }
  ^
program_source:2925:25: error: no member named 'get_num_mip_levels' in 'ProgramScope_Frag::MetalTexture'
    u_envRadianceMips = textureQueryLevels(u_envRadiance);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
program_source:213:45: note: expanded from macro 'textureQueryLevels'
#define textureQueryLevels(texture) texture.get_num_mip_levels()
                                    ~~~~~~~ ^

I'm a bit confused about how this could have been working at all, as far as I can tell any scene using MaterialX has this problem.

@brechtvl
Copy link
Contributor Author

brechtvl commented Feb 5, 2025

CC @ld-kerley, since I guess you might want to be aware of this.

* mx_microfacet.glsl uses functions from mx_math.metal, so the
  latter must be included first.
* Defining atan(y, x) for GLSL compatibility conflicts with
  mx_math.metal using ::atan(y_over_x). Work around this by tweaking
  the definition.
* textureQueryLevels(u_envRadiance) does not take into account that
  this texture is wrapped in a MetalTexture class.

Signed-off-by: Brecht Van Lommel <[email protected]>
@dgovil
Copy link
Collaborator

dgovil commented Feb 5, 2025

I've asked Lee to take a look, but in the meantime these look good to me. Thanks for catching those, Brecht.

Do you know if these work backwards against 1.38 as well? Or do we need to guard accordingly?

@tcauchois this would likely need to get in for 25.5 if that is when you plan to switch to 1.39 by default.

@brechtvl
Copy link
Contributor Author

brechtvl commented Feb 5, 2025

I couldn't figure out if there are OpenUSD tests for MaterialX + Metal. So this is based on Blender tests.

  • 1.38.8: Doesn't build with OpenUSD 25.02 anymore.
  • 1.38.10: Seems to work with this PR. Some materials have pre-existing errors like:
program_source:2723:30: note: expanded from macro 'N4_file'
#define N4_file MetalTexture{HdGetSampler_N4_file(), samplerBind_N4_file}
                             ^
program_source:1036:7: note: 'HdGetScalar_N4_file' declared here
float HdGetScalar_N4_file() { return HdGet_N4_file(0).x; }
      ^
program_source:3603:21: error: no viable conversion from 'float' to 'texture2d<float>'
    mx_image_color4(N4_file, N4_layer, N4_default, N5_out, N4_uaddressmode, N4_vaddressmode, N4_filtertype, N4_framerange, N4_frameoffset, N4_frameendaction, N4_uv_scale, N4_uv_offset, N4_out);
  • 1.39.2: No errors in the Blender tests.

@dgovil
Copy link
Collaborator

dgovil commented Feb 5, 2025

Awesome, thanks!

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10652

(This is an automated message. See here for more information.)

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ld-kerley
Copy link

Firstly, I can confirm this PR appears to resolve the issues.

I was poking around at this from both the OpenUSD side and the MaterialX side and if we make some small changes on the MaterialX side then this PR can shrink significantly.

With the changes here, then this PR only needs to re-order the mx_math.metal include to come before the mx_microfacet.glsl include. Thats also something I think we should look at ways to fix on the MaterialX side too - but I don't think thats a trivial change.

@brechtvl
Copy link
Contributor Author

brechtvl commented Feb 6, 2025

Thanks! I'm happy to simplify or discard this PR if needed.

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