-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
base: dev
Are you sure you want to change the base?
add "Forbid too small crop region" option #16602
Conversation
522a1c2
to
11971e8
Compare
I've compacted the code |
@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 |
I double check and please show they are equivalent you have three if condition blocks nested 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")
hmm, I feel the opposit, min max seems more readable to me if value > max_allowed
value = max_allowed value = min(value, max_allowed) |
summary 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 |
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 |
87b7ed9
to
2cb3c2d
Compare
I've reverted. Sorry, but I cannot approve it |
Maybe @catboxanon as the second maintainer can resolve our technical conflict? |
sure that's some other people make the call |
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: