Rename Image.get_rect() to Image.copy_rect()

[alazifk]

Describe the project you are working on

Image Editing Software

Describe the problem or limitation you are having in your project

IMHO Image.get_rect() could be named better. It sounds like a getter for a Rect2i with the size of the Image but it copies an Image out of the rect provided.
Image.get_rect() returns an Image.
Image.get_used_rect() returns a Rect2i.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

When you blend or blit an Image you need to provide the rect you want to use from the source image. Probably most often you want to use the full image, so calling Image.get_rect() here is an easy mistake to make.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Image copy_rect(rect: Rect2i) const instead of
Image get_rect(rect: Rect2i) const

If this enhancement will not be used often, can it be worked around with a few lines of script?

It is a minor issue, but a good time to improve it.

Is there a reason why this should be core and not an add-on in the asset library?

Self-explanatory

[Mickeon]

Would it be too verbose to call it get_copy_from_rect(), or get_cropped_image(), or get_crop... get_crop_from_rect? I totally agree with the sentiment of the proposal, but I personally feel like copy_rect doesn't properly imply that it returns the image. It'd be equally as vague as get_rect().

[kleonc]

Another option: get_subimage.

[alazifk]

I don't like get_cropped_image() because there is also Image.crop().
I think copy_rect is pretty clear in the context of calling it on an image.
image.copy_rect() copies a rectangular area out of an image. The only issue I have with get_rect() is that get is a magic word for getters (and we might need the name at a later point in time).

I'd prefer copy_subimage over get_subimage, because we are creating a new Image and copying data from the other one which I assume is a costly operation. It sounds somewhat like accessing a layer too me though.

Some alternatives:
image.cutout(rect)
image.copy_cutout(rect)
image.copy_area(rect)
image.clip(rect)

I still like image.copy_rect() best and would be fine with image.copy_subimage().

[Mickeon]

I feel like a rename is necessary, mostly because of the bigger picture. There's a lot of similar get_rect() in Godot (mostly inherited by CanvasItem) and they all return an actual Rect2 or Rect2i. It would make sense, for the typical user, to expect the same from an Image, by association.

What about get_cropped()or get_crop()? I feel "crop" has to be included because it really is the most common term, as well as the actual name of this operation. If not that, at least "get" has got to, to further imply this is a returned value, and is not the same as crop().

[kleonc]

I'd prefer copy_subimage over get_subimage, because we are creating a new Image and copying data from the other one which I assume is a costly operation. It sounds somewhat like accessing a layer too me though.

Note there are many methods across the engine named get_* which perform costly operations, e.g. Texture.get_data/Texture2D.get_image (3.x/4.0) or TileMap.get_used_cells. Meaning the "avoid get_ prefix when naming computation heavy methods" rule is not currently followed anyway. So, whether we like it or not, seems like currently get_subimage would be more consistent with the rest of the engine than copy_subimage.

I agree get_rect is a bad name and should be probably changed. To what? I don't have a clear favorite among the proposed renames.

[alazifk]

@Mickeon Sorry, I misremembered that image.crop() returns the image cropped to its get_used_rect(). Given that it does not I'm fine with any name. I think it's all within the realm of reasonable names. The only argument I see against anything involving crop is that Image.crop() allows sizes bigger than the source image, so it is not completely consistent. But I barely care, I am more interested in the function not being named get_rect than any specific name.

@kleonc There is no convention either way, so I'd say both is possible. I think that it is good practice to replace get prefixes when possible for costly operations in order to communicate clearly that some sort of computation is happening. I would definitely replace TileMap.get_used_cells() with something like TileMap.count_used_cells(), because the first makes it easy to assume that one is just grabbing a cached value, while the latter makes it clear that you have to cache the value yourself. I think that reduces user error.

[MikeFP]

I'd prefer copy_subimage over get_subimage, because we are creating a new Image and copying data from the other one which I assume is a costly operation. It sounds somewhat like accessing a layer too me though.

Some alternatives:
image.cutout(rect)
image.copy_cutout(rect)
image.copy_area(rect)
image.clip(rect)

I think image.clip(rect) makes the most sense, as it's not a getter, and does not allow for sizes bigger than the source image, as @alazifk said, making it sufficiently different from image.crop(). Plus, the term "clip" is often used in image editors including the size limitation anyway, so it would be more consistent with current name conventions.

[Mickeon]

"clip" and "crop" are almost synonyms in common speech. The attempt with this rename is to make sure there is as little ambiguity as possible, and clip() wouldn't achieve that, at least I believe.

[MikeFP]

"clip" and "crop" are almost synonyms in common speech. The attempt with this rename is to make sure there is as little ambiguity as possible, and clip() wouldn't achieve that, at least I believe.

You're right. In that case, in an attempt to make naming consistent with the current API, I think image.crop_subimage() would be a good fit. I think "sub_image" makes more sense than "area/rect/cutout" because the method does not in fact crop an area, but rather an image with limited size, which happens to need a rect argument.

Personally, I like "clip" more, so if it's ok to rename image.crop() to image.clip(), I'd prefer image.clip_subimage().

[alazifk]

IMHO crop is better, there is no real semantic difference to clip but it is the word commonly used for the operation. That image.crop() allows bigger sizes is not a deal breaker, either. For image.crop() it makes sense, e.g. when you want to resize the canvas of a painting. But for image.get_cropped_image() probably nobody needs this (at least I cannot think of a use case) and if somebody does it can always be added later.

[Mickeon]

Overall, it seems like the consensus is that a rename is necessary, but unsure of the new name. Although, I've seen get_subimage() mentioned a lot, so I may modify my PR accordingly.