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

rename/deprecate block_size -> block_copy_size, improve docs #4647

Merged
merged 8 commits into from
Nov 13, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 7, 2023

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

Description
I keep seeing people getting confused about block_size, so I made its name more explicit and fixed the docs a bit (for instance the link in there was broken)

@Wumpf Wumpf requested a review from a team as a code owner November 7, 2023 09:17
@teoxoy
Copy link
Member

teoxoy commented Nov 7, 2023

texel_block_size has been renamed to texel_block_copy_footprint in the spec which is a lot clearer in what it means. Can we use that instead?

gpuweb/gpuweb#3922

@Wumpf
Copy link
Member Author

Wumpf commented Nov 7, 2023

Oh didn't notice, thank you! Let's go with that then and link to it ofc.
That's a pretty awkward name though imho. Also doesn't imply bytes -.-

@Wumpf
Copy link
Member Author

Wumpf commented Nov 7, 2023

actually.. linking it okay. But do we think anyone will ever find TextureFormat::block_copy_footprint if they're looking for a byte size of a texel?

@Wumpf
Copy link
Member Author

Wumpf commented Nov 7, 2023

Technically we are providing the informative value that is described in the spec as

The texel block memory cost of a GPUTextureFormat is the number of bytes needed to store one texel block. It is not fully defined for all formats. This value is informative and non-normative.

so from that pov we would need to call it block_memory_cost :/

@Wumpf
Copy link
Member Author

Wumpf commented Nov 7, 2023

moving (potential) discussion to Matrix

@teoxoy
Copy link
Member

teoxoy commented Nov 7, 2023

The texel block memory cost is only "informative and non-normative".
The texel block copy footprint is the useful value since it's needed to calculate some of the parameters of ImageDataLayout/ImageCopyBuffer.

@Wumpf Wumpf changed the title rename/deprecate block_size -> block_size_in_bytes, improve docs rename/deprecate block_size -> block_copy_size, improve docs Nov 11, 2023
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Teodor Tanasoaia <[email protected]>
@Wumpf Wumpf enabled auto-merge (squash) November 13, 2023 18:56
@Wumpf Wumpf merged commit f742051 into gfx-rs:trunk Nov 13, 2023
27 checks passed
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.

2 participants