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

Implement invariant attribute #1789

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Mar 23, 2022

@teoxoy
Copy link
Member Author

teoxoy commented Mar 23, 2022

Also fixes HLSL's modifiers not being written in the vertex output and fragment input structs.

Before
no_mod

After
mod

BuiltIn {
built_in: BuiltIn,
/// can only be `true` for [`BuiltIn::Position`]
invariant: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps there is a better place for this flag to be?
If user locations are to be supported in the future than the current API of this PR wouldn't work, anyway.
If it's only ever going to be supported on VS positions, then we could place it on the entry point itself (see struct EntryPoint). Would you be interested to check and see how this works out in terms of code complexity?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the EntryPoint way but I think the code would end up a lot more complex than it is now since the frontends would need to first read the invariant attribute, then look for all entry points that use that position builtin (since it can be in a struct) and set it on the entry point. Backends would also have trouble with this since they would have to map the invariant attribute back.

If the spec changes to allow the attribute to be used on locations, we can change the binding to be a struct with an inner field being the enum we have now and add the invariant flag on the struct.

I agree that it might not be the best spot for it however I tried to put it in multiple places and decided at last to put it here.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good insight, thank you!

@kvark kvark requested a review from jimblandy March 28, 2022 05:05
@kvark
Copy link
Member

kvark commented Mar 28, 2022

Going to ask @jimblandy to take a look as well - on the API change only.
We take extra care for any API related changes. Sorry about the delay this is causing!

@teoxoy
Copy link
Member Author

teoxoy commented Mar 28, 2022

Thanks, no hurry!

@teoxoy
Copy link
Member Author

teoxoy commented Apr 10, 2022

Rebased to fix conflicts

@kvark
Copy link
Member

kvark commented Apr 11, 2022

Jim seems to have missed this one, unfortunately. Let's go ahead and merge then.

@kvark kvark merged commit 6ee1fd4 into gfx-rs:master Apr 11, 2022
@teoxoy teoxoy deleted the invariant-attribute branch April 11, 2022 07:46
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.

Support invariant qualifier
2 participants