Overlay and replace with negative position

[AndreKR]

overlay() and replace() currently take u32 for x and y:

image/src/imageops/mod.rs

Lines 261 to 262 in 8ab2d70

	/// Replace the contents of an image at a given coordinate (x, y)
	pub fn replace<I, J>(bottom: &mut I, top: &J, x: u32, y: u32)

As such x and y cannot be negative. This means in effect that the overlaid image can only cross the right and bottom edge of the destination image but not the left and top edge. This is pretty surprising. I think it should either not be allowed to go out of bounds at all or - my much preferred solution - allow the offset to be negative, so I can place the overlaid image wherever I want, even if it sticks out at the left or top.

[HeroicKatora]

From an interface perspective, changing it to i32 means we can no longer deal with all possible images. While a top-left 'negative' overlay can be represented with a subimage overlaid at (0, 0). I would rather choose an interface that is total.

Alternatively, we could make the parameters more interchangeable by letting you define the new coordinates for both images. For example, placing at (-1, -1) is the same as placing at (0, 0) over an image at (1, 1). Then internally we could perform the correct choice of which sub-image to use for both arguments.

[AndreKR]

changing it to i32 means we can no longer deal with all possible images

I don't understand, do you mean images that are larger than an i32 but smaller than an u32?

Alternatively, we could make the parameters more interchangeable by letting you define the new coordinates for both images

This sounds interesting, what exact signature did you have in mind?

(In fact when I was looking through the available functions for one that would copy a block from one image to another I didn't consider overlay() at first because I was expecting one that has more parameters (source block position, block size, destination block position). I'm not saying that is a good interface, just that I was kind of expecting it from other languages.)

[AndreKR]

FWIW, here's my current code to work around this issue:

// Set up example data
let mut out = DynamicImage::new_rgb8(500, 500);
let tile = DynamicImage::new_rgb8(100, 100);
let (pos_x, pos_y): (i32, i32) = (-50, -50);

// The imageops functions don't like negative offsets, so when the tile is to be positioned
// partially outside the image (to the top or left), we have to cut it before.
let (partial_offset_x, partial_offset_y) = (-min(pos_x, 0) as u32, -min(pos_y, 0) as u32);
// But if a tile is completely outside the top or left edge of the output image we do nothing.
if partial_offset_x > tile.width() || partial_offset_y > tile.height() {
	continue;
let tile = tile.view(partial_offset_x, partial_offset_y, tile.width() - partial_offset_x, tile.height() - partial_offset_y);
image::imageops::replace(&mut out, &tile, max(pos_x, 0) as u32, max(pos_y, 0) as u32);

[HeroicKatora]

This sounds interesting, what exact signature did you have in mind?

First of all, yes, I do mean images between i32::MAX, u32::MAX, however unlikely that may seem to you. It'd be a pain to discover that our own methods can't interact with all images that we can ourselves create. I would suppose something along the lines:

pub fn replace<I, J>(bottom: &mut I, top: &J, virtual_bot: (u32, u32), top: (u32, u32))

Then if you want to use 'negative indices' you can call it as:

let base: u32 = i32::MAX as u32;
replace(&mut out, &tile, (base, base), (base - 50, base - 50));

while the case of purely positive numbers can choose to simplify as

let base: u32 = i32::MAX as u32;
replace(&mut out, &tile, (0, 0), (x, y));

That being said, I believe we can also choose to use i32 in this case. The maximum sized image is an edge case that we wish to support but still, it is an edge case. A bit more deliberation might be required but the function is independent of the 'origin' so we could also make it have signed arguments as well. That way, only edge cases need to care about the exact definition and be careful to use i32::MIN as a base point while everyone else can be oblivious and use more intuitive offsets.

pub fn replace<I, J>(bottom: &mut I, top: &J, virtual_bot: (i32, i32), top: (i32, i32))

// And internally transform it to the former form.
replace(&mut out, &tile, (0, 0), (-50, -50));

[AndreKR]

pub fn replace<I, J>(bottom: &mut I, top: &J, virtual_bot: (u32, u32), top: (u32, u32))

I don't like that signature at all. I read it three times to understand what the parameters mean and then a few minutes later I had to think again because it didn't stick.

Before we do that, I'd rather suggest having a separate function for "virtual canvas" operation, but...

It'd be a pain to discover that our own methods can't interact with all images that we can ourselves create.

That's a good point. If the decision has been made that the maximum supported image size is u32::MAX, then why not simply use i64 for these "virtual space" parameters? Sure, it wastes some space, but that's nothing compared with the memory requirements of an image. And also users would have to cast but as you can see from my workaround code, there are a lot of casts anyway. I think that this code is somewhat representative because with image manipulation there is always some coordinate calculation that can end up negative, static values are rather the exception.

[fintelia]

I don't think there's a ton of value in changing interfaces or adding more functions to handle these less common uses, when the functions themselves are so simple. If the existing replace function doesn't do quite what you want, it is quite straightforward to implement your own variant that does: the bulk of the function is just a nested for loop!

[HeroicKatora]

I kind of agree and disagree. Yes, it's simple to adapt but it's not trivial (there were two rounds of bug fixes for oob panics in the methods in the past, iirc, until we introduced a single function to calculate the loop iteration counts). And we arbitrarily allow one thing and not the other. Also I've seen this question/need pop up at least twice in the history and we're doing a major change so when if not now?

[AndreKR]

@fintelia It was kind of my point that "calculated coordinates that can end up outside the image bounds" is actually the more common use and "static coordinates that I know will never start outside the image bounds" is the less common use. So your argument goes both ways: If we have a convenient function that works with any coordinates and you want to get rid of casts or want better optimization, then you can copy the function and optimize it.

@HeroicKatora Yep, I also needed three attempts until my workaround code worked as desired. I didn't put the if in at first, leading to an overflow in the subtraction.

[fintelia]

Those are fair points. In that case, I'd lean towards just tweaking the replace function to have coordinates be i32's or i64's.

[HeroicKatora]

This makes me heavily lean towards i64, if we don't intend to have a single parameter as a separate type.

strct Location(pub i64, pub i64).

[fintelia]

If anyone wants to open a PR converting replace to take i64 arguments, I think that would be a good inclusion in the next major release