-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Texture format testing #8653
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
base: trunk
Are you sure you want to change the base?
Texture format testing #8653
Conversation
| /// [sRGB transfer function]: https://en.wikipedia.org/wiki/SRGB#Transfer_function_(%22gamma%22) | ||
| #[repr(C)] | ||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)] | ||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, strum::EnumIter)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: We shouldn't expose EnumIter in our public API; we've never done so before, and I think we should discuss as a group before we actually do it. In the case of EnumIter, it's usually better to make a façade around it that returns impl Iterator.
issue: Even if we do have such an opaque API, I'm not sure if it makes sense to expose an exhaustive iterator for it anyway, except for, say, an unstable-testing feature.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not at all attached to my current way of doing things, this is just quick and dirty so I could set up the tests. I do very much think that we want the iteration to be automatically generated though, so that someone can't forget to add a format down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of an extra feature here for testing, but ultimately I think the decision should be made by people more experienced with programming and with this codebase. If that makes sense to you I'm happy to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is #8438
| /// ASTC RGBA channel | ||
| #[repr(C)] | ||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)] | ||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(blocking): Why do we need to derive Default here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used as a field in the Astc texture format enum variant. In order to enumerate each variant once strum needs this.
ErichDonGubler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to Move Fast and Cover Things™ for tests, but the API changes seem like something we may not want to (stably) expose for downstreams. CC @gfx-rs/wgpu for thoughts on that specifically.
Connections
Closes #8646
CI Failures mostly represent wgpu bugs, not issues with this PR
Description
Adds some testing for texture formats to make sure that what a device claims to support it actually supports
Testing
Is test
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.