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

Add non-panicking get_pixel/mut methods to ImageBuffer #1500

Merged
merged 2 commits into from
Jul 15, 2021

Conversation

okaneco
Copy link
Contributor

@okaneco okaneco commented Jun 26, 2021

This relands part of #1229 which added methods to ImageBuffer that return
Option<&P> and Option<&mut P> from get_pixel[_mut]-like methods. It does
not include adding methods to the GenericImage and GenericImageView traits.

Naming is still a concern #1243.

@fintelia
Copy link
Contributor

fintelia commented Jun 26, 2021

If this is purely about getting the optimizer to produce better code, I wonder if we should instead do something like:

if x >= self.width { 
    return None;
}
let num_channels = <P as Pixel>::CHANNEL_COUNT as usize;
let i = (y as usize).saturating_mul(self.width as usize).saturating_add(x as usize);

self.data.get(i..i+num_channels)
    .map(|pixel_indices| <P as Pixel>::from_slice(pixel_indices))

This way we incorporate the invariant that width * height = data.len(), and avoid an extra check.

@okaneco
Copy link
Contributor Author

okaneco commented Jun 27, 2021

Thanks for that suggestion, I've incorporated that change.

I'm interested in better code generation but more so in providing methods that return Option or Result to users where possible instead of panicking.

@fintelia fintelia merged commit c865ae8 into image-rs:master Jul 15, 2021
@okaneco okaneco deleted the checked_pixel branch July 15, 2021 19:31
@okaneco okaneco mentioned this pull request Nov 1, 2021
22 tasks
@HeroicKatora HeroicKatora mentioned this pull request Feb 4, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants