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 "Forbid too small crop region" option #16602

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

light-and-ray
Copy link
Contributor

@light-and-ray light-and-ray commented Oct 28, 2024

Description

Expands crop region if it is smaller then processing resolution. This can happen for example if was selected 60px mask, padding = 90, so crop region has 240px side. With this option it will be expanded to 512px to not lose "free" context which doesn't eat resolution. This behavior can be either desirable or not, so I've made an option

Copy of #16260 but disconnected with my other patch

Checklist:

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 30, 2024

I've compacted the code
should be functionally equivalent
can you double check

@light-and-ray
Copy link
Contributor Author

@w-e-w These 2 pieces of code look non-equal. x1 can be positive, but x2 can be bigger then image_width

            if c1 < 0:
                c2 = min(c2 - c1, image_dimension)
                c1 = 0
        if x1 < 0:
            x2 -= x1
            x1 -= x1
        if x2 >= image_width:
            x2 = image_width

Also I dislike this minimization in general. Looks very unreadable and non-intuitive

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 31, 2024

These 2 pieces of code look non-equal. x1 can be positive, but x2 can be bigger then image_width

I double check and please show they are equivalent

you have three if condition blocks
in the 1st if block you already made it so that x2 is at maximum the dimension of the image
the 3rd if block is a repeat of the same check as the 1st if block, since it's the same it will never trigger if the 2nd if block never triggers (which modifies x2)
this means that 3rd if block can be nested inside the 2nd if block

nested 3rd if block into 2nd if block

        if x1 < 0:
            x2 -= x1
            if x2 >= image_width:
                x2 = image_width
            x1 -= x1

rewrite with min

        if x1 < 0:
            # x2 -= x1 
            # this is is same as x2 = x2 - x1

            # x2 = x2 - x1
            # if x2 >= image_width:
            #    x2 = image_width
            # this same as asking if x2 - x1 is smaller then image_widthm if not use image_width
            # which is the same as asking the minimum values of these two

            x2 = min(x2 - x1, image_width)

            x1 -= x1

so the final form

        if x1 < 0:
            x2 = min(x2 - x1, image_width)
            x1 -= x1

two double check I also wrote simple a test of all combination of arguments from 0~11 and it passes the test

test code

from itertools import product
from tqdm import tqdm


def expand_too_small_crop_region_2(crop_region, processing_width, processing_height, image_width, image_height):
    def _expand(c1, c2, processing_dimension, image_dimension):
        if (diff := processing_dimension - (c2 - c1)) > 0:
            diff_w_l = diff // 2
            c1 -= diff_w_l
            c2 += diff - diff_w_l
            if c2 >= image_dimension:
                c1 -= c2 - image_dimension
                c2 = image_dimension
            if c1 < 0:
                c2 = min(c2 - c1, image_dimension)
                c1 = 0
        return c1, c2

    x1, y1, x2, y2 = crop_region
    x1, x2 = _expand(x1, x2, processing_width, image_width)
    y1, y2 = _expand(y1, y2, processing_height, image_height)
    new_crop_region = x1, y1, x2, y2
    return new_crop_region


def expand_too_small_crop_region_1(crop_region, processing_width, processing_height, image_width, image_height):
    x1, y1, x2, y2 = crop_region
    desired_w = processing_width
    diff_w = desired_w - (x2 - x1)
    if diff_w > 0:
        diff_w_l = diff_w // 2
        diff_w_r = diff_w - diff_w_l
        x1 -= diff_w_l
        x2 += diff_w_r
        if x2 >= image_width:
            diff = x2 - image_width
            x2 -= diff
            x1 -= diff
        if x1 < 0:
            x2 -= x1
            x1 -= x1
        if x2 >= image_width:
            x2 = image_width
    desired_h = processing_height
    diff_h = desired_h - (y2 - y1)
    if diff_h > 0:
        diff_h_u = diff_h // 2
        diff_h_d = diff_h - diff_h_u
        y1 -= diff_h_u
        y2 += diff_h_d
        if y2 >= image_height:
            diff = y2 - image_height
            y2 -= diff
            y1 -= diff
        if y1 < 0:
            y2 -= y1
            y1 -= y1
        if y2 >= image_height:
            y2 = image_height
    return x1, y1, x2, y2


if __name__ == '__main__':
    test_range = 12
    for x1, x2, y1, y2, processing_width, processing_height, img_w, img_h in  tqdm(product(
            range(test_range), range(test_range), range(test_range), range(test_range),
            range(test_range), range(test_range), range(test_range), range(test_range),
    ), total=test_range ** 8):
        crop_region = (x1, y1, x2, y2)
        new_crop_region_1 = expand_too_small_crop_region_1(crop_region, processing_width, processing_height, img_w, img_h)
        new_crop_region_2 = expand_too_small_crop_region_2(crop_region, processing_width, processing_height, img_w, img_h)
        if new_crop_region_1 != new_crop_region_2:
            print(f'Error: {crop_region}, {processing_width}, {processing_height}, {img_w}, {img_h}\nnew_crop_region_1: {new_crop_region_1}\nnew_crop_region_2: {new_crop_region_2}')
            break
    else:
        print("All good")


Also I dislike this minimization in general. Looks very unreadable and non-intuitive

hmm, I feel the opposit, min max seems more readable to me

if value > max_allowed
    value = max_allowed
value = min(value, max_allowed)

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 31, 2024

summary
this is your orignal logic

if crop_width is smaller the process_width
    extends the crop_width in both directions equally until it matches the process_width

    if the right edge of extend_crop_width is now out of bounds (greate then image width)
        shifts the extend_crop_width to the left so that is within image width

    if the left edge of extend_crop_width is now out of bounds (small then 0)
        shifts the extend_rop_width to the righ so that is within image width

    # make sure the right edge is still in bounds
    if the right edge of extend_crop_width is now out of bounds (greate then image width)
        shrink down the region so that it's within bounds
        # this will make the region smaller the process_width but it's already as large as it can be

the thing I did with the min() is just

    if the left edge of extend_crop_width is now out of bounds (small then 0)
        shifts the extend_rop_width to the righ so that is within image width, but don't go out of bounds

@light-and-ray
Copy link
Contributor Author

light-and-ray commented Oct 31, 2024

But why you want this code, if to understand it you need to write 2 pages of pseudo-code and tests? Just revert it, and it's all

I can't understand what is a point of it. This generalization is a very bad pattern of programming I think

@light-and-ray
Copy link
Contributor Author

light-and-ray commented Oct 31, 2024

I've reverted. Sorry, but I cannot approve it

@light-and-ray
Copy link
Contributor Author

Maybe @catboxanon as the second maintainer can resolve our technical conflict?

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 31, 2024

sure that's some other people make the call

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.

2 participants