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

geninfo from Infotexts #14645

Merged
merged 4 commits into from
Jan 20, 2024
Merged

geninfo from Infotexts #14645

merged 4 commits into from
Jan 20, 2024

Conversation

w-e-w
Copy link
Collaborator

@w-e-w w-e-w commented Jan 14, 2024

Description

per prior discussion
switch the source of information to the more reliable infotexts
reduce the complexity of updateing geninfo

  • reuse seed form infotexts
  • save_files using info from infotexts
  • add a new arg skip_fields to parse_generation_parameters, defaults to shared.opts.infotext_skip_pasting
    set to [] to not skip any fields or set to custom list[str] for special use case

    if this this is a new function I would do it the other way around
    set default to [] and pass shared.opts.infotext_skip_pasting wheing using for applying infotext
    but it is possible some extension is relying on the current behavior
    that is why it is implemented this way as to not changing current behavior

I'm not sure if we use gen input in some other place if so they will have to be updated too

related


minor behavioral changes
previously when saving single images using the save button the info written to CSV is always the info of the first image regardless of the image chosen to be saved,
I think this is a bug so I modified the code so that it will write the corresponding info to the CSV of the chosen image


there's also something that I'm a bit puzzled,
when saving zip we also save all the images as individual files alongside the zip file
basically are saving the files twice
is this intended?


copied from prior discussion for documentation purposes

w-e-w — 2024/01/12 21:08
I still don't like geninfo is created in stages of the pipeline and then have to be updated individually
and different entries have different rules

I really think it should be consolidated into one object link to one image
(which basically is what infotexts is already) 
AUTOMATIC — 2024/01/12 21:17
as in, an array, where each ittem correspons to each image in the gallery?
w-e-w — 2024/01/12 21:18
the issue is not with the array the issue is with other things such as all_seeds
when grid is returned to the gallery
all_seeds is not updated
geninfo['index_of_first_image'] is set to 1
same logic with subseed
AUTOMATIC — 2024/01/12 21:19
what I mean it, now geninfo is a dict with info about all images
what it can be, is a list, where each item is a dict wuth info about one image
w-e-w — 2024/01/12 21:21
what it can be, is a list, where each item is a dict wuth info about one image
which is what  geninfo['infotexts'] already is
AUTOMATIC — 2024/01/12 21:21
right but you'd have to parse it
w-e-w — 2024/01/12 21:21
you also have to parse json string back to dict
AUTOMATIC — 2024/01/12 21:22
geninfo = {'images': [{"seed": 1, infotext: "...", ...}, {"seed": 2, infotext: "...", ...}]}
 
w-e-w — 2024/01/12 21:29
you could do that but that
but that also means that extension will also have to write code to update gen info

on the other hand if everything is just inside infotext
any extensions that already adds p.extra_generation_params will work automatically

unless you wish to rewrite the entire geninfo so that it based of generation_params

it is not that complicated, I hardly have to implement anything
I have a code for parseing infotext form html_info_txt2img
https://github.com/w-e-w/sd-webui-hires-fix-tweaks/blob/main/hires_fix_tweaks/ui.py#L13-L28
just a HTMLParser and generation_parameters_copypaste.parse_generation_parameters 
AUTOMATIC — 2024/01/12 21:32
so your suggestion is to keep everything as it is, but use infotexts field to get info, and forget about other fields?
w-e-w — 2024/01/12 21:32
exactly
I don't really see the advantage of keeping them separate
AUTOMATIC — 2024/01/12 21:32
what about extensions that use all_seeds and expect it to be correct?
w-e-w — 2024/01/12 21:33
hmmm, backwards compatibility issues
are there any extensions that use all_seeds?
w-e-w — 2024/01/12 21:42
the difference between all_seeds and what can be grabbed from infotexs is all_seeds is the seeds of all bach
while if you grab every seed info from every infotext in infotexts and is 
infotexs might have other images that are not stable diffusion images such as grid or detection maps
so they're are not equivalent
but I really not sure if anyone's actually using all_seeds apart from reuse seed in the ui 
 
to be honest I think lots of extension doesn't do a good job of updating geninfo
AUTOMATIC — 2024/01/12 21:43
we use it ourselfves when saving image too
ui_common.py, save_files
w-e-w — 2024/01/12 21:44
ourself is a small issue
we can modify it to make sure that it works with infotext
w-e-w — 2024/01/12 21:45
this is one of the reason I want to move to use only infotexts
because it simplifies extension development and reduces people forgetting to update stuff
w-e-w — 2024/01/12 21:54
even though all_seeds and infotexs are not equivalent
the difference doesn't even matter because if an extension wishes to
if it extension returns extra images they will update all_seeds otherwise it will cause issues
so if they are an exception that returns multiple images all_seeds basically is already "dirty"
 
so if an extension does rely on all_seeds
the extension is likely going to be very unstable
 
so I don't think it matters that much if this change breaks extension that specifically relies on all_seeds
because it's like they already broken by other things
AUTOMATIC — 2024/01/12 22:11
all right, it makes sense, let's use infotexts then

Checklist:

Copy link
Contributor

@aalbacetef aalbacetef left a comment

Choose a reason for hiding this comment

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

just a few minor comments

modules/infotext_utils.py Show resolved Hide resolved
modules/infotext_utils.py Show resolved Hide resolved
modules/infotext_utils.py Outdated Show resolved Hide resolved
@AUTOMATIC1111 AUTOMATIC1111 merged commit 7581da5 into dev Jan 20, 2024
6 checks passed
@AUTOMATIC1111 AUTOMATIC1111 deleted the infotexts branch January 20, 2024 07:26
@w-e-w w-e-w mentioned this pull request Feb 17, 2024
@pawel665j pawel665j mentioned this pull request Apr 16, 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.

3 participants