Image::reinterpret_size and Image::reinterpret_stacked_2d_as_array now return a Result #20797
Conversation
crates/bevy_image/src/image.rs
Outdated
| /// An error that occurs when reinterpreting the image. | ||
| #[derive(Error, Debug)] | ||
| pub enum TextureReinterpretationError { | ||
| #[error("incompatible sizes: old = {old:?} new = {new:?}")] | ||
| IncompatibleSizes { old: Extent3d, new: Extent3d }, | ||
| #[error("must be a 2d image")] | ||
| WrongDimension, | ||
| #[error("must not already be a layered image")] | ||
| InvalidLayerCount, | ||
| #[error("can not evenly divide height = {height} by layers = {layers}")] | ||
| HeightNotDivisableByLayers { height: u32, layers: u32 }, | ||
| } | ||
|
|
There was a problem hiding this comment.
Need some help on naming.
IQuick143
added
A-Rendering
IQuick143
added
C-Refinement
|
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. |
examples/3d/skybox.rs
Outdated
| image.reinterpret_stacked_2d_as_array(image.height() / image.width()); | ||
| image | ||
| .reinterpret_stacked_2d_as_array(image.height() / image.width()) | ||
| .unwrap(); |
There was a problem hiding this comment.
This is a user-facing example. This should, at the minimum, use expect, and in the best case handle the error/bubble it up.
There was a problem hiding this comment.
It behaves the same way as before. Thought about adding a comment. Good call on using expect - let me know what you think
crates/bevy_image/src/image.rs
Outdated
| @@ -1741,6 +1750,19 @@ pub enum TranscodeFormat { | |||
| Rgb8, | |||
| } | |||
|
|
|||
| /// An error that occurs when reinterpreting the image. | |||
There was a problem hiding this comment.
s/the/a
This should mention the functions that might return it.
There was a problem hiding this comment.
I am not familiar with what "s/the/a" means.
I was looking at the other Errors in this file and this looks the same to me. The other ones do not mention the functions either. Do you have an example from elsewhere in bevy that I can base this on?
There was a problem hiding this comment.
"s/the/a" means subsitute (or search and replace) "the" by "a". It comes from sed or perl.
So it should be "a image" instead of "the image".
As a non-native speaker I would've used "an image" though, but that may be wrong.
There was a problem hiding this comment.
I am not familiar with what "s/the/a" means.
Sorry about that, bad habit of mine. Should've made a suggestion instead.
As a non-native speaker I would've used "an image" though, but that may be wrong.
This would be grammatically correct.
| self.texture_descriptor.size, | ||
| new_size | ||
| ); | ||
| /// Changes the `size` if the total number of data elements (pixels) remains the same. |
There was a problem hiding this comment.
The docs here should mention what errors it might return.
There was a problem hiding this comment.
Just for my understanding: This makes sense here, because it returns TextureReinterpretationError, but will only ever construct one variant. Correct?
I will add it.
| /// | ||
| /// # Panics | ||
| /// Panics if the texture is not 2D, has more than one layers or is not evenly dividable into | ||
| /// the `layers`. |
There was a problem hiding this comment.
These should be changed into docs on what errors it might return.
There was a problem hiding this comment.
I did not see this with other functions. Here it could return every enum member. Should I still add something to the comment here?
There was a problem hiding this comment.
We should be documenting errors and what cases might cause them, otherwise it may be difficult for consumers of these APIs to know how to handle them.
There was a problem hiding this comment.
Well this lead me down a rabbit hole. I just kept the original comment and changed to # Errors. Apparently this is sometimes a thing in rust and even has a clippy lint.
Not sure if this is used anywhere in bevy at the moment, but the documentation concerning errors seem inconsistent anyways. Let me know if this is good now.
KirmesBude
changed the title
Objective
Remove panics from image reinterpretation methods.
Solution
Introduce new errors and emit those instead of panicking.
Testing
Moved out of #20536