vladmandic / automatic

SD.Next: Advanced Implementation of Stable Diffusion and other Diffusion-based generative image models
https://github.com/vladmandic/automatic
GNU Affero General Public License v3.0
5.56k stars 409 forks source link

[Extension]: sd_grid_add_image_number causes corrupt settings display #1392

Closed Woisek closed 1 year ago

Woisek commented 1 year ago

Issue Description

After todays update, the settings has cut off buttons after three entries. Zwischenablagebild (1)

What happened here? ๐Ÿ˜ณ

Version Platform Description

Already up to date. Using VENV: I:\Super SD 2.0\automatic\venv 10:53:58-056332 INFO Starting SD.Next 10:53:58-061343 INFO Python 3.10.6 on Windows 10:53:58-103232 INFO Version: 76e54938 Sat Jun 10 19:51:35 2023 -0400 10:53:58-479675 INFO Setting environment tuning 10:53:58-498609 INFO nVidia CUDA toolkit detected 10:53:59-741259 INFO Torch 2.0.0+cu118 10:53:59-758214 INFO Torch backend: nVidia CUDA 11.8 cuDNN 8700 10:53:59-761205 INFO Torch detected GPU: NVIDIA GeForce RTX 2070 SUPER VRAM 8192 Arch (7, 5) Cores 40

Acknowledgements

vladmandic commented 1 year ago

this seems to be theme specific. i've tried default black-orange and gradio/default and cannot reproduce. what is the theme you're using?

Woisek commented 1 year ago

Again, this is no "theme". It's an altered appearance stored in the user.css. I never changed anything regarding the display in the settings. And yes, it's like that in the original orange theme, too. Zwischenablagebild (2)

vladmandic commented 1 year ago

i cannot reproduce - do you have user.css?

Woisek commented 1 year ago

Again, this is no "theme". It's an altered appearance stored in the user.css. I never changed anything regarding the display in the settings. And yes, it's like that in the original orange theme, too.

The screenshot above was made without the user.css ...

vladmandic commented 1 year ago

I have to ask questions because clearly something is causing it and I cannot reproduce. I've tried Chrome and Firefox. What browser do you use?

Woisek commented 1 year ago

It's OK, ask away. ๐Ÿ˜Š FF 114 It must have came with the update this morning, I remember seeing the black-orange.css as modified. Unfortunately I've closed and restarted WebUI in the meantime, so I can not say it exactly ... ๐Ÿค”

Edit: the rule for #settings > div.tab-nav button has no width definition. If width: max-content is added, the button become visible. But they are still positioned too far to the left. 1 2

Gegell commented 1 year ago

Hmm does not seem to be due to the theme... There should be an additional modification indicator button generated for image options which seems to be missing (e.g. a button with id modification_indicator_saving-images). This causes everything to shift by 1 grid cell (this tab layout now uses a grid layout), which means the tab headers are moved into the narrow slots of the modification indicators, and vice versa.

This can occur due to 3 possible options:

  1. The button is not generated in the first place in ui.py
  2. The button is generated, but not moved correctly by the snippet in ui.js
  3. One of the extensions tries to remove a settings tab, but deletes the modification indicator instead.

Could you look whether such an element exists at all on the ui? (e.g. does any element with the id given above exist)

Woisek commented 1 year ago

If the question was on me ... yeah, I can see it. Zwischenablagebild (3) Zwischenablagebild (5)

Gegell commented 1 year ago

