rgthree / rgthree-comfy

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

[FEATURE REQUEST] Fast Groups Muter/Bypasser switches custom order #65

Closed alessandroperilli closed 11 months ago

alessandroperilli commented 11 months ago

Per my previous comment on the commit, these new nodes are amazing and they could drastically reduce the amount of wires in AP Workflow and others. But I cannot use them because I cannot specify a custom order for the switches.

My most likely use case would be the specification of a series of group titles via RegEx. E.g.: info|builder

In an ideal world, the switches would follow the order I just used in the regular expression. Instead, they either follow the alphabetical order or the order in which the groups were created/located (not sure):

Screenshot 2023-12-01 at 15 34 29

Would it be possible to follow the custom order I specified?

Thanks!

rgthree commented 11 months ago

Yea, the title match is just a filter, not a sort. Unfortunately Groups in ComfyUI aren't really first-class items like nodes are and, because of that, it's harder to have a custom order if groups are changing.

(Note, you can change your sort property to "alphanumeric" to fix this specific order, as "p" comes before "u", but that's not answering your real question)

My hope was that users' have their graphs laid out in a fashion where position sort (position like a reading a book, left before right, top before bottom) makes sense; that workflows are generally laid out from top left to bottom right. But, I added an "alphanumeric" sort option to provide a way for users to manually sort by changing the group title (so user's could prepend a number, say, to groups that dictate an order. "1. Prompt Builder" and "2. Universal Negative Prompt" etc.).

That may not be always advantageous, but I'll have to think of an elegant solution for a custom sort that will not be too complex for users to setup.

alessandroperilli commented 11 months ago

I thought about putting a number in the group title, but that becomes really unreadable and ugly in the resulting master switch. It's not ideal.

How about using your new Bookmark node and adding a number/label there?

Cloud the Fast Groups Muter/Bypasser node read the content of those bookmarks and arrange the groups' order accordingly?

I have no idea how complicated this suggestion is, but it seems a lot easier than trying to rearrange the group in the way you are describing. Applying that to a big workflow like AP would impair the readability of the flow to the point of forcing users to memorize group positions rather than understanding the flow.

To clarify:

I'm suggesting that a group is called "Group A" and contains a Bookmark node with the text "1" (or "A") in it. TheFast Groups Muter/Bypasser node read the content of the Bookmark node and creates a switch called "Group A". The text "1" (or "A") doesn't appear anywhere in the switch.

rgthree commented 11 months ago

Interesting.. but I think that's too much indirection between nodes.

If the ugliness in the switch is more of the problem than the group names, I wonder if removing "Enable" in the row would be cleaner?

(I was debating doing this anyway, as I think it's self explanatory..)

alessandroperilli commented 11 months ago

The word Enable is redundant, yes, and removing it would make the space cleaner. But it's not just that. There are other issues. Let's say I go for the number in the group title and the alphabetic ordering to try to replicate the situation below without the 14 wires I currently have to use:

Screenshot 2023-12-02 at 23 06 48

What would happen is that users would see something like:

Image Sources 1 - Image Generator 2 - Uploader

Image Manipulators (Level 1) 3 - SDXL Refiner 4 - Inpainter + HighRes Fix 5 - IPAdapter 6 - etc.

It's not intolerable, but it's a bit weird that they would have to see the numbering segmented across switch groups :)

Then, there's the fact that if I want to change the order of the switches, I'd have to rename an obscene number of groups every time (which is way more clicks than moving around the Bookmark nodes in my alternative proposal) - this is why I originally hoped that just listing the order as a string would do. It's the fastest way to reorder a large number of groups.

At that point, I might prefer the wires to that.

That said, this amazing node is not for me. It's for everybody. It doesn't have to suit my needs. If it can't adapt well to my situation, it's completely OK.

rgthree commented 11 months ago

Okay, I like the custom idea. Here's how it works (also in the node help dialog):

alessandroperilli commented 11 months ago

This is absolutely great. Thank you. I'll test it in the next few days and, if everything works fine, I'll include it in AP Workflow 6.1.

One last question: does this approach have any impact on the browser rendering performance (for example at startup, or on refresh)?

If I have 5 groups of switches, as I understand it, I am effectively replicating a 14-switches node five times and, for each, I'm just hiding the switches I don't want people to see.

I cannot notice any degradation in performance from my very early tests, but I have 96GB RAM and I might not notice.

Thanks

P.s.: I notice you decided to keep the word "Enable." I still think it's redundant.

rgthree commented 11 months ago

Technically that's right; each fast groups toggler looks at all the groups, and filters them. If using a custom alphabet it introduces a nested loop increasing time complexity.

For any feasible workflow this should take ~0-1ms, maybe 5ms on a slower machine. If you start to exceed 500 groups and 10+ Fast Groups switches, then there would probably be some slow down, though, ComfyUI is probably slowing down itself. For comparison, the Fast Muters loop every 500ms and check state, while ComfyUI is looping every ~16ms and calculating the entire app to draw each node.

However, there is probably some optimizations I could pull out of the indiviual nodes and into a service so multiple group nodes are sharing groups state rather than calculating each iteration. I'll probably do that.

rgthree commented 11 months ago

Oh, and for keep "Enable", yes, I did keep it. I have a stashed change that removes it...

The issue is around usability. "Enable.. yes/no" is very clear. Without that hint, it starts to become too ambiguous if the "on" state means it's active or muted/bypassed. (For a node called "Bypasser" for instance, one may first assume the "on" state means it's being bypassed).

The stashed change works well for muting, but changing the toggle iconography. But the fast bypasser is much more difficult to get the correct usability pattern. So, "Enable.. yes/no" is staying for now.

alessandroperilli commented 11 months ago

OK, testing has began, and I still have some issues with the sorter. I am trying to recreate the node on the right:

Screenshot 2023-12-04 at 12 40 11

But it seems like I cannot define a custom alphabet term with a space in it. So I have the two "Face something" labels that keep ending together.

rgthree commented 11 months ago

It should work, but since it's just a "startsWith" walking through the custom alphabet, the first lone "f" is capturing both nodes since they both start with "f". You would need to have the more restrictive entry first (so, you would need atleast face d before f).

The alphabet h,face d,o,f,u should work (no quotes needed).

alessandroperilli commented 11 months ago

I tried every possible syntax except no syntax... Thank you.