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

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

Conversation

jay3d
Copy link

@jay3d jay3d commented Apr 24, 2023

The title says all.

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

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

Changes' descriptions should, you know, describe what the change actually does and, more important, why. 😃 It isn't obvious that this change is necessary or even desirable.

Are you seeing fragment shaders with barycentric inputs that have one of the interpolation decorations? What app/game?

Flat and NoPerspective decorations on these should never be seen. Flat cannot be used at all with barycentrics, and NoPerspective is of course handled by BaryCoordNoPerspKHR. Your change will silently drop Centroid and Sample decorations, which will make shaders that use them "work" but cause incorrect rendering with MSAA when these decorations are used.

Comment on lines -11962 to 11965
if (builtin == BuiltInBaryCoordKHR || builtin == BuiltInBaryCoordNoPerspKHR)
if (builtin == BuiltInBaryCoordKHR)
{
if (has_member_decoration(type.self, index, DecorationFlat) ||
has_member_decoration(type.self, index, DecorationCentroid) ||
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

@HansKristian-Work
Copy link
Contributor

Is there any forward progress on this PR?

@rcaridade145
Copy link

If DecorationNoPerspective is allowed this line https://github.com/KhronosGroup/SPIRV-Cross/blob/main/spirv_msl.cpp#L11981 should not be here.
Not exactly sure where center_perspective would come from.

@jay3d
Copy link
Author

jay3d commented Jun 25, 2023

Are you seeing fragment shaders with barycentric inputs that have one of the interpolation decorations? What app/game?

My app, and that's enough for me: https://www.youtube.com/watch?v=_omNIB8q4vM

I'm all ears... How do you suggest this could be fixed?

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