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

Change GpuImage::size from UVec2 to Extent3d #16815

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Noxmore
Copy link

@Noxmore Noxmore commented Dec 14, 2024

Objective

When preparing GpuImages, we currently discard the depth_or_array_layers of the Image's size by converting it into a UVec2.

Solution

Change GpuImage::size to Extent3d, and just pass that through when creating GpuImages.
Also copy the aspect_ratio, and size (now size_2d for disambiguation from the field) functions from Image to GpuImage for ease of use with 2D textures.
I originally copied all size-related functions (like width, and height), but i think they are unnecessary considering how visible the size field on GpuImage is compared to Image.

Testing

Tested via cargo r -p ci for everything except docs, when generating docs it keeps spitting out a ton of

error[E0554]: `#![feature]` may not be used on the stable release channel
 --> crates/bevy_dylib/src/lib.rs:1:21
  |
1 | #![cfg_attr(docsrs, feature(doc_auto_cfg))]
  | 

Not sure why this is happening, but it also happens without my changes, so it's almost certainly some strange issue specific to my machine.

Migration Guide

  • GpuImage::size is now an Extent3d. To easily get 2D size, use size_2d().

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 14, 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.

Very straightforward. I agree with discarding the width etc helpers :)

@JMS55
Copy link
Contributor

JMS55 commented Dec 14, 2024

To double check my understanding, Image's already used Extent3d for all their sizing stuff, and it was only GpuImage that was locked to 2d?

@Noxmore
Copy link
Author

Noxmore commented Dec 14, 2024

To double check my understanding, Image's already used Extent3d for all their sizing stuff, and it was only GpuImage that was locked to 2d?

Yep.
Though, while i was looking around, i found ManualTextureView to also use a UVec2 for size, but i don't know much about that, and i figured it was probably out of scope of this PR anyway.

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-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants