Skip to content

Added Detail Daemon#315

Merged
w-e-w merged 4 commits intoAUTOMATIC1111:extensionsfrom
muerrilla:extensions
Jul 2, 2024
Merged

Added Detail Daemon#315
w-e-w merged 4 commits intoAUTOMATIC1111:extensionsfrom
muerrilla:extensions

Conversation

@muerrilla
Copy link
Contributor

Info

Here's the repo.
And here's the original reddit discussion.

Checklist:

  • I have read the Readme.md
  • The description is written in English.
  • The index.json and extension_template.json have not been modified.
  • The entry is placed in the extensions directory with the .json file extension.

@w-e-w
Copy link
Collaborator

w-e-w commented May 14, 2024

tested seems to work without breaking issue

but some minore issue regarding the JavaScript and elem_id

and there is one thing that's better taking care of before it's listed on index and used by the masses

  • sensible default values
    at least from my brief experience with your extensions the default values are... not usable, it could be the 1.5 model I am testing with but, but for me it the default Amount of 0.5 is way to high, something like 0.1 is already a lot and images just end up garbled
    adjusting all the values may change things but regardless I think it makes more sense to give new users a more sensible default values for them to start with otherwise it would look bad for extension

from your readme

I'll write up some proper docs on how best to set the parameters, as soon as possible.

understandable but similar to above if users don't understand how to use this properly, it looks bad for extension

if a default values are more sensible this is less crucial, as users have a good starting position for them to experiment

the graph color changing based on the amount doesn't help either
I'm not sure if your intentions is if good values then would be green and bad values will be red
if this is your intention then that is not good enough and actually cause more confusion than it does help
I was just disabling the colors until you have a better grass of what is a good value


some minore issue UI JavaScript and elem_id

  • most extension accordions are collapsed by default you have it opened
    I suggested to collapse consistency sake

  • you have chosen to hardcoately active color, this is fine when you're using the default theme but it won't look as good if you're using a something else, it's best to use a color defined by the gradio theme so it changes based on the theme that the user is using

  • you have set a static elem_id for some of your components
    but since your extension exist both on the txt2img and img2img tab which means IDs will be duplicated (it cannot be)
    which means will automatically generate random IDs and causing your JavaScript stuff to break
    assuming that tab order is default you can see that your active toggle only works on txt2img tab and not img2img tab

txt2img img2img
image image

works on txt2img but not on img2img, color dose not match the custom theme

the current best practice is to use
scripts.Script.elem_id() to automatically generate prefixes for the element ID so that it doesn't conflict with elements of other extensions or different instances on different tabsor multiple instances across different tabs

  • there exist a custom element in web UI called InputAccordion this it the Accordion with a checkbox that is used by hires-fix refine and other and all the modules
    maybe instead of coding the color toggling active JavaScript you maded just use the InputAccordion
    when uses clicks InputAccordion it expands and the checkbox is automatically checked and disable when flapsed,
    note the checkbox can be manually toggle if the user wishes to, this allows it to be enable when collapsed
collasped expaned
image image

last this is just a personal opinion
personally I found the amount of DD_xxxxx: var in infotext is a bit annoying, it is possible to compact it into one dictionary

note there is nothing wrong with your current infotext, I just personally don't like it


I made a fix PR for some parts

@w-e-w
Copy link
Collaborator

w-e-w commented May 14, 2024

my main concern is sensible default values
if you don't mind this and want to release it to the masses
then I will merge it

@muerrilla
Copy link
Contributor Author

muerrilla commented May 14, 2024

Hey! Thanks for the in depth review. Most of your points are spot on. I think it's better if you withhold from merging it until my coming update.

sensible default values

Very true. Actually I also discovered that if the fix is only applied to the uncond latent, the results will be much more controllable and less destructive (especially for new users). So I'm gonna use that mode as default, and set better default parameters in the coming update.

I'll write up some proper docs on how best to set the parameters, as soon as possible.

I've already updated the readme, adding some thorough explanation of how the extension works. But actual (theory-free) examples that can work as quick tutorials for beginners will have to wait till I have some free time. So I'd say this is not very high priority, when the default values are fine as you also suggested.

the graph color changing based on the amount doesn't help either I'm not sure if your intentions

