Skip to content

Inconsistency on Vulkan Format enum #427

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

Open
gdkchan opened this issue Mar 17, 2021 · 6 comments
Open

Inconsistency on Vulkan Format enum #427

gdkchan opened this issue Mar 17, 2021 · 6 comments
Labels
area-Vulkan enhancement New feature or request
Milestone

Comments

@gdkchan
Copy link

gdkchan commented Mar 17, 2021

Summary of feature

Currently there's a small inconsistency on the Format enum names. The snorm formats have the N capitalized, so they are SNorm, while the unorm ones aren't.

Example:

B8G8R8A8Unorm
B8G8R8A8SNorm

I propose changing them to:

B8G8R8A8Unorm
B8G8R8A8Snorm

So basically, changing SNorm to Snorm.
For consistency. I hope it's not too late to do this kind of breaking change on the library.

@gdkchan gdkchan added the enhancement New feature or request label Mar 17, 2021
@Perksey
Copy link
Member

Perksey commented Mar 17, 2021

This depends on how they're defined upstream. We're fine with upstream-induced breaking changes such as these, but if how we're parsing it is consistent with upstream then it'd be much harder to justify such a break.

Will investigate and get back to you.

@Perksey
Copy link
Member

Perksey commented Mar 17, 2021

Ok so it looks like we actually special-case SNorm here. @HurricanKai do you think it's worth a break in 2.x? Also, should we discuss with the working group as this will affect OpenGL too?

@HurricanKai
Copy link
Member

HurricanKai commented Mar 17, 2021

Can't we just add the Snorm in 2.next and mark the SNorm as obsolete (and hide it from intellisense?), then remove the SNorm in 2.(next + 2-5)?

@Perksey
Copy link
Member

Perksey commented Mar 17, 2021

That would be a very involved change, the current BuildTools pipeline doesn't really suit token overloading like that.

@HurricanKai
Copy link
Member

Don't you think for this case we can just make that change manually? Otherwise I'd be unhappy to break, since SNorm/Unorm are basically everywhere in Vulkan code.

@Perksey
Copy link
Member

Perksey commented Mar 17, 2021

We can just save this for 3.0, adding a lot of code just for this issue into BuildTools doesn't seem worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Vulkan enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

3 participants