bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Prefer `UVec2` when working with texture dimensions

Open CptPotato opened this issue 1 year ago • 5 comments

Objective

The physical width and height (pixels) of an image is always integers, but for GpuImage bevy currently stores them as Vec2 (f32). Switching to UVec2 makes this more consistent with the underlying texture data.

I'm not sure if this is worth the change in the surface level API. If not, feel free to close this PR.

Solution

  • Replace uses of Vec2 with UVec2 when referring to texture dimensions.
  • Use integer types for the texture atlas dimensions and sections.

Sprite::rect remains unchanged, so manually specifying a sub-pixel region of an image is still possible.


Changelog

  • GpuImage now stores its size as UVec2 instead of Vec2.
  • Texture atlases store their size and sections as UVec2 and URect respectively.
  • UiImageSize stores its size as UVec2.

Migration Guide

  • Change floating point types (Vec2, Rect) to their respective unsigned integer versions (UVec2, URect) when using GpuImage, TextureAtlasLayout, TextureAtlasBuilder, DynamicAtlasTextureBuilder or FontAtlas.

CptPotato avatar Feb 04 '24 14:02 CptPotato

Is it worth keeping Image::size_f32()?

https://github.com/bevyengine/bevy/blob/36804dc266fdd0c3aad6e99a7d9a6e87bb3a4d43/crates/bevy_render/src/texture/image.rs#L556-L560

width and height are integers anyways, so this is just a minor convenience over using .size().as_vec2().

CptPotato avatar Feb 04 '24 15:02 CptPotato

Going to wait on the URect changes; ping me (or add the Ready-For-Final-Review label) if that's done and it's been a while.

alice-i-cecile avatar Feb 05 '24 13:02 alice-i-cecile

I think I'm mostly done with my changes. Will rebase soon and probably finish it up this weekend (still have to test that I didn't break things).

Also, I left Sprite::rect unchanged as Vec2, because I think it's ok to allow users to specify sub-pixel areas if they want to.

CptPotato avatar Feb 08 '24 18:02 CptPotato

CI failures are real: looks like you need to fix up some examples.

alice-i-cecile avatar Feb 12 '24 15:02 alice-i-cecile

Yup, I also think #11783 will cause additional conflicts (even though they're trivial to solve). I'd wait for that PR, since I think it's a less controversial change and could be merged relatively quickly.

CptPotato avatar Feb 12 '24 16:02 CptPotato

Sorry for the longer than expected delay. I rebased onto main and verified that the 2D sprite examples have identical output compared to main. CI should also be good now.

CptPotato avatar Feb 25 '24 09:02 CptPotato