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

Extras upscaler: option limit target resolution #15293

Conversation

light-and-ray
Copy link
Contributor

@light-and-ray light-and-ray commented Mar 17, 2024

Description

By request: #15237

Useful for batches where can be big images, which give OOM error

Screenshots/videos:

Screenshot_20240317_141657

Checklist:

@light-and-ray
Copy link
Contributor Author

@drdancm is it correct? Or I should add something else?

@AUTOMATIC1111
Copy link
Owner

I'd rather not overcomplicate the UI with an option that 99.99% of users won't need. At least make it a setting.

@light-and-ray
Copy link
Contributor Author

Done

@AUTOMATIC1111
Copy link
Owner

Well, that's not precisely what I had in mind. I wanted for the actual number to be set from settings, not for the checkbox for enabling/disabling, from the standpoint of making code less complex.

@light-and-ray
Copy link
Contributor Author

light-and-ray commented Mar 17, 2024

I think it's much useful to have it in ui, because it's not only about avoiding OOM. Maybe someone wants to process folder to get images at maximum 2048p for instance. It's a useful option

@AUTOMATIC1111
Copy link
Owner

And he will go into settings and set it to 2048 there. If he needs to change it often, he can put it into the toprow.

@light-and-ray
Copy link
Contributor Author

light-and-ray commented Mar 17, 2024

I want to have quick access to this, but don't want to see it in the quick settings.

Hmm, maybe I should extend extra options builtin extension to extras tab too?

@light-and-ray
Copy link
Contributor Author

light-and-ray commented Mar 17, 2024

But I still think it's a good enough option of the script, and we should have it inside UI. Who thinks it too?

@light-and-ray
Copy link
Contributor Author

@AUTOMATIC1111 You said: "99.99% of users won't need", so I think it's no matter either a slider is inside ui and checkbox in settings or a slider is inside settings

@AUTOMATIC1111
Copy link
Owner

Yes, my preference is the latter - a slider or, an input box, in settings.

@drdancm
Copy link

drdancm commented Mar 18, 2024

Looks good to me, however I think the explanation (when you hover) should make it clear that the max pixel resolution that this means the LONGEST SIDE. Probably what you have in mind, but making it super clear to people, would in my mind, make it impossible to miss-understand.

This is, in my opinion, an absolutely great improvement for anyone doing batch processing.
Thanks ! ! ! ! ! ! ! ! ! ! ! !

@drdancm
Copy link

drdancm commented Mar 18, 2024

Absolutely have this inside the UI, otherwise, people won't know about it. Users only look into SETTINGS, when something is missing. To new Users, the SETTINGS are pretty confusing. There are so many different settings, so many different names which are not intuitively obvious. Automatic 1111 is a fantastic and wonderful project, and although easier to use, and more powerful, than many similar programs, it is somewhat short on being easy to learn by someone who is not a programmer, and relies on a single click install in the first place.

@light-and-ray
Copy link
Contributor Author

@AUTOMATIC1111 ready to merge @drdancm approved

@AUTOMATIC1111
Copy link
Owner

Well, I'm glad that drdancm approves, but that's a really small sample size. Is this feature even useful outside of batch jobs?

@light-and-ray
Copy link
Contributor Author

light-and-ray commented Mar 19, 2024

Yes it is, but batch is the main. Why is it not enough?

@drdancm
Copy link

drdancm commented Mar 19, 2024

Batch jobs are quite important for any serious artists or photographer. In any case, what terrible harm does it cause you, or anyone who is not interested in using this feature, and cannot imagine why this would be extremely useful ?
After all, the batch feature is there, but without the target limit is is very inefficient.

@AUTOMATIC1111
Copy link
Owner

Here are my thoughts:

  1. changing arguments of the function to **args is not something I want to merge in.
  2. if this is only useful in batch mode, I'd rather have it as option in batch UI, without an option to disable it.

@light-and-ray
Copy link
Contributor Author

if this is only useful in batch mode

Not only

changing arguments of the function to **args is not something I want to merge in.

Why? It looks better then too long arguments

@AUTOMATIC1111 AUTOMATIC1111 merged commit 7f3ce06 into AUTOMATIC1111:dev Mar 31, 2024
3 checks passed
AUTOMATIC1111 added a commit that referenced this pull request Mar 31, 2024
@light-and-ray light-and-ray deleted the extras_upscaler_limit_target_resolution branch March 31, 2024 10:45
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.

None yet

3 participants