huh interesting... I assume the issue occurs due to the id appearing twice. Once for the base setting saving-images (which is in plain text the tab named "Image Options", and once for the second tab "Saving images/grids" which is probably added by an extension.

iirc gradio lets one generate invalid html, by having multiple elements with the same id. This in turn breaks the ui.js in unforseen ways, as it uses getElementById to get the modification indicator of the corresponding tab to add to the tab headers. I might be able to circumvent this issue by finding another way to identify these corresponding elements, if I can't rely on the id :/

Woisek commented 1 year ago

Thanks for looking into it. ๐Ÿ‘

vladmandic commented 1 year ago

Duplicate ids are definitely possible in Gradio, but styling should be on parent, not on each element, so it should not break. I wonder what's different that I cannot reproduce.

Gegell commented 1 year ago

not a styling issue. I add a modification indicator for each setting, and afterwards one for each tab in the python back-end, then shuffle it in the js onto the tab titles.

If there is now a setting with the same name as the tab key, we have 2 modification indicators with the same name. This then leads to definitively wrong (undefined?) behavior as the HTML then is invalid. In my case Firefox takes the first element with the corresponding id, which is the modification indicator of the setting instead of the tab. This element will then be placed in the tab headers as the indicator instead of the one which should actually appear there. (There seems to be one missing in the issue instead, but I chalked it up to undefined behavior... ๐Ÿคท )

I do think this should get fixed by removing the name duplication. At least weird behavior gets fixed, which is obtainable by adding a setting with the same name as a setting tab. E.g. in shared.py add something like:

 options_templates.update(options_section(('saving-images', "Image Options"), {
     ...
+    "saving-images": OptionInfo(True, "Save images"),
 }))
Gegell commented 1 year ago

So, I tried to fix (what I believe to be) the issue. Would be cool if you could confirm whether this is actually the case for you @Woisek. Here my fork with the proposed fix: https://github.com/Gegell/automatic/tree/settings-improvements

Woisek commented 1 year ago

Cool, but ... I should/can I test this ... ? ๐Ÿค”

Gegell commented 1 year ago

Yeah, depending on how much knowledge with git you have either git checkout to my branch (see https://stackoverflow.com/questions/9153598/how-do-i-fetch-a-branch-on-someone-elses-fork-on-github)

ooor, copy the following 2 files from my repository into the corresponding location in your installation:

Woisek commented 1 year ago

The second option is much more easier ... ๐Ÿ˜ I tell you when I had the chance.

vladmandic commented 1 year ago

let me know, i'd like to fix this asap and if suggested code works, i'll merge. only issue i see with the code is usage of ||= operator which in the following line will cause element insert multiple times.

Woisek commented 1 year ago

Ok ... better, but not very convincing ... ๐Ÿค”

Zwischenablagebild Zwischenablagebild (1)

vladmandic commented 1 year ago

i've double-checked and width of elements in left column is set in style.css via:

#settings > div.tab-nav {
    display: grid;
    grid-template-columns: repeat(auto-fill, .3em minmax(10em, 1fr));
    flex: 1 0 auto;
    width: 11em;
    align-self: flex-start;
    gap: var(--spacing-md);
}

and i still cannot reproduce. @Woisek just to confirm, can you start server with --safe so to rule out some extension messing with styling or creation of conflicting elements?

Woisek commented 1 year ago

@vladmandic OK, in safe mode it's back to normal. meaning, an extension is the culprit. The question is now ... what extension does change the UI for the settings ... ? ๐Ÿค” A "sort by: last installed" would be nice for such things and also in general.

vladmandic commented 1 year ago

yup, i can add that. but need to figure this out before i do that. you can either do the grunt work by looking up every style.css fetched in browser inspector -> network -> css only or you can disable all and enable one-by-one and tell me which one messes things up.

Woisek commented 1 year ago

A, good hint with the css, will look into that. ๐Ÿ‘

Woisek commented 1 year ago

Soo ... I know I'm not stupid ... ๐Ÿคช It has something to do with the black-orange theme.

Enabled: Zwischenablagebild

Disabled: Zwischenablagebild (1)

Woisek commented 1 year ago

Update: in black-orange.css, there is this rule:

body, button, input, select, textarea
{
    font-family: var(--font);
    overflow-x: hidden;
}

Removing the overflow-x makes the text visible, but it's still off position.

vladmandic commented 1 year ago

i'll remove the overflow tag, but we still need to find which extension is messing things up.

Woisek commented 1 year ago

Honestly, I don't think that an extension does this. I went through all loaded css files and switched every single one of them off and on. The black-orange.css was the only one that responded to this. I had some extensions in mind, that I installed lately and which I had problems with, like the redone presets utilities from Gershel, the new open pose editor and Photopea. Those disabled did nothing change. And other extensions don't have or need any css AFAIK. ๐Ÿค”

Gegell commented 1 year ago

Again, i do not believe that the underlying issue is a css error (or really fixable well by a change there, compare that your fix still shifts the elements to the left, which indicates that the tab header elements are in the wrong grid column) There seems to be some elements which are missing in the setting tabs for some reason, either because something is not selected properly, something is removed or added to that tab list. This is something that I only believe that an extension is responsible for, seeing as neither @vladmandic nor myself can truly seems to reproduce your UI state :/ Maybe post the list of your installed extensions & I can take a shot at trying to reproduce it...?

vladmandic commented 1 year ago

a) i cannot reproduce with base install b) i've just removed overflow c) you said yourself it looks fine with --safe, so that's pretty much in conflict with "i don't think an extension does this" d) you've switched loaded css files on/off? how? even if firefox has that option, that is pretty weird as i don't know how would that work on the fly. and it doesn't even have to be css, extension can make a ui mess if it really choose to.

@Woisek lets not play the blame game, lets do extensions from zero and then enable one-by-one, its going to be faster than discussing here.

Woisek commented 1 year ago

you've switched loaded css files on/off? how?

Via the developer tools, you see it in the screen shot I provided. You can switch styles on and off.

even if firefox has that option, that is pretty weird as i don't know how would that work on the fly.

CSS is an immediately action, how do you think that all those fancy eye candies work? That's not JS alone. ๐Ÿ˜Š Also that's nothing new, it has always worked that way.

lets not play the blame game,

I don't blame anyone. I just tell what I experienced. And it's a bit funny that all was working and this problem occured after I saw the black-orange.css changed after an update. Strangely enough, I can't see in the commits when the css has changed. As if it was not being recorded. But that would also be odd.

Woisek commented 1 year ago
  1. Round: All User extensions off, problem still exist
  2. Round: All build-in extensions turned off, problem still exists

With my user.css Zwischenablagebild (2)

Without my user.css Zwischenablagebild (3)

Without black-orange.css ... oh, wait ... ๐Ÿคจ Zwischenablagebild (4)

vladmandic commented 1 year ago

Without black-orange.css ... oh, wait ... ๐Ÿคจ

no need for sarcasm - they are both equally messed up.

and both screenshots show tons of extensions installed. and you said that with --safe there is no issue. so lets start there.

Woisek commented 1 year ago

no need for sarcasm - they are both equally messed up.

Well, I said multiple time on what to probably look at. And this is more than just a hint now.

and both screenshots show tons of extensions installed.

