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

MSL: Use of MSLVertexAttr #1050

Closed
billhollings opened this issue Jun 29, 2019 · 4 comments · Fixed by #1384
Closed

MSL: Use of MSLVertexAttr #1050

billhollings opened this issue Jun 29, 2019 · 4 comments · Fixed by #1384
Labels
question Further progress depends on answer from issue creator.

Comments

@billhollings
Copy link
Contributor

The use of MSLVertexAttr has evolved over time. Currently...almost none of the information it contains is used at all within SPIRV-Cross...I assume because we rely completely on the stage_in construct (originally we supported both stage_in and indexed vertex access.

Like a good government worker...MoltenVK is currently diligently filling out the contents of this struct...even though most of it serves no purpose.

I'm wondering if we should trim this struct...either remove it entirely in favour of other API options...or at least trim (and maybe rename) it for it's current use as a re-mapper of tessellation formats and builtins.

@HansKristian-Work @cdavis5e ?

@billhollings billhollings added the question Further progress depends on answer from issue creator. label Jun 29, 2019
@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Jul 1, 2019

Yup, looking at MSLVertexAttr, it does not seem to be relevant anymore. I know the format is used to do fixups, but is that all?

I'm fine changing the API in the C++ backend, but I cannot break any API in the C wrapper. We can deprecate and add a better interface though, as long as we can express the old APIs with the new. As I understand it, most of the fields in MSLVertexAttr are completely ignored anyways, so we don't have to care about those when moving to a new API.

@billhollings
Copy link
Contributor Author

billhollings commented Jul 1, 2019

Understood about the C ABI...although I wonder if anyone is using the C equivalents to this...since the C++ struct was driven by MoltenVK originally. But we can just stub the references out if we change this struct I guess.

@cdavis5e Since the remaining use of MSLVertexAttr is related to tessellation functionality...I'll leave it to you to comment about whether we should simply strip the unused members from the struct...or change the API in some other way (eg- move tess info to a different struct or function and get rid of the struct altogether).

@HansKristian-Work
Copy link
Contributor

@billhollings @cdavis5e Any further comments?

@billhollings
Copy link
Contributor Author

As I mention...I'm fine to trim it (preferred) or remove it from the C++ API and replace with something simpler. The C ABI could stay the same since it uses a parallel spvc_msl_vertex_attribute struct.

As far as I can tell...SPIRV-Cross only uses the location, builtin, and format values.

@cdavis5e Most of those three are used by code introduced with tess...so you'd be better able to remark on whether we can elide any of those elements too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further progress depends on answer from issue creator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants