Return Result from image::resize #19380
Conversation
alice-i-cecile
added
A-Rendering
alice-i-cecile
added
M-Needs-Migration-Guide
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
| @@ -1505,6 +1510,12 @@ pub enum TextureAccessError { | |||
| WrongDimension, | |||
| } | |||
|
|
|||
| #[derive(Error, Debug)] | |||
| pub enum ResizeError { | |||
There was a problem hiding this comment.
Missing docs here and for the enum variant. Doesn't need to be fancy.
| @@ -173,7 +173,7 @@ pub fn update_viewport_render_target_size( | |||
| }; | |||
| let image = images.get_mut(image_handle).unwrap(); | |||
| if image.data.is_some() { | |||
| image.resize(size); | |||
| let _ = image.resize(size); | |||
There was a problem hiding this comment.
Comment here about why we're ignoring the Result please.
examples/2d/pixel_grid_snap.rs
Outdated
| @@ -108,7 +108,7 @@ fn setup_camera(mut commands: Commands, mut images: ResMut<Assets<Image>>) { | |||
| }; | |||
|
|
|||
| // Fill image.data with zeroes | |||
| canvas.resize(canvas_size); | |||
| let _ = canvas.resize(canvas_size); | |||
There was a problem hiding this comment.
I think this is probably better to bubble up.
There was a problem hiding this comment.
@alice-i-cecile How does bevy prefer to have errors bubble up at the system level? I see some other examples are simply hitting these with an .unwrap() to panic. Is that sufficient for the example folder?
For future knowledge, what's the go-to play in this scenario when we don't want the system to panic?
crates/bevy_image/src/image.rs
Outdated
| @@ -851,14 +851,19 @@ impl Image { | |||
|
|
|||
| /// Resizes the image to the new size, by removing information or appending 0 to the `data`. | |||
| /// Does not properly scale the contents of the image. | |||
| pub fn resize(&mut self, size: Extent3d) { | |||
| self.texture_descriptor.size = size; | |||
| /// This method does nothing when self.data is None. | |||
There was a problem hiding this comment.
| /// This method does nothing when self.data is None. | |
| /// This method does nothing and returns an error when `self.data` is `None`. |
| pub enum ResizeError { | ||
| /// Failed to resize an Image because it has no data. | ||
| #[error("unable to resize an image without data")] | ||
| ResizedWithoutData, |
There was a problem hiding this comment.
Nit nit:
ImageWithoutData ?
- we're already in the context of a
resize. resizedcould be interpreted as an action which occured during the error, but it failed.
| @@ -173,7 +173,8 @@ pub fn update_viewport_render_target_size( | |||
| }; | |||
| let image = images.get_mut(image_handle).unwrap(); | |||
There was a problem hiding this comment.
Can't leave a direct code suggestion because it covers a deleted line, but these should just be
let _ = images.get_mut(image_handle).unwrap().resize(size);
|
I kinda disagree with the issue in the first place: |
Hmm, that's a good point. Maybe we should just be updating the documentation instead? |
alice-i-cecile
added
S-Needs-Design
Objective
Return a Result when resizing an image so that we can return an Err when image.data is None.
Fixes #19376