Rename TextureFormat::is_srgb() to performs_srgb_encoding().#9758
Rename TextureFormat::is_srgb() to performs_srgb_encoding().#9758kpreid wants to merge 1 commit into
TextureFormat::is_srgb() to performs_srgb_encoding().#9758Conversation
This is intended to clarify the correct use of the method, and in particular, to communicate that it does *not* mean “this (surface) texture format stores sRGB data”, which may be true or false independently. Also rewrote the documentation of `add_srgb_suffix()` and `remove_srgb_suffix()`.
|
The formatting CI failure appears to be an existing issue. No idea why it hit this PR only. |
I've also hit this in my personal fork. I guess something changed in last few hours somewhere in the CI stack. |
|
Yeah our version of prettier isn't pinned. |
|
This is creyD/prettier_action#154 |
|
|
||
| /// Returns `true` for srgb formats. | ||
| /// Returns `true` for `*Srgb` formats: those which, when read, automatically apply sRGB | ||
| /// decoding, and when written, automatically apply sRGB encoding. |
There was a problem hiding this comment.
Is it worth noting that encoding/decoding only happened in shader read/write, not buffer copies?
There was a problem hiding this comment.
Yes, I think that would be a good addition, but I am not sure how to phrase it.
There was a problem hiding this comment.
How about
Returns `true` for `*Srgb` formats: those which, when read or written to as a sampled texture or a render target, automatically apply the sRGB transfer function.
| /// textures, by the [`SurfaceColorSpace`][crate::SurfaceColorSpace]). | ||
| #[must_use] | ||
| pub fn is_srgb(&self) -> bool { | ||
| pub fn performs_srgb_encoding(&self) -> bool { |
There was a problem hiding this comment.
Personally I dislike performs_srgb_encoding and prefer is_srgb:
- This method returns a bool so
is_srgbname is clearer.performs_srgb_encodingsounds like an operation was performed on the texture format, rather than checking it. - I think
is_srgbor "sRGB texture formats" is the correct term.
There was a problem hiding this comment.
I would be happy to call it is_-something given a good suggestion of what that name. However, I strongly disagree that is_srgb() is an adequate name. It has in the past greatly confused me and others about what role it has — resulting in writing incorrect code.
In particular, is_srgb() being true implies that one must not write sRGB encoded values into it (but rather linear values). That’s the opposite of what it sounds like it could mean. The whole API design is kind of confusing, in my opinion, but we should do the best we can to clarify it in the parts we have control over (wgpu-specific functions such as this one, and documentation).
There was a problem hiding this comment.
I think what causes confusion is when you use the *UnormSrgb format its naming does not clearly indicate what it does (though this name is the convention across all backends) - not when you use the is_srgb function.
There was a problem hiding this comment.
In particular, is_srgb() being true implies that one must not write sRGB encoded values into it (but rather linear values).
Actually this is not accurate. For queue.write_texture, srgb encoded values should be written. Typically srgb textures do store srgb encoded data.
There was a problem hiding this comment.
We talked about this at the maintainers meeting and we'd prefer something other than is_srgb(). How about applies_srgb_transfer_function or performs_srgb_conversion? We can also have a doc alias for is_srgb so it shows up in searches.
There was a problem hiding this comment.
has_srgb_suffix if we want the name to be neutral.
There was a problem hiding this comment.
has_srgb_suffix() seems like an acceptable name to me; it is not very informative, but it matches add_srgb_suffix() and remove_srgb_suffix(), which makes it clear how it is related to the rest.
I will wait for further input before revising this PR.
4 participants
Description
TextureFormat::is_srgb()toperforms_srgb_encoding().add_srgb_suffix()andremove_srgb_suffix().This is intended to clarify the correct use of
is_srgb(), and in particular, to communicate that it does not mean “this (surface) texture format stores sRGB data”, which may be true or false independently.Testing
Ran
cargo docand reviewed the changed documentation.Squash or Rebase?
Rebase
Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.