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

Feature: methods for getting a pixel using signed coordinates #1851

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions src/image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,65 @@ pub trait GenericImageView {
self.get_pixel(x, y)
}

/// Returns the pixel located at (x, y) if the location is [`in_bounds`],
/// otherwise `None` is returned.
///
/// Location coordinates can be negative.
///
/// # Safety
///
/// The [`in_bounds`] check must work as expected.
///
/// [`in_bounds`]: #method.in_bounds
fn checked_get_pixel(&self, x: i64, y: i64) -> Option<Self::Pixel> {
if x.is_negative() || y.is_negative() {
None
} else {
match (u32::try_from(x), u32::try_from(y)) {
(Ok(x), Ok(y)) => {
if self.in_bounds(x, y) {
Some(unsafe { self.unsafe_get_pixel(x, y) })
} else {
None
}
},
_ => None
}
}
}

/// Returns the pixel located at (x, y) if the location is [`in_bounds`],
/// otherwise the nearest bound pixel is returned.
///
/// Location coordinates can be negative.
///
/// [`in_bounds`]: #method.in_bounds
fn saturating_get_pixel(&self, x: i64, y: i64) -> Self::Pixel {
#[inline(always)]
fn convert(value: i64, bound: u32) -> u32 {
value.is_positive()
.then(|| {
match u32::try_from(value) {
Ok(value) => {
if value < bound {
value
} else {
bound - 1
}
},
Err(_) => bound,
}
}).unwrap_or(0u32)
}

unsafe {
self.unsafe_get_pixel(
convert(x, self.width()),
convert(y, self.height())
)
}
}
Copy link
Member

@HeroicKatora HeroicKatora Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth double checking that zero-sized images are either prohibited by GenericImage or properly handled by this patch

They are not. Panic in those cases would be permissible behavior but maybe it's not desirable. The methodical evaluation of how we wrote the trait is, however:

Safety

  • The coordinates must be in_bounds of the image.

Note that in_bounds is both safe, and the trait itself is safe. Thus, we can't make any assumption about its implementation other than the method signature. Since we haven't called the method here either, we can not argue to have fulfilled the precondition properly. I don't think this method possible in a generic, sound manner either way.

However, we can add specialized implementations of these two new methods for ImageBuffer. Since the in_bounds method is local, we can use its implementation details and rely on them being correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to get_pixel_unchecked. Note that the generic implementation provided by the trait does not in fact call any unsafe method, it defaults to a purely safe fallback. This is correct and intended; since there's nothing unsafe about the trait, the implementor has no obligations to 'do the right thing'. There's no inherent property of in_bounds that the generic implementation could rely on for any effect; so calling the safe fallback is indeed the best useful implementation we can provide.

Note to self: ImageBuffer is still dubious if the user chooses a non-default container. Damn.


/// Returns an Iterator over the pixels of this image.
/// The iterator yields the coordinates of each pixel
/// along with their value
Expand Down Expand Up @@ -1447,6 +1506,51 @@ mod tests {
source.view(2, 2, 0, 0);
}

#[test]
fn test_checked_get_pixel() {
let source = ImageBuffer::from_pixel(3, 3, Rgba([255u8, 0, 0, 255]));

assert!(source.checked_get_pixel(0, 0).is_some());
assert!(source.checked_get_pixel(2, 2).is_some());
assert!(source.checked_get_pixel(0, 2).is_some());
assert!(source.checked_get_pixel(2, 0).is_some());

assert!(source.checked_get_pixel(-1, -1).is_none());
assert!(source.checked_get_pixel(3, 3).is_none());
assert!(source.checked_get_pixel(0, 3).is_none());
assert!(source.checked_get_pixel(3, 0).is_none());
assert!(source.checked_get_pixel(-1, 0).is_none());
assert!(source.checked_get_pixel(0, -1).is_none());
}

#[test]
fn test_saturating_get_pixel() {
let source = GrayImage::from_vec(3, 3, vec![
0u8, 1u8, 2u8,
3u8, 4u8, 5u8,
6u8, 7u8, 8u8,
]).unwrap();

for x in 0..source.width() {
for y in 0..source.height() {
assert_eq!(source.saturating_get_pixel(x as i64, y as i64).to_owned(), *source.get_pixel(x, y));
}
}

assert_eq!(source.saturating_get_pixel(-1, -1).to_owned(), *source.get_pixel(0, 0));
assert_eq!(source.saturating_get_pixel(-1, 0).to_owned(), *source.get_pixel(0, 0));
assert_eq!(source.saturating_get_pixel(0, -1).to_owned(), *source.get_pixel(0, 0));
assert_eq!(source.saturating_get_pixel(3, 3).to_owned(), *source.get_pixel(2, 2));
assert_eq!(source.saturating_get_pixel(3, 2).to_owned(), *source.get_pixel(2, 2));
assert_eq!(source.saturating_get_pixel(2, 3).to_owned(), *source.get_pixel(2, 2));
assert_eq!(source.saturating_get_pixel(3, -1).to_owned(), *source.get_pixel(2, 0));
assert_eq!(source.saturating_get_pixel(2, -1).to_owned(), *source.get_pixel(2, 0));
assert_eq!(source.saturating_get_pixel(3, 0).to_owned(), *source.get_pixel(2, 0));
assert_eq!(source.saturating_get_pixel(-1, 2).to_owned(), *source.get_pixel(0, 2));
assert_eq!(source.saturating_get_pixel(-1, 3).to_owned(), *source.get_pixel(0, 2));
assert_eq!(source.saturating_get_pixel(0, 3).to_owned(), *source.get_pixel(0, 2));
}

#[test]
fn test_copy_sub_image() {
let source = ImageBuffer::from_pixel(3, 3, Rgba([255u8, 0, 0, 255]));
Expand Down