-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Image::reinterpret_size and Image::reinterpret_stacked_2d_as_array now return a Result #20797
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
base: main
Are you sure you want to change the base?
Changes from all commits
3f2297a
b00a139
5357efd
831c413
9e73122
7b4a673
63de6b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1063,21 +1063,21 @@ impl Image { | |
} | ||
} | ||
|
||
/// Changes the `size`, asserting that the total number of data elements (pixels) remains the | ||
/// same. | ||
/// | ||
/// # Panics | ||
/// Panics if the `new_size` does not have the same volume as to old one. | ||
pub fn reinterpret_size(&mut self, new_size: Extent3d) { | ||
assert_eq!( | ||
new_size.volume(), | ||
self.texture_descriptor.size.volume(), | ||
"Incompatible sizes: old = {:?} new = {:?}", | ||
self.texture_descriptor.size, | ||
new_size | ||
); | ||
/// Changes the `size` if the total number of data elements (pixels) remains the same. | ||
/// If not, returns [`TextureReinterpretationError::IncompatibleSizes`]. | ||
pub fn reinterpret_size( | ||
&mut self, | ||
new_size: Extent3d, | ||
) -> Result<(), TextureReinterpretationError> { | ||
if new_size.volume() != self.texture_descriptor.size.volume() { | ||
return Err(TextureReinterpretationError::IncompatibleSizes { | ||
old: self.texture_descriptor.size, | ||
new: new_size, | ||
}); | ||
} | ||
|
||
self.texture_descriptor.size = new_size; | ||
Ok(()) | ||
} | ||
|
||
/// Resizes the image to the new size, keeping the pixel data intact, anchored at the top-left. | ||
|
@@ -1128,21 +1128,31 @@ impl Image { | |
/// Takes a 2D image containing vertically stacked images of the same size, and reinterprets | ||
/// it as a 2D array texture, where each of the stacked images becomes one layer of the | ||
/// array. This is primarily for use with the `texture2DArray` shader uniform type. | ||
/// | ||
/// # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should be changed into docs on what errors it might return. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not see this with other functions. Here it could return every enum member. Should I still add something to the comment here? |
||
pub fn reinterpret_stacked_2d_as_array(&mut self, layers: u32) { | ||
pub fn reinterpret_stacked_2d_as_array( | ||
&mut self, | ||
layers: u32, | ||
) -> Result<(), TextureReinterpretationError> { | ||
// Must be a stacked image, and the height must be divisible by layers. | ||
assert_eq!(self.texture_descriptor.dimension, TextureDimension::D2); | ||
assert_eq!(self.texture_descriptor.size.depth_or_array_layers, 1); | ||
assert_eq!(self.height() % layers, 0); | ||
if self.texture_descriptor.dimension != TextureDimension::D2 { | ||
return Err(TextureReinterpretationError::WrongDimension); | ||
} | ||
if self.texture_descriptor.size.depth_or_array_layers != 1 { | ||
return Err(TextureReinterpretationError::InvalidLayerCount); | ||
} | ||
if self.height() % layers != 0 { | ||
return Err(TextureReinterpretationError::HeightNotDivisibleByLayers { | ||
height: self.height(), | ||
layers, | ||
}); | ||
} | ||
|
||
self.reinterpret_size(Extent3d { | ||
width: self.width(), | ||
height: self.height() / layers, | ||
depth_or_array_layers: layers, | ||
}); | ||
})?; | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Convert a texture from a format to another. Only a few formats are | ||
|
@@ -1741,6 +1751,19 @@ pub enum TranscodeFormat { | |
Rgb8, | ||
} | ||
|
||
/// An error that occurs when reinterpreting an 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}")] | ||
HeightNotDivisibleByLayers { height: u32, layers: u32 }, | ||
} | ||
|
||
/// An error that occurs when accessing specific pixels in a texture. | ||
#[derive(Error, Debug)] | ||
pub enum TextureAccessError { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
title: "Image::reinterpret_size and Image::reinterpret_stacked_2d_as_array now return a Result" | ||
pull_requests: [20797] | ||
--- | ||
|
||
`Image::reinterpret_size` and `Image::reinterpret_stacked_2d_as_array` now return a `Result` instead of panicking. | ||
|
||
Previously, calling this method on image assets that did not conform to certain constraints could lead to runtime panics. The new return type makes the API safer and more explicit about the constraints. | ||
|
||
To migrate your code, you will need to handle the `Result` returned by `Image::reinterpret_size` or `Image::reinterpret_stacked_2d_as_array`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs here should mention what errors it might return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my understanding: This makes sense here, because it returns TextureReinterpretationError, but will only ever construct one variant. Correct?
I will add it.