Open light-and-ray opened 3 weeks ago
I've compacted the code should be functionally equivalent can you double check
@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
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
```py 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)
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
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
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 https://github.com/AUTOMATIC1111/stable-diffusion-webui/pull/16260 but disconnected with my other patch
Checklist: