TinyTerra / ComfyUI_tinyterraNodes

A selection of nodes for Stable Diffusion ComfyUI
GNU General Public License v3.0
332 stars 39 forks source link

file saves and xyPlot #107

Open rjgoif opened 2 months ago

rjgoif commented 2 months ago

Really loving the xyPlot upgrade. This is now one of my favorite nodes in all of ComfyUI because I can iterate over everything.

One suggestion I thought of while using it heavily: why is there no option to output the individual images to the "image" output of the Ksampler node? If I output individual images via the toggle on the advanced xyPlot node, they all save to my default directory as pngs. I cannot edit their folder, change their metadata, or even use a different file format to save HD space.

Take the following workflow example, which has a few problems as it is currently set up: workflow

  1. The image output only sends the first image of the xyPlot. The other images (and the grid itself) never make it past the preview window in the ksampler node.
  2. There are no intrinsic save options in the ksampler node.
  3. Subfolder names are not parsed from the file prefix, as far as I can tell.

Problem 3 might just be because I don't know the right syntax to use folders, I dunno. If so, easy to fix with a little documentation.

Problem 2 seems like a big thing to tackle, as it would make the Ksampler nodes bigger and more complicated while requiring you to keep all the Ksampler nodes up to date together. Instead...

Solving problem 1 solves the other two. If you output all the images from the ksampler, they can be saved with pre-existing save nodes from other packages with all the options the user wants. Additionally, incorporating a "total number of XY images" output from the xyPlot node (a la this example below, "Total images") image

allows the user to employ other nodes nodes to "extract image N+1" where N is the number of XY iterations, making image N+1 the grid image itself. The user can then save the grid with a different prefix, to a different folder, at a different quality/size, etc.

Does that make sense or am I rambling too much? Happy to discuss further.

TinyTerra commented 2 months ago

one thing to point out for that wf - just as another option, you can use %something;otherthing% as a search and replace. so in that example you could have the prompt as 'a cat riding a motorcycle' in the loader, and then the x plot be

<1:v_label> <2:v_label> [1:positive='%cat;lizard%']

(although i pushed a change that allows no value axis if not using a custom label, it was broken previously)

On to the actual issues..

    • You can change which image is output using 'latent_index' in the adv-xyPlot node (admittedly this isn't the best implementation and i wasn't happy with it from the beginning)
    • I could change it so that output_individuals isn't related to saving the individuals and accumulate them within the node - but that raises the question of how to handle the multi image/latent outputs going into another KSamp/node. I guess the only way would be having an extra node between them (I'd rather not rely on external nodepacks for core features so i'd add a ttN version).. selecting the index in the xyPlot or in the KSamp has the issue of forcing a re-run of the node to select a separate image, which one of the main reasons the current way is far from ideal.
    • I'd rather avoid erroring on input if multiple images/latents are sent to another ttN node - which i think they currently would, that means refactoring a majority of my nodes... and figuring out the best way to deal with it (only use the first image/latent if pushed in? I guess that makes the most sense?
  1. I've just pushed an update for this, adding file_type and embed_workflow (currently only works for png) to all the KSamps

  2. I thought I'd already fixed this, but I hadn't. Last push also addresses this, in the save_prefix it behaves how you'd expect it to > some_folder/sub_folder/filename

Mostly makes sense, and not too much rambling! - I appreciate the thought out feedback, always helps to make the nodepack better.

rjgoif commented 2 months ago

I didn't realize that's what latent_index did. I see that it works with the individual images now, but I can't figure out how to make the output of the image noodle the actual grid. If I have 2 images in a grid, index 0 is the first, index 1 is the second, and index 2 (or any number higher) is the second image again. Negative numbers cause errors. I would have assumed that index N where N is the number of images outputs the grid itself.

Can the xyplot node itself have an output of just the grid? Can you feed something back into itself via an output noodle? I feel like rgthree does something like that but again would have to test and come up with an example to prove it.

I think a bunch of other nodes take a list of input images as a queue to process them all in sequence, then output them all in sequence. I could be wrong, though. I would have to test some examples.

Save prefix folder assignment works now, thanks!

Looking forward to webp embeddings. They're smaller files than pngs and I make use of them from the WAS pack, which has a really good image saver node.

rjgoif commented 2 months ago

