Add `copy_sub_image()` based on `ptr::copy_nonoverlapping()`

[yoanlcq]

This is doable with the current API using row iterators, but it's slightly annoying. I'd like a single function that does this. My use case is building font atlases from indiviual bitmaps.

I figure this would (roughly) look like this:

impl<'a, Pixel: Copy> ImgRefMut<'a, Pixel> {
    pub fn copy_sub_image(&mut self, dst_left: usize, dst_top: usize, src: ImgRef<Pixel>, src_left: usize, src_top: usize, width: usize, height: usize) {
        self.sub_image_mut(dst_left, dst_top, width, height).copy_image(src.sub_image(src_left, src_top, width, height))
    }
    pub fn copy_image(&mut self, src: ImgRef<Pixel>) {
        assert_eq!(self.width(), src.width());
        assert_eq!(self.height(), src.height());
        // Then, use row iterators and `ptr::copy_nonoverlapping()` to perform the copy efficiently.
    }
}

(The API is inspired by GDI's BitBlt()).

[kornelski]

Yes, copying would be useful. However, I wouldn't trust any Win32 APIs as an example of a good design ;)

So two issues with this:

• There should be some better naming scheme, perhaps inspired by Rust's naming. There's clone_from_slice, extend, etc. Order of arguments should also be consistent, so should top/left be before or after src?

• copy_sub_image takes too many parameters. I think it's fine to omit src box, since src.sub_image does that already. Only take top/left position in the destination image, and copy whole src ImgRef there.

[yoanlcq]

I agree with point 2. As for point 1, I don't see what's wrong; The function uses "copynonoverlapping" so it should start with "copy", because that's what it does. Other names I could come up with are "replace" or "overwrite", and at the end there's either "image", "sub_image", "other", "region" or "pixels".

In fact I think we could "copy" (haha) what the image crate does; Namely, its copy_from function, which appears to correspond to what you want.

[kornelski]

There could be copy_from for T: Copy, and clone_from for T: Clone

[kornelski]

but I'm not sure where the location should go

copy_from(top, left, src) seems wrong, because it sounds like from top/left. copy_from(src, top, left) is better. Or copy_at(src, top, left) maybe?

[yoanlcq]

OK, what about :

• copy_rows_from(src, dst_left, dst_top) (notice how X goes before Y), for T: Copy.
• clone_rows_from(...) for T: Clone.

I've added "rows" in-between for these reasons :

• clone_from() sounds like "clone the whole object" but we want to clone the image's contents;
• The copy/clone is done using row iterators (in case of T: Copy, the implementation is, indeed, one ptr::copy_nonoverlapping per row)

Would that be OK?

[kornelski]

Interesting observation about clone_*_from. I agree, but "rows" makes me wonder what's special about them. Area? Fragment? Rect? Sub? sub_image?

I'm loving that the borrow checker will ensure copy_nonoverlapping is indeed non-overlapping.

[yoanlcq]

So how about copy_sub_image/clone_sub_image ? I kinda liked rows because that's what the implementation is necessarily supposed to do; In the existing API there's rows() but not cols(), and rightfully so; this is the expected memory layout. copy_[area/fragment/rect/sub] are inconsistent with existing names in the API, and look like they qualify the right-hand-side.

To me, the current candidates are (along with their clone couterpart, and in no particular order):

• copy_sub_image
• copy_from
• copy_rows_from
• copy_at

This kind of discussion could go on for a long time and at the end of the day the API will be very hard to misuse. So I prefer to let you have the final word on this. :)

Lastly, I guess using the same naming as a well-estabished crate such as image could be nice (the name would then be copy_from).

[kornelski]

Yea, clone|copy_sub_image is probably the best.