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.

Sign up for GitHub

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

alternative implantation of "Forbid too small crop region" #16617

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
+30 −0
Open

alternative implantation of "Forbid too small crop region" #16617

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11971e8
add "Forbid too small crop region" option
light-and-ray Oct 28, 2024
2cb3c2d
fix x2 >= image_width and y2 >= image_height
light-and-ray Jul 28, 2024
884f1dc
compact expand_too_small_crop_region
w-e-w Oct 30, 2024
b2bbc01
rename variables and print coordinates
w-e-w Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions modules/masking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,33 @@ def expand_crop_region(crop_region, processing_width, processing_height, image_w
return x1, y1, x2, y2


def expand_too_small_crop_region(crop_region, processing_width, processing_height, image_width, image_height):
"""expands a crop_region to not have dimensions smaller than processing_dimensions"""

def _expand_segment(c1, c2, processing_dimension, image_dimension):
Copy link
Contributor

@light-and-ray light-and-ray Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awful naming. They are not dimensions, they are lengths. And your function is _expand_segment, how segment can have processing and image. processing_dimension -> desirable_length, image_dimension -> maximal_coordinate can be better

Copy link
Collaborator Author

@w-e-w w-e-w Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not to happy with the names myself
as always bad at naming things

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that dimensions is used to refer to the collective of width and height
dimension is refers to any side
width and height are specific sides

the variable names is a bit strange length seem to be better
maximal_coordinate not too sure

"""expands the segment given by c1 c2 to the desired_dimension but not exceeding the boundaries of the image_dimension"""
if (diff := processing_dimension + c1 - c2) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:= is less readable

# if the region is smaller than processing_dimension, extend both sides equally
diff_l = diff // 2
c1 -= diff_l
c2 += diff - diff_l
if c1 < 0: # shift the region to the right by c1
c2 = min(c2 - c1, image_dimension) # ensure c2 is within image_dimension
c1 = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why = 0 and when c2 -= c1; c1 -= c1 is more readable. Also if is better then min in this case

Copy link
Collaborator Author

@w-e-w w-e-w Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why = 0

imo c1 = 0 you read and see that something is just 0
while c1 -= c1 you need to think about it for a sec
not important in the grand scheme of things

Also if is better then min in this case

can't agree, I think min an max makes code cleaner

Copy link
Contributor

@light-and-ray light-and-ray Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why = 0

imo c1 = 0 you read and see that something is just 0
while c1 -= c1 you need to think about it for a sec

Imo c1 -= c1 doesn't require any thoughts that it's 0, and without min function c2 -= c1 in pair with c1 -= c1 tells you that you're shifting the segment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not convinced if is more readable

def expand_too_small_crop_region(crop_region, processing_width, processing_height, image_width, image_height):
    """expands a crop_region to not have dimensions smaller than processing_dimensions"""

    def _expand_segment(c1, c2, desirable_length, maximal_coordinate):
        """expands the segment given by c1 c2 to the desired_dimension but not exceeding the boundaries of the maximal_coordinate"""
        diff = desirable_length + c1 - c2
        if diff > 0:
            # if the region is smaller than desirable_length, extend both sides equally
            diff_l = diff // 2
            c1 -= diff_l
            c2 += diff - diff_l
            if c1 < 0:  # shift the region to the right by c1
                c2 -= c1
                c1 = 0
                if c2 > maximal_coordinate:
                    c2 = maximal_coordinate
            elif c2 >= maximal_coordinate:  # shift the region to the left by (c2 - maximal_coordinate)
                c1 -= c2 - maximal_coordinate
                c2 = maximal_coordinate
                if c1 < 0:
                    c1 = 0
        return c1, c2

    x1, y1, x2, y2 = crop_region

elif c2 >= image_dimension: # shift the region to the left by (c2 - image_dimension)
c1 = max(c1 - c2 + image_dimension, 0) # ensure c1 is not below 0
c2 = image_dimension
w-e-w marked this conversation as resolved.
Show resolved Hide resolved
return c1, c2

x1, y1, x2, y2 = crop_region
x1, x2 = _expand_segment(x1, x2, processing_width, image_width)
y1, y2 = _expand_segment(y1, y2, processing_height, image_height)
new_crop_region = x1, y1, x2, y2
if new_crop_region != crop_region:
print(f"Crop region {crop_region} was smaller then process resolution and has been expanded to {new_crop_region}")
Copy link
Contributor

@light-and-ray light-and-ray Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this new data is useful in the console output. Crop region is an internal variable. For user could be more useful new padding value, but in this case it can be different for w and h. I thought about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest I wasn't considering if it was useful to the user
since you already added the print I just put my debug coordinate outputs also in ther
and didn't remove it afterwards

do you think is notified user about the adjustment of crop region even useful in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's useful because a person can not understand why their generation in the preview looks like padding is too big although they set it small, and so it can confuse them, so this message should be. But with no cropping region debugging info

return new_crop_region


def fill(image, mask):
"""fills masked regions with colors from image using blur. Not extremely effective."""

Expand Down
2 changes: 2 additions & 0 deletions modules/processing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,6 +1639,8 @@ def init(self, all_prompts, all_seeds, all_subseeds):
crop_region = masking.get_crop_region_v2(mask, self.inpaint_full_res_padding)
if crop_region:
crop_region = masking.expand_crop_region(crop_region, self.width, self.height, mask.width, mask.height)
if shared.opts.forbid_too_small_crop_region:
crop_region = masking.expand_too_small_crop_region(crop_region, self.width, self.height, mask.width, mask.height)
x1, y1, x2, y2 = crop_region
mask = mask.crop(crop_region)
image_mask = images.resize_image(2, mask, self.width, self.height)
Expand Down
1 change: 1 addition & 0 deletions modules/shared_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@
"return_mask_composite": OptionInfo(False, "For inpainting, include masked composite in results for web"),
"img2img_batch_show_results_limit": OptionInfo(32, "Show the first N batch img2img results in UI", gr.Slider, {"minimum": -1, "maximum": 1000, "step": 1}).info('0: disable, -1: show all images. Too many images can cause lag'),
"overlay_inpaint": OptionInfo(True, "Overlay original for inpaint").info("when inpainting, overlay the original image over the areas that weren't inpainted."),
"forbid_too_small_crop_region": OptionInfo(False, "Forbid too small crop region").info("Correct inpaint padding for only masked to avoid crop region sides less then processing resolution"),
}))

options_templates.update(options_section(('optimizations', "Optimizations", "sd"), {
Expand Down