-
-
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 2 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,20 @@ 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. | ||
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 +1127,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 +1750,19 @@ pub enum TranscodeFormat { | |
Rgb8, | ||
} | ||
|
||
/// An error that occurs when reinterpreting the image. | ||
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. s/the/a This should mention the functions that might return it. 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 am not familiar with what "s/the/a" means. 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. "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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry about that, bad habit of mine. Should've made a suggestion instead.
This would be grammatically correct. |
||
#[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 |
---|---|---|
|
@@ -157,7 +157,9 @@ fn asset_loaded( | |
// NOTE: PNGs do not have any metadata that could indicate they contain a cubemap texture, | ||
// so they appear as one texture. The following code reconfigures the texture as necessary. | ||
if image.texture_descriptor.array_layer_count() == 1 { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It behaves the same way as before. Thought about adding a comment. Good call on using expect - let me know what you think |
||
image.texture_view_descriptor = Some(TextureViewDescriptor { | ||
dimension: Some(TextureViewDimension::Cube), | ||
..default() | ||
|
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.