Metal: GLSL shader compatibility changes for global uniform and interface name collision.
ClosedPublic
Actions

Authored by Jason Fielder (jason_apple) on Sep 6 2022, 6:07 PM.

Details

Reviewers

Clément Foucault (fclem)

Maniphest Tasks

T96261: Metal Viewport

Commits

rB18b45aabf91a: Metal: GLSL shader compatibility changes for global uniform and interface name…

Summary

For the Metal shader translation support for shader-global uniforms are remapped via macro's, and in such cases where a uniform name matches a vertex attribute name, compilation errors will occur due to this injected syntax being incompatible with the immediate code.

Also adding source-level function interface alternatives where sized arrays are passed in. These are not supported directly in Metal shading language and are instead handled as pointers. These pointers require explicit address-space qualifiers in some cases, if device/constant address space memory is passed into the function.

Ref T96261

Diff Detail

Repository: rB Blender

Event Timeline

Jason Fielder (jason_apple) requested review of this revision.Sep 6 2022, 6:07 PM

Jason Fielder (jason_apple) created this revision.

Harbormaster completed remote builds in B23642: Diff 55366.Sep 6 2022, 6:07 PM

Michael Parkin-White (MichaelPW) retitled this revision from Metal: GLSL shader compatibility changes for global uniform and interface name collision, as globals are handled via macro's, and in such cases where a uniform name matches a vertex attribute name, compilation errors will occur. to Metal: GLSL shader compatibility changes for global uniform and interface name collision..Sep 6 2022, 6:44 PM

Michael Parkin-White (MichaelPW) edited the summary of this revision. (Show Details)

Metal: GLSL shader compatibility changes for global uniform and interface name collision, as globals are handled via macro's, and in such cases where a uniform name matches a vertex attribute name, compilation errors will occur.
Address to some extent issues with invalid embedded IDs in existing files.

Harbormaster completed remote builds in B23643: Diff 55373.Sep 6 2022, 8:28 PM

Small changes needed but otherwise looks good to me.

source/blender/blenkernel/intern/collection.c
239–252 ↗	(On Diff #55373)	Are these changes expected? It looks like a merge error since they are unrelated to shaders. Same thing for node.cc.
source/blender/draw/engines/workbench/shaders/workbench_transparent_accum_frag.glsl
9–16	Better replace local `viewvecs` with `drw_view.viewvecs` directly to avoid this issue in this particular case. However, I'm curious to know if this is a performance issue or a compilation error?
source/blender/gpu/intern/gpu_shader_interface.cc
25–31 ↗	(On Diff #55373)	Use `MEM_SAFE_FREE` instead.

This revision now requires changes to proceed.Sep 9 2022, 11:38 AM

Michael Parkin-White (MichaelPW) added inline comments.Sep 14 2022, 1:28 PM

source/blender/blenkernel/intern/collection.c
239–252 ↗	(On Diff #55373)	Ah yes apologies, looks like a few changes got scooped in as part of the rebase. Will fix diff along with any other changes. Thanks.
source/blender/draw/engines/workbench/shaders/workbench_transparent_accum_frag.glsl
9–16	Will replace this case. This is a compilation error currently. The passing of array syntax is invalid in MSL, and instead the data type would need to be treated as a pointer, rather than a scalar type. As this is a pointer, address space is required. Pointers in MSL are implicitly thread local unless otherwise specified. In this case, as the data comes from a uniform constant buffer, the data type is of "constant" to global memory, and thus needs to be explicitly specified. One other way around this would be to instead pack the type into a struct and pass by value.