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.

Sign up for GitHub

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

Jump to bottom

Fixed generating Metal shaders that make use of fragment barycentric coords #2137

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

Fixed generating Metal shaders that make use of fragment barycentric coords #2137

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 32 additions & 29 deletions spirv_msl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11959,7 +11959,7 @@ string CompilerMSL::member_attribute_qualifier(const SPIRType &type, uint32_t in
else
quals = member_location_attribute_qualifier(type, index);

if (builtin == BuiltInBaryCoordKHR || builtin == BuiltInBaryCoordNoPerspKHR)
if (builtin == BuiltInBaryCoordKHR)
{
if (has_member_decoration(type.self, index, DecorationFlat) ||
has_member_decoration(type.self, index, DecorationCentroid) ||
Comment on lines -11962 to 11965
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comment.

Without my changes, when compiling GLSL shaders to SPIR-V which has gl_BaryCoordNoPersp, then using SPIRV-Cross to generate Metal shaders, this error will be displayed:

Flat, Centroid, Sample, NoPerspective decorations are not supported for BaryCoord inputs.

Which is not entirely true because Metal supports Centroid and NoPerspective decorations for BaryCoord input according to this spec:
https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf
at page 103:

A fragment function input declared with the [[barycentric_coord]] attribute can only be
declared with either the center_perspective (default) or center_no_perspective
sampling and interpolation attributes. The barycentric coordinates and per-pixel primitive ID
can be passed as fragment function input in structures organized as shown in these examples:

struct FragmentInput0 {
    uint primitive_id [[primitive_id]];
    // [[center_perspective]] is the default, so it can be omitted.
    float3 barycentric_coord [[barycentric_coord, center_perspective]];
};

struct FragmentInput1 {
    uint primitive_id [[primitive_id]];
    float2 linear_barycentric_coord [[barycentric_coord,
    center_no_perspective]];
};

So there's clearly a bug there; My changes may not be 100% perfect but they work in my use case.
You may take a look further and advice accordingly.
PS: This PR will complement this: KhronosGroup/glslang#3198

Expand All @@ -11977,35 +11977,38 @@ string CompilerMSL::member_attribute_qualifier(const SPIRType &type, uint32_t in
// FragCoord builtin; it's always noperspective on Metal.
if (!type_is_integral(mbr_type) && (!is_builtin || builtin != BuiltInFragCoord))
{
if (has_member_decoration(type.self, index, DecorationFlat))
if (builtin != BuiltInBaryCoordKHR && builtin != BuiltInBaryCoordNoPerspKHR)
{
if (!quals.empty())
quals += ", ";
quals += "flat";
}
else if (has_member_decoration(type.self, index, DecorationCentroid))
{
if (!quals.empty())
quals += ", ";
if (has_member_decoration(type.self, index, DecorationNoPerspective))
quals += "centroid_no_perspective";
else
quals += "centroid_perspective";
}
else if (has_member_decoration(type.self, index, DecorationSample))
{
if (!quals.empty())
quals += ", ";
if (has_member_decoration(type.self, index, DecorationNoPerspective))
quals += "sample_no_perspective";
else
quals += "sample_perspective";
}
else if (has_member_decoration(type.self, index, DecorationNoPerspective))
{
if (!quals.empty())
quals += ", ";
quals += "center_no_perspective";
if (has_member_decoration(type.self, index, DecorationFlat))
{
if (!quals.empty())
quals += ", ";
quals += "flat";
}
else if (has_member_decoration(type.self, index, DecorationCentroid))
{
if (!quals.empty())
quals += ", ";
if (has_member_decoration(type.self, index, DecorationNoPerspective))
quals += "centroid_no_perspective";
else
quals += "centroid_perspective";
}
else if (has_member_decoration(type.self, index, DecorationSample))
{
if (!quals.empty())
quals += ", ";
if (has_member_decoration(type.self, index, DecorationNoPerspective))
quals += "sample_no_perspective";
else
quals += "sample_perspective";
}
else if (has_member_decoration(type.self, index, DecorationNoPerspective))
{
if (!quals.empty())
quals += ", ";
quals += "center_no_perspective";
}
}
}

Expand Down