rgthree / rgthree-comfy

Making ComfyUI more comfortable!
MIT License
1.16k stars 81 forks source link

Nested context muting #3

Closed RassilonSleeps closed 1 year ago

RassilonSleeps commented 1 year ago

Preface:

Perhaps I'm just oblivious to an obvious logic problem

Issue:

Context connected to the output of a muted context are still processed resulting in errors

While attempting to expand my workflow with toggle-able options via muting, I tried using context muting at the end of a mutable section of workflow. Once I muted said section, I would receive errors upon attempting to run. Is this something that can be done as is, or would this require additions to the context module on your part? As I prefaced, perhaps I'm just missing something.

Example:

In my use case I was attempting to create toggles allowing for the saving multiple image outputs in multiple locations using mutable context into the WAS Suite's Image saver. Here's a mock-up example where the second upscaler (U2) workflow is muted: Screenshot_2023-08-22_02-30-59

Obviously I could mute each of the contexts corresponding to a muted section, but in a more complex non mock-up workflow this would quickly become tedious.

Would it be possible to add a 'Mute' boolean to the I/O of contexts? If the context is able to read and change its mute state according to the boolean it would allow for more complex workflows; especially if it can be carried by the pipe and overridden by new input of the boolean like other I/O. Here's another basic mock-up with U2 disabled showing how such a workflow could be beneficial: Screenshot_2023-08-22_02-26-42

This functionality would nicely complement the logic nodes of other custom nodes packs.

Post Script:

Seriously I can not thank you enough for the amazing work you've done in releasing these nodes, they really are a game changer!

rgthree commented 1 year ago

Thanks, I'm glad you like them!

For the issue, I may be having trouble understanding. Do you have an example workflow json I could load that breaks, and which nodes you're trying to have be muted or not?

Your first mockup looks like it would work (all three alts are saved, none of the gen's are). Is it simply that you'd rather the node only have one toggle to mute all three in the group?

RassilonSleeps commented 1 year ago

Sure, here's an example workflow I made to demonstrate it: https://gist.github.com/RassilonSleeps/9635aef52b4156bb0682d85b74583b37

It requires 'Impact Pack' for latent upscaler and 'WAS Suite' for the save image, as I wanted to keep it representative of my actual workflow. I also added some additional examples on the right side under the 'Extra Flow' section, which results in the error when the 'Extra Flow' context is muted and the contained contexts are not.

Essentially the issue is that contexts piped from a muted context are still having their outputs processed. In the example the save image nodes are connected to mutable contexts (gen or alt) which receives its inputs from the context pipe of the former section (U1 or U2). So when, for example, upscaler 2 (U2) is muted the save image node errors out for receiving 'NoneType'.

My second mock-up was just to show that it would be feasible to simplify mute switches if a boolean system was implemented; although, I realized the example mute boolean reroutes would require comparison logic but that's entirely unrelated.

rgthree commented 1 year ago

Thanks, much easier to see (it as hard for me to understand where the connections were in the mock).

I see the issue and, unfortunately, it's a bit complex because of the flexibility of the Context node, it can't be validated so if it's not muted, the backend will use it and the next node will complain (whereas, when it is muted, the backend knows the next node will complain and suppress the error).

Unfortunately, there isn't an easy fix; the ComfyUI backend won't let me verify Context nodes in a flexible way, so it'd have to be hacked in the UI layer.. maybe try to mute connections of Context when using the Fast Muter or something (which doesn't feel great, tbh).

But, for your specific workflow, you can still do what you want, we just can't rely on that second Context to be the in between. It's too bad the Muter can't directly mute the Image Save since there's no output, but if we replace the "Save UX to..." Context nodes with a node that has an image input then that can stay unmuted if the U1/2 Context node is muted without a popup error.

Here's an example using the "Scale Image By" instead (for U2). In the screenshot the Context U2 is muted, but the Scale Image By node is not, and there's no stopping error. Since it's a 1x scale there shouldn't be any work done, we're just using it so we can connect the output to the save node, AND to the muter node.

image

It's not pretty, but it'll get you the functionality you want until I can think of an elegant solution otherwise.

RassilonSleeps commented 1 year ago

Sorry, my mock-up definitely could have had the noodles presented better. I figured it was a matter of backend complexity and likely something that wouldn't easily be changed in the node. Makes sense to just use something to act as a pass-through and get input from the per section context output.

I'll be honest though, I hadn't even thought about muting anything other than a context node. (┛ಠДಠ)┛彡┻━┻ Brain too smooth, see preface.

Anyways, I appreciate your time looking into it and for helping provide a functional alternative. Thank you, closing the issue.