Installed of course, but like I wrote, I deactivated all extensions, as you can see in the navigation (that's why I left this portion in the screen shot). You don't really expect I de-install all the stuff ... ?!

and you said that with --safe there is no issue. so lets start there.

I don't know what --safe does, but it doesn't necessary have something to do with the extensions and also disables something internally. And what exactly should now start (again)? We started, I deactivated all extension as you suggested and the problem still exists. So, what else now?

Besides, if extensions can be active even when disabled, then 1, what sense does disabling make and 2. there is then something deeper going on in the WebUI structur, IMHO. As a bonus, if point 1 is true, how do you expect to do a debug with

lets do extensions from zero and then enable one-by-one,

... ??

Woisek commented 1 year ago

BTW, on what commit was the black-orange.css changed?

Gegell commented 1 year ago

Installed of course, but like I wrote, I deactivated all extensions, as you can see in the navigation

Maybe try temporarily removing the extensions from the extensions folder, e.g. move to other location instead of just deactivating them. Iirc some of their stuff still gets loaded even when deactivated.

vladmandic commented 1 year ago

Well, I said multiple time on what to probably look at. And this is more than just a hint now.

if you feel so, feel free to do it yourself. i've stated that black-orange does look different in your screenshots, but BOTH screenshots are messed up (to you it seems one is slightly less messed up than other, but thats it). so fixing black orange theme does nothing as it does not solve the underlying problem. i'm trying to help and it feels like you're trying to proove a point instead?

Installed of course, but like I wrote, I deactivated all extensions, as you can see in the navigation (that's why I left this portion in the screen shot). You don't really expect I de-install all the stuff ... ?!

what --safe does is simply skips loading of any user extensions. nothing else. so i don't know what you think you did, but you did not deactivate them if the problem persists when you "disable" them, but goes awat in "safe" mode.

Gegell commented 1 year ago

@vladmandic After trying to install as many extensions as possible I found from @Woisek screenshot, I finally managed to reproduce it :) Now I should be able to debug more easily ^^

grafik

vladmandic commented 1 year ago

@Gegell thanks. but which extension?

Gegell commented 1 year ago

Still searching which one, but imma let you know once I do ^^ Just wanted to give a heads up.

Woisek commented 1 year ago

I'm doing one by one ... needs time ...

Gegell commented 1 year ago

Soo, as far as I can see this is the state with only sd_grid_add_image_number enabled, and the current master branch, not my attempted fix: grafik

I hope this is the only one extension, otherwise there might be multiple different issues at play here :/

This error actually gets fixed on the branch with the fix, others do not seem to exhibit such behavior either :) New branch, no miss alignments: New branch, no miss alignments

Woisek commented 1 year ago

Yeah, I also come to this conclusion, that sd_grid_add_image_number is the issue ... ๐Ÿ˜ž

vladmandic commented 1 year ago

https://github.com/AlUlkesh/sd_grid_add_image_number/blob/8a78ca7bdd6f18425f03bb64343f28ea871ef0d7/scripts/sd_grid_add_image_number.py#L42L44

it uses a system defined section saving-images and tries to change it to Saving images/grids causing hell in html. please report to extension owner.

Woisek commented 1 year ago

OK, after the final run, I have all extensions except for sd_grid_add_image_number and it looks how it's supposed to be. But I don't understand, how this can be? sd_grid_add_image_number was updated 10 days ago. And this issue came up yesterday. How goes this together? ๐Ÿค”

vladmandic commented 1 year ago

settings tab logic was massively updated - previously it was just a dumb static list, now it actually scans whats new, whats changed, etc. and i'm not disabling that new code because extension does something bad.

and yes, @Gegell update would work, but it works because it changes names of everything. again, i don't want to have to change names in sdnext just to cater to a bad extension - that is upside/down.

example, where is your samples_format setting now? its not under system section because system section has been overwritten with extension. maybe its under extension? point is - extension should never change system stuff.

Woisek commented 1 year ago

OK, thanks all for this difficult birth and hopefully we will see that again. ๐Ÿ˜„

Gegell commented 1 year ago

and yes, @Gegell update would work, but it works because it changes names of everything. again, i don't want to have to change names in sdnext just to cater to a bad extension - that is upside/down.

The fix should only change the names of the modification indicators though, which are completely new, other stuff like tab names etc. remain untouched by me. Though, the id is still used when setting the corresponding hover information on the indicator, there might still be issues / unexpected behavior there.

So while I don't suggest to immediately merge it, I do think it's good to be as accommodating & breaking as little as possible. I suppose I'll make the fix more robust, when I have some more free time ^^ (either later today or tomorrow)

AlUlkesh commented 1 year ago

I just changed the extension. It's now searching for the existing section name associated with the section id. This should make it work for the main repo and this one.