The graph is yellow at zero, then yellow to green when the mean of the adjustment amount in all steps is positive (detail will be increased) and yellow to red when it's negative (detail will be decreased). But perhaps using a pair of colors with less existing semantic baggage (green = good, red = bad) is a better idea.

  • most extension accordions are collapsed by default you have it opened

Oops! This was intended for dev stage. Forget to turn it back to closed by default.

  • you have chosen to hardcoately active color

Good point, will fix.

  • you have set a static elem_id for some of your components...

I have already fixed this in the coming update by using
elem_classes instead, so the duplicates can be handled correctly.

the current best practice is to use scripts.Script.elem_id()

Oooh I didn't know that exists. Awesome. So the way to do it is elem_id=self.elem_id("dd-accordion") ?

  • there exist a custom element in web UI called InputAccordion

This is nice and handy, but I prefer the UX to be consistent with other extensions (ControlNet, FreeU, etc. all do the "expand accordion then check enable" thing). I also need to keep the color toggling to keep it consistent with my other personal extensions and theme. But I'll make the color theme-based.

last this is just a personal opinion personally I found the amount of DD_xxxxx: var in infotext is a bit annoying, it is possible to compact it into one dictionary

I totally agree. [damn! I just saw this 👇]

I made a fix PR for some parts

Oh man! Is this why the say you should always read the whole thing before you hit reply? What a pleasant surprise! Let me check it out.

@w-e-w
Copy link
Collaborator

w-e-w commented May 14, 2024

set to draft for now
message me when you think this extension is ready for the masses

@w-e-w w-e-w marked this pull request as draft May 14, 2024 17:11
@w-e-w
Copy link
Collaborator

w-e-w commented May 16, 2024

@muerrilla
do you think it's ready to be listed?

@w-e-w
Copy link
Collaborator

w-e-w commented May 17, 2024

@muerrilla hello?

@muerrilla
Copy link
Contributor Author

@muerrilla hello?

Hey there! Sorry, the day job suddenly got very busy. Let's hold it a few days till I can pin down the best default values, and complete the readme (hopefully by th end of the weekend) and then we're good to go.

@w-e-w
Copy link
Collaborator

w-e-w commented May 18, 2024

ahh ok
and no worries no pressure take your time

I was a bit confused and weren't certen that I should merge this
because I saw you change the default value back when we are trying to solve the other issues
and since you didn't make any changes since I was worried that we had a miscommunication that you think you already said it's good to go but I thought it wasn't

@w-e-w w-e-w mentioned this pull request May 24, 2024
4 tasks
@w-e-w
Copy link
Collaborator

w-e-w commented Jun 22, 2024

any updates?

@muerrilla
Copy link
Contributor Author

any updates?

Hey there! Sorry for disappearing again and keeping you waiting on this. I changed the default values a bit. Did tons of testing but couldn't come up with the perfect values. 😆 Also changed the both mode a bit (it depends on the cfg scale now) so it breaks compatibility with the previous versions. I was planning on adding some cfg rescale functionality to it before release, since it seems to be essential for fixing the burning/wash-out effect, but couldn't find the free time to do it.

So I'd say it's good enough to be listed as is. I'll add those features (and better docs!) when I can. Thanks.

@w-e-w
Copy link
Collaborator

w-e-w commented Jul 1, 2024

@muerrilla
haha found more major bug
hires fix enabled + batch count > 1
hopefully the last

@w-e-w w-e-w marked this pull request as ready for review July 1, 2024 13:33
@w-e-w
Copy link
Collaborator

w-e-w commented Jul 1, 2024

I change the tag
removed editing
doesn't seem to fit

"editing": "an extension that changes images, not using stable diffusion.",
"manipulations": "an extension that changes images with stable diffusion.",

denoising and scheduler manipulation is squarely in stable diffusion operation so manipulation tag

@muerrilla
Copy link
Contributor Author

muerrilla commented Jul 1, 2024

I change the tag removed editing doesn't seem to fit

"editing": "an extension that changes images, not using stable diffusion.",
"manipulations": "an extension that changes images with stable diffusion.",

denoising and scheduler manipulation is squarely in stable diffusion operation so manipulation tag

oh! Guess I had missed the "not" in that sentence. 🤦‍♂️

@w-e-w w-e-w merged commit 3c49994 into AUTOMATIC1111:extensions Jul 2, 2024
github-actions bot pushed a commit that referenced this pull request Jul 2, 2024
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