stable-diffusion-webui-extensions icon indicating copy to clipboard operation
stable-diffusion-webui-extensions copied to clipboard

Added Detail Daemon

Open muerrilla opened this issue 1 year ago • 8 comments

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.

muerrilla avatar May 13 '24 16:05 muerrilla

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

  • https://github.com/muerrilla/sd-webui-detail-daemon/pull/1

w-e-w avatar May 14 '24 10:05 w-e-w

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

w-e-w avatar May 14 '24 10:05 w-e-w

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.

muerrilla avatar May 14 '24 16:05 muerrilla

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

w-e-w avatar May 14 '24 17:05 w-e-w

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

w-e-w avatar May 16 '24 03:05 w-e-w

@muerrilla hello?

w-e-w avatar May 17 '24 07:05 w-e-w

@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.

muerrilla avatar May 18 '24 13:05 muerrilla

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 avatar May 18 '24 15:05 w-e-w

any updates?

w-e-w avatar Jun 22 '24 00:06 w-e-w

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.

muerrilla avatar Jun 28 '24 20:06 muerrilla

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

  • https://github.com/muerrilla/sd-webui-detail-daemon/pull/4

w-e-w avatar Jul 01 '24 13:07 w-e-w

I change the tag removed editing doesn't seem to fit https://github.com/AUTOMATIC1111/stable-diffusion-webui-extensions/blob/98a63bc927b0b080a58e17d1662275985333dde1/tags.json#L12-L13 denoising and scheduler manipulation is squarely in stable diffusion operation so manipulation tag

w-e-w avatar Jul 01 '24 13:07 w-e-w

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

https://github.com/AUTOMATIC1111/stable-diffusion-webui-extensions/blob/98a63bc927b0b080a58e17d1662275985333dde1/tags.json#L12-L13

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

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

muerrilla avatar Jul 01 '24 18:07 muerrilla