Back to your statement about outputting images in a group: I think this workflow shows how it can be done- I don't think the images are a batch because you can't select them as a batch (but you can after using a "rebatch images" node). I have taken these grouped images and used them as inputs into tt nodes (e.g. as image overrides in a Ksampler node) and it worked just fine, no errors. The image output on the other side is another group of images, as expected.

workflow 5

This other workflow uses a separate node to make a plot, but that functionality could be baked into the xyPlot node itself too as a final image in the series (or as I said above, a direct image output from the xyPlot node).

TinyTerra commented 1 month ago

Thanks for the extra examples. i do think using batches or at least multi image/latent out is the better way to do it, so I'll continue to work on it as i have time.

Latest push added workflow embedding for webp.

TinyTerra commented 1 month ago

I have added, essentially this, now with a separate plot image output. I'm not sure yet if the single images should really stay on the KSampler preview output or just the plot image, but nevertheless the image and latent outputs should allow you to use an image picker. I might implement a basic one within ttN just for pack completeness but we'll see.

A side note though, the backend execution of the xyPlots is currently broken on more complex workflows, sometimes nodes in the faux prompt don't re-execute and end up outputting their previous values if they haven't had any direct input changes.. I'll continue to work on making it more robust to fix this.

But hopefully the image/latent out is nicer to use now :)

rjgoif commented 1 month ago

oh YES! I am messing with it now. It is so flexible!

You are a treasure among devs, you know that?

TinyTerra commented 1 month ago

I'm not so sure, but thank-you for the kind words! :)

I think I've decided on only outputting the plot image to the UI of the node.. Did you have any thoughts on that, or is it much of a muchness?

And for the time being just be wary of the output in more complex wf's - the known issue currently is with xyPlot in a second KSamp with the hiresfixScale node between. It's holding onto the previous plot's hires node output..

rjgoif commented 1 month ago

just be wary of the output in more complex wf's

Sure, will keep an eye on it. My current projects don't chain Ksamplers together much so I might not strain this for now.

I think I've decided on only outputting the plot image to the UI of the node.. Did you have any thoughts on that, or is it much of a muchness?

I don't follow you. Do you mean from the XYplot node itself? I think the implementation right now (one output for the image stack, one for the plot) works really well. You think it's too much? I think it's fine and pretty self explanatory.

TinyTerra commented 1 month ago

Oh, no, I just mean the KSampler preview window image

having it show the plot image + actual images vs just the plot image image

Leaning towards only the plot image, but its probably fairly inconsequential haha

TinyTerra commented 1 month ago

just be wary of the output in more complex wf's

on that note, latest push seems to have fixed it

rjgoif commented 1 month ago

Hmmm I think one of these updates broke things. This wf with the up-to-date nodes is broken:

broken workflow

!!! Exception during processing!!! '2'
Traceback (most recent call last):
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\execution.py", line 151, in recursive_execute
    output_data, output_ui = get_output_data(obj, input_data_all)
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\execution.py", line 81, in get_output_data
    return_values = map_node_over_list(obj, input_data_all, obj.FUNCTION, allow_interrupt=True)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\execution.py", line 74, in map_node_over_list
    results.append(getattr(obj, func)(**slice_dict(input_data_all, i)))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI_tinyterraNodes\ttNpy\tinyterraNodes.py", line 1302, in sample
    return process_xyPlot(samp_model, samp_clip, samp_samples, samp_vae, samp_seed, samp_positive, samp_negative, lora_name, lora_strength, lora_strength, steps, cfg, sampler_name,
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI_tinyterraNodes\ttNpy\tinyterraNodes.py", line 1260, in process_xyPlot
    plot_image, images, samples, ui_results = plotter.xy_plot_process()
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI_tinyterraNodes\ttNpy\tinyterraNodes.py", line 747, in xy_plot_process
    self.execute_prompt(x_prompt, self.extra_pnginfo, x_label, y_label)
  File "C:\Users\USER\AI\Comfy\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI_tinyterraNodes\ttNpy\tinyterraNodes.py", line 648, in execute_prompt
    ui_out = self.executor.outputs_ui[self.unique_id].get('images')
             ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
KeyError: '2'

Prompt executed in 3.71 seconds

EDIT: b69483b works, 652d55e is broken. I reverted for now.

TinyTerra commented 1 month ago

Ah of course. The latest push should address this. Thanks for all the info!