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

Prefer UVec2 when working with texture dimensions #11698

Merged

Conversation

CptPotato
Copy link
Contributor

@CptPotato CptPotato commented Feb 4, 2024

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
Copy link
Contributor Author

CptPotato commented Feb 4, 2024

Is it worth keeping Image::size_f32()?

/// Returns the size of a 2D image as f32.
#[inline]
pub fn size_f32(&self) -> Vec2 {
Vec2::new(self.width() as f32, self.height() as f32)
}

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

@CptPotato CptPotato force-pushed the prefer-u32-texture-dimensions branch 2 times, most recently from e225a8b to 80e61f8 Compare February 4, 2024 15:09
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 4, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This makes far more sense to me: if we're just converting them to integers at the end anyways we should store and accept these values as ints.

@@ -19,7 +19,7 @@ use bevy_utils::HashMap;
#[reflect(Debug)]
pub struct TextureAtlasLayout {
// TODO: add support to Uniforms derive to write dimensions and sprites to the same buffer
pub size: Vec2,
pub size: UVec2,
/// The specific areas of the atlas where each texture can be found
pub textures: Vec<Rect>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might make sense to turn this into an URect?

Copy link
Contributor Author

@CptPotato CptPotato Feb 5, 2024

Choose a reason for hiding this comment

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

Good point, I'll give it a try and see if this causes conflicts anywhere.

As a side question: Is it a valid use case to specify fractional portions for the textures in a texture atlas? With URect I don't think it would be possible anymore. TextureAtlasLayout::from_grid is a similar story, now that I look at it.

Copy link
Contributor

@Kanabenki Kanabenki Feb 5, 2024

Choose a reason for hiding this comment

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

I don't think sub-pixel indexing is a use case needed in the texture atlas code from a quick skimming of it, and if a user need for some reason to do some fractional indexing, they can still convert from UVec2 to Vec2. To be consistent some similar changes would also be needed in TextureAtlasBuilder, but that can be done in another PR 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the types in TextureAtlasLayout and TextureAtlasBuilder to use "u32" types (URect, etc.) as you suggested. Though, because of these changes I also had to update a bit more code than I would have like.

Interactions with guillotiere are also a bit odd because these types use i32 instead of u32. So theres a few as i32 here and there.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 5, 2024
@alice-i-cecile
Copy link
Member

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.

@CptPotato CptPotato force-pushed the prefer-u32-texture-dimensions branch from b838d2e to a81a2d1 Compare February 8, 2024 17:52
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 8, 2024
@CptPotato
Copy link
Contributor Author

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.

@alice-i-cecile
Copy link
Member

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

@CptPotato
Copy link
Contributor Author

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.

auto-merge was automatically disabled February 25, 2024 09:12

Head branch was pushed to by a user without write access

@CptPotato
Copy link
Contributor Author

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.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 25, 2024
Merged via the queue into bevyengine:main with commit a7be8a2 Feb 25, 2024
23 checks passed
@CptPotato CptPotato deleted the prefer-u32-texture-dimensions branch February 25, 2024 16:21
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# 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](https://docs.rs/wgpu/latest/wgpu/struct.Extent3d.html).

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`](https://github.com/bevyengine/bevy/blob/a81a2d1da31cc2eebcd9d431ed2b73a678ce61e3/crates/bevy_sprite/src/sprite.rs#L29)
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`.
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# 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](https://docs.rs/wgpu/latest/wgpu/struct.Extent3d.html).

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`](https://github.com/bevyengine/bevy/blob/a81a2d1da31cc2eebcd9d431ed2b73a678ce61e3/crates/bevy_sprite/src/sprite.rs#L29)
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`.
ChristopherBiscardi added a commit to ChristopherBiscardi/bevy_ecs_tilemap that referenced this pull request Jun 7, 2024
[11698](bevyengine/bevy#11698) changed `GpuImage::size` to `UVec2`.

Right above this, `Extent3d` does the same thing, so I'm taking a small leap and assuming can `as`.
@rparrett rparrett mentioned this pull request Jun 19, 2024
rparrett added a commit to StarArawn/bevy_ecs_tilemap that referenced this pull request Jul 5, 2024
* Update to 0.14.0-rc.2

* [12997](bevyengine/bevy#12997): rename `multi-threaded` to `multi_threaded`

* RenderAssets<Image> is now RenderAssets<GpuImage>

Implemented in [12827](bevyengine/bevy#12827)

* FloatOrd is now in bevy_math

implemented in [12732](bevyengine/bevy#12732)

* convert Transparent2d::dynamic_offset to extra_index

[12889](bevyengine/bevy#12889) Gpu Frustum Culling removed the dynamic_offset of Transparent2d and it became `extra_index` with the special value `PhaseItemExtraIndex::NONE`, which indicates the `None` that was here previously

* RenderPhase<Transparent2d> -> ViewSortedRenderPhases<Transparent2d>

[12453](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy#12453): Render phases are now binned or sorted.

Following the changes in the `mesh2d_manual` [example](https://github.com/bevyengine/bevy/blob/ecdd1624f302c5f71aaed95b0984cbbecf8880b7/examples/2d/mesh2d_manual.rs#L357-L358): use the `ViewSortedRenderPhases` resource.

* get_sub_app_mut is now an Option

in [9202](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy/pull/9202) SubApp access has changed

* GpuImage::size f32 -> u32 via UVec2

[11698](bevyengine/bevy#11698) changed `GpuImage::size` to `UVec2`.

Right above this, `Extent3d` does the same thing, so I'm taking a small leap and assuming can `as`.

* GpuMesh::primitive_topology -> key_bits/BaseMeshPipeline

[12791](bevyengine/bevy#12791) the `primitive_topology` field on `GpuMesh` was removed in favor of `key_bits` which can be constructed using `BaseMeshPipeline::from_primitive_topology`

* RenderChunk2d::prepare requires &mut MeshVertexBufferLayouts now

[12216](bevyengine/bevy#12216) introduced an argument `&mut MeshVertexBufferLayouts` to `get_mesh_vertex_buffer_layout`, which bevy_ecs_tilemap calls in `RenderChunk2d::prepare`

* into_linear_f32 -> color.0.linear().to_f32_array(),

[12163](bevyengine/bevy#12163) bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed.

LinearRgba is now its own type that can be accessed via [`linear()`](https://docs.rs/bevy/0.14.0-rc.2/bevy/color/enum.Color.html#method.linear) and then converted.

* Must specify type of VisibleEntities when accessing

[12582](bevyengine/bevy#12582) divided `VisibleEntities` into separate lists. So now we have to specify which kind of entity we want. I think we want the Mesh here, and I think we can get rid of the `.index` calls on Entity since Entity [already compares bits](https://docs.rs/bevy_ecs/0.14.0-rc.2/src/bevy_ecs/entity/mod.rs.html#173) for optimized codegen purposes. Waiting to do that until the other changes are in though so as to not change functionality until post-upgrade.

* app.world access is functions now

- [9202](bevyengine/bevy#9202) changed world access to functions. [relevent line](https://github.com/bevyengine/bevy/pull/9202/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR640)
- This also surfaced [12655](bevyengine/bevy#12655) which removed `Into<AssetId<T>>` for `Handle<T>`. using a reference or .id() is the solution here.

* We don't need `World::cell`, and it doesn't exist anymore

In [12551](bevyengine/bevy#12551) `WorldCell` was removed.

...but it turns out we don't need it or its replacement anyway.

* examples error out unless this bevy bug is addressed with these features being added

bevyengine/bevy#13728

* check_visibility is required for the entity that is renderable

As a result of [12582](bevyengine/bevy#12582) `check_visibility` must be implemented for the "renderable" tilemap entities. Doing this is trivial by taking advantage of the
existing `check_visibility` type arguments, which accept a [`QF: QueryFilter + 'static`](https://docs.rs/bevy/0.14.0-rc.2/bevy/render/view/fn.check_visibility.html).

The same `QueryFilter`` is used when checking `VisibleEntities`. I've chosen `With<TilemapRenderSettings` because presumably if the entity doesn't have a `TilemapRenderSettings` then it will not be rendering, but this could be as sophisticated or simple as we want.

For example `WithLight` is currently implemented as

```rust
pub type WithLight = Or<(With<PointLight>, With<SpotLight>, With<DirectionalLight>)>;
```

* view.view_proj -> view.clip_from_world

[13289](bevyengine/bevy#13489) introduced matrix naming changes, including `view_proj` which becomes `clip_from_world`

* color changes to make tests runnable

* clippy fix

* Update Cargo.toml

Co-authored-by: Rob Parrett <[email protected]>

* Update Cargo.toml

Co-authored-by: Rob Parrett <[email protected]>

* final clippy fixes

* Update Cargo.toml

Co-authored-by: Rob Parrett <[email protected]>

* Simplify async loading in ldtk/tiled helpers

See Bevy #12550

* remove second allow lint

* rc.3 bump

* bump version for major release

* remove unused features

---------

Co-authored-by: Rob Parrett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants