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

[ Feature ] [edit: option to] show group buttons in multiple rows ( like before [edit: but better] ) #303

Closed
JorgeR81 opened this issue Nov 6, 2024 · 88 comments

Comments

@JorgeR81
Copy link

JorgeR81 commented Nov 6, 2024

I use narrow, full height, Controllers.

I have 2 buttons per controller.
It would be better to just show the buttons in 2 rows.

bt

@rollingcookies
Copy link

rollingcookies commented Nov 6, 2024

I would do it this way:

image

[ ■ ] - minimize widget or controller
[ B ] - bypass of the controller
[ M ] - mute of the controller
[ Group1 B ] - group with enabled bypassing with an icon with an arrow circling the circle
[ Group1 M ] - the muted group has a closed eye icon

The controller name is set by double-clicking the default name

Regarding the need to use both modes (Bypass, mute) I wrote here:
#302 (comment)

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 6, 2024

What about having an user option to display 1 group button per row, with its individual icon buttons.

This would also adress this issue:


This would also alllow:

  • bypass a group without selecting it
  • remove a group button without selecting it
  • ( for visual simplicity the advanced controls icon could be allways hidden, if a group is not selected )

m1

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 6, 2024

But for the larger Controllers, I would still like to have the single row "view".
( e.g.  in the "big picture area", to show the image of each stage )

bb1


Maybe this would be toggled, per Controller, instead of being a global setting. 

We could have an icon in each Controller to toggle "list view" ( and it could be blue en enabled ).

list

mockup:

tabs view

bb1x

list view

m1z

@chrisgoringe
Copy link
Owner

I like where this is going....

@chrisgoringe chrisgoringe added this to the To be roadmapped milestone Nov 6, 2024
@chrisgoringe chrisgoringe changed the title [ Feature ] show group buttons in multiple rows ( like before ) [ Feature ] [edit: option to] show group buttons in multiple rows ( like before [edit: but better] ) Nov 6, 2024
@chrisgoringe
Copy link
Owner

chrisgoringe commented Nov 6, 2024

I wonder if, in list view, it would look good swapped around, so the icons are still on the left as they were before, and the title floats right? Just for slightly more consistency between the views. Maybe for wide controllers it would look weird, though.

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 6, 2024

in list view, it would look good swapped around

I did that to avoid aligning the buttons to the right, but it also looks good.

m1zr


Or we could still align to the left:

m1zr1


Or have the same width on buttons and align the text to the center.

m1zr2


And maybe the + icon in the bottom row

m1zr3


And if wider:

wide


But swapped around also looks good.
It feels more natural to have the icons in front of the name.
And this way, they still have the same icon order ( but with the button behind them ).

wide1

@rollingcookies
Copy link

rollingcookies commented Nov 7, 2024

Now it looks like you want the name of the group tab to be the title of the window at the same time, but at the same time you want to be able to display several titles (names of groups) in a row. This is wrong - a window (controller) can have only one title otherwise the interface turns into 💩! You can solve this problem with accordions! 😊

Next, I recommend prioritizing the functions that the user uses often and those that the user uses rarely. If we omit long considerations, I have removed the button to close the [ X ] window and delete the current group from the controller window into a pop-up menu in the form of items: delete and delete all. Also, are you not considering options where the group name would be multiple words, how would a chain of multiple group switches look then with all those switches being in the controller window headers?

Once again, I remind you of the need for both bypass and mute switches to be present in the interface
#302 (comment)

This makes three options, taking into account the above wishes:

  1. A variant where the group name is in the header of the controller window, the other controllers are selected from the menu:
    image

Hovering over the [ ... ] icon opens a popup menu:
image

Hovering over [ v ] displays a popup menu with a list of groups and a function to add a new group:
image

  1. A variant when we want group names to be both displayed in the controller and be a header for the group:
    image

[ T ] - trash can icon for deleting a group from the controller

  1. Variant from the first comment [ Feature ] [edit: option to] show group buttons in multiple rows ( like before [edit: but better] ) #303 (comment)

Also in all variants there is a unified minimize icon [ ■ ], which can be a circle as in nodes in a graph and which makes the same indentation from the left edge for easy reading of Controller, group and widget headers

I'd settle on the first option, as you should neatly place the inclusion of advanced widgets ), in the first option it would remain a compact and efficient solution, you just need to add an item to the popup menu:
image

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

Now it looks like you want the name of the group tab to be the title of the window at the same time, but at the same time you want to be able to display several titles (names of groups) in a row. This is wrong - a window (controller) can have only one title otherwise the interface turns into 💩! You can solve this problem with accordions! 😊

The Controller windows never had any title / name.
It's just the group name, when you have a single group button ( see the images ).

As we discussed before, it's better not to have "names" that are not used in the node's graph ( e.g. a title for each Controller ).
This way the Controller would be easier to understand, in the long run.

If you just want to have a single group button in the controller, you don't need "list view".
Maybe the button could be hidden, ( and "list view" disabled ), if there is just a single group button.

b0

b1

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

Once again, I remind you of the need for both bypass and mute switches to be present in the interface
#302 (comment)

We could still have a "mute" button, with any of these layouts.

Maybe we could use the ban icon to "mute", instead.

ban

And to bypass we could use of these:
001

06

022

02

https://www.primefaces.org/diamond/icons.xhtml

@chrisgoringe
Copy link
Owner

I've never used mute - but isn't it done using a widget on a node, the rgthree switcher?

In which case, surely the best approach is just to support that widget on the controller like any other widget? Rather than trying to duplicate the functionality?

I'm very resistant to the idea of making the controller add functionality which isn't already in the workflow.

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

The rgthree switcher does not need to be in the controller. 

It's automatic ! No widgets.

The first non muted/bypassed link is used.

We just need to be able to mute/bypass a node/group in the Controller

bpr

@rollingcookies
Copy link

The rgthree switcher does not need to be in the controller. 

It's automatic !

The first non muted/bypassed link is used.

We just need to be bale to mute/bypass a node/group in the Controller

bpr

Mute mode is needed for nodes that pass through values in bypass mode and they have data on their output even in bypass mode and in this case Any Switch node will not work correctly!

@chrisgoringe
Copy link
Owner

chrisgoringe commented Nov 7, 2024

In the example above, you jsut have to bypass the upper "Load Image" node.

How do you mute a node?

@rollingcookies
Copy link

I've never used mute - but isn't it done using a widget on a node, the rgthree switcher?

In which case, surely the best approach is just to support that widget on the controller like any other widget? Rather than trying to duplicate the functionality?

I'm very resistant to the idea of making the controller add functionality which isn't already in the workflow.

In fact, if you make support for Fast Groups Muter (rgthree) and Fast Groups Bypasser (rgthree) nodes, you don't need the Mute and Bypass buttons in the controller at all! That would be the best solution!

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

Mute and bypass would have different effects, with multiple nodes in the sequence .

zz1

zz2

@chrisgoringe
Copy link
Owner

Look, like I say, I've never used mute.

Is it part of the Comfy core functionality? How do you actually mute a node?

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

Is it part of the Comfy core functionality? How do you actually mute a node?

Ctrl +m

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

In fact, if you make support for Fast Groups Muter (rgthree) and Fast Groups Bypasser (rgthree) nodes, you don't need the Mute and Bypass buttons in the controller at all! That would be the best solution!

We could support these nodes.

But it would be better to have the buttons in the Controller.

@chrisgoringe
Copy link
Owner

ok - so mute is actually core, and is just a variation on bypass (that produces no output instead of passing the input through)?

Can you mute a group (in core Comfy)?

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

Can you mute a group (in core Comfy)?

Yes, that's what I did in the UE nodes: chrisgoringe/cg-use-everywhere#171

I bypassed a group by muting it !

@chrisgoringe
Copy link
Owner

How do you mute a group in Comfy? Without any custom nodes?

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

In core:

Set Group Nodes to Never

nev

@chrisgoringe
Copy link
Owner

So "never" is "mute"....

Hmmm.

Maybe the best icon would be a mute icon, then, like pi-volume-off with a diagonal slash superimposed...

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

With rgthree we can have buttons to mute and bypass in the group header.

He uses an eye to mute.

zr

mute

@chrisgoringe
Copy link
Owner

Seems to me that we have space in the current layout to have both bypass and mute icons on the header, and also we have space on the node title bar to add them on a node by node basis.

Choosing the right icons might be the hardest bit!

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 7, 2024

Choosing the right icons might be the hardest bit!

Yeah ...

@rollingcookies
Copy link

I vote for the Rgthree icons - they are easy to understand and they are already being used for this purpose (if, of course, you have democracy :D)

@rollingcookies
Copy link

Anyways, as I said in this discussion, muted state should be similar to nodes in graph (low opacity) instead of having a grey overlay.

Yes, have them behave as in the graph with nodes - the white ones become translucent when turned on

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

Anyways, as I said in #309 (comment), muted state should be similar to nodes in graph (low opacity) instead of having a grey overlay.

I agree with this, in principle.
And I had the same opinion, you have.

But, in practice, if you have many panels, it's much easier to differentiate the panel's state, the way it is now.

I'm always looking to replicate the ComfyUI style.
But we also need to have what works best in the Controller.

img

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

That red should be a more pastel color, too

Yeah, I would agree with that.

Also, the red pastel icon color, would look better, in these "grey" muted panels.

@LukeG89
Copy link

LukeG89 commented Nov 8, 2024

But, in practice, if you have many panels, it's much easier to differentiate the panel's state, the way it is now.

If you make lower opacity (let's say 20% or so) you should be able to differentiate them from active and bypassed

Not to mention that if you have grey nodes, you won't notice the difference

grey

@rollingcookies
Copy link

Anyways, as I said in #309 (comment), muted state should be similar to nodes in graph (low opacity) instead of having a grey overlay.
I agree with this, in principle. And I had the same opinion, you have.
But, in practice, if you have many panels, it's much easier to differentiate the panel's state, the way it is now.
I'm always looking to replicate the ComfyUI style. But we also need to have what works best in the Controller.

First of all, I would move the Bypass/Mute icon to the right side of the widget header - it will not stick to the text of the widget name.
Second, the icons have statuses that the user counts by if they need to.
Third, the status of the widget is also tracked by the saturation of the widget itself - it becomes semi-transparent completely and it is clearly visible.
Fourth, the widget status (bypass/mute) is not the most important information that the user needs to work with the Controller.
Fifth, you can make icons not in a circle, but just standard icons: triangle, two sticks, square - it will be much easier to recognize the icon by its external shape

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

If you make lower opacity (let's say 20% or so) you should be able to differentiate them from active and bypassed

Yes, I agree it could be finetuned a bit. I would have to see how it looks.

But I'd like to keep the the same overall brightness on bypassed and muted nodes.
This looks better in the GUI, because they are related features.


Not to mention that if you have grey nodes, you won't notice the difference

You can notice the difference due to slider color, and also icon color.

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

First of all, I would move the Bypass/Mute icon to the right side of the widget header - it will not stick to the text of the widget name.

Yes, I also wanted to do that. But it does not look very good in the controller ( #305 (comment) )
The icons are too far apart from the text.


Second, the icons have statuses that the user counts by if they need to.

Third, the status of the widget is also tracked by the saturation of the widget itself - it becomes semi-transparent completely and it is clearly visible.

What do you mean ?
That Bypass/Mute icon color is not needed ?

What about your suggestion for red pastel color ?
I liked that suggestion. I think it would look good !


Fourth, the widget status (bypass/mute) is not the most important information that the user needs to work with the Controller.

What's more important than to know if a node is muted or bypassed ?
That can make a massive difference in the final image.


Fifth, you can make icons not in a circle, but just standard icons: triangle, two sticks, square - it will be much easier to recognize the icon by its external shape

We need to have the same overall shape because it's the same button.
This is what other multi-state icons do ( e.g. crossed and uncrossed icons )

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

I think we should just give a more pastel color to the mute icon, for now.

We should let @chrisgoringe, move on to more important features, in version 1.4 ( e.g. snapping ).


In version 1.5 we will have ComfyUI theme support.

This will allow much more aesthetic customization.

With Color Palettes, we may even be able to change the bypass color, like we now can do in ComfyUI:

Comfy-Org/ComfyUI_frontend#993

@rollingcookies
Copy link

rollingcookies commented Nov 8, 2024

First of all, I would move the Bypass/Mute icon to the right side of the widget header - it will not stick to the text of the widget name.

Yes, I also wanted to do that. But it does not look very good in the controller ( #305 (comment) ) The icons are too far apart from the text.

It doesn't matter if the icons are not next to the widget header, the main thing is that they are inside the widget they disable. But the fact that the Bypass/Mute toggle button is moved because of the [Pinned] widget fixing icon - it looks bad.

image

In lists with pinned items it is not necessary to put a pinned icon on each item, it is enough to make a separate section at the top with the title Pinned, and after this section place all other items in the list.

================
Pinned
----------------
Item 10
Item 11
Item 12
================
Item 1
Item 2
Item 3
.....
================

Second, the icons have statuses that the user counts by if they need to.
What do you mean ? That Bypass/Mute icon color is not needed ?

Yes, you don't need colors for these buttons as there are icons for the button states!

Third, the status of the widget is also tracked by the saturation of the widget itself - it becomes semi-transparent completely and it is clearly visible.
What about your suggestion for red pastel color ? I liked that suggestion. I think it would look good !

This was about the mute/bypass group button (which is always placed on the background of the controller) but not the buttons for widgets. It's bad to color-code a Bypass/Mute button that will be on top of a background that has a random color - these colors can often conflict and cause your eyes to bleed. A better solution is to make the buttons inside the widget white and play with the transparency of the button as well.

Fourth, the widget status (bypass/mute) is not the most important information that the user needs to work with the Controller.
What's more important than to know if a node is muted or bypassed ? That can make a massive difference in the final image.

You are wrong! The most important thing is the widget data and the work with this data by users, the other things is less important information!

Fifth, you can make icons not in a circle, but just standard icons: triangle, two sticks, square - it will be much easier to recognize the icon by its external shape
We need to have the same overall shape because it's the same button. This is what other multi-state icons do ( e.g. crossed and uncrossed icons )

Yes, but on the small scale of micro-buttons, other rules work and you can discard unnecessary detail in favor of readability of what the icon depicts!

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

It doesn't matter if the icons are not next to the widget header, the main thing is that they are inside the widget they disable.

Having the button next to the name makes it easier to see what nodes are muted / bypassed, at a glance.


In lists with pinned items it is not necessary to put a pinned icon on each item, it is enough to make a separate section at the top with the title Pinned, and after this section place all other items in the list.

The "pin" icon is just for nodes with images, to auto adjust the image size, when we resize the Controller.
It's not to keep pinned nodes above of the other nodes.


What do you mean ? That Bypass/Mute icon color is not needed ?

Yes, you don't need colors for these buttons as there are icons for the button states!

What about your suggestion for red pastel color ? I liked that suggestion. I think it would look good !

This was about the mute/bypass group button (which is always placed on the background of the controller) but not the buttons for widgets.

I think it may be confusing if the mute/bypass button has color for groups, but not for individual nodes / widgets.


What's more important than to know if a node is muted or bypassed ? That can make a massive difference in the final image.

You are wrong! The most important thing is the widget data and the work with this data by users, the other things is less important information!

But if the widget is muted or bypassed, the widget data we have there will not matter.
That's why we need to make it clear that a node is muted or bypassed.


We need to have the same overall shape because it's the same button. This is what other multi-state icons do ( e.g. crossed and uncrossed icons )

Yes, but on the small scale of micro-buttons, other rules work and you can discard unnecessary detail in favor of readability of what the icon depicts!

Having colors in the mute/bypass icons, helps readability.

If the buttons look too small on your screen, you may need to increase the Controller's font size in the user settings.
The whole Controller GUI will be scaled accordingly.

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

About the "pastel" red color.

I think it could look similar to the one used in the Queue menu.

red


The bypass color is also a more "pastel" color an it looks better.

bp

@rollingcookies
Copy link

I think it may be confusing if the mute/bypass button has color for groups, but not for individual nodes / widgets.

Yep. Better make all buttons white/gray like in ComfyUI! You somehow ignore the fact that a colored button on a colored random background is a terrible solution.

But if the widget is muted or bypassed, the widget data we have there will not matter. That's why we need to make it clear that a node is muted or bypassed.

This is clearly visible even without Bypass/Mute buttons - the entire widget becomes semi-transparent. If you make the button on the right side, it will be visible - there will be free space around it! 🤦

If the buttons look too small on your screen, you may need to increase the Controller's font size in the user settings. The whole Controller GUI will be scaled accordingly.

And I would advise you to study color theory and the rules of color mixing :)

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 8, 2024

Yep. Better make all buttons white/gray like in ComfyUI!

Comfy UI also has blue and red buttons.


This is clearly visible even without Bypass/Mute buttons - the entire widget becomes semi-transparent.

I think having both visual cues is better: button color and panel color.


And I would advise you to study color theory and the rules of color mixing :)

What are the colors that are not matching well in the Controller ?

  • The purple bypassed nodes have a purple bypass button.
  • The black / grey muted panels have a red button

@rollingcookies
Copy link

What are the colors that are not matching well in the Controller ?

  • The purple bypassed nodes have a purple bypass button.
  • The black / grey muted panels have a red button

In the nodes column, the widgets in the Muted state are not grey, but semi-transparent and have the same color that the user specified. Making them a different color in this state is to further confuse the user.

Also in the above link, (Comfy-Org/ComfyUI_frontend#993)
discusses the possibility of changing the user color for the Bypass state, which means that soon the bypass icon will also not match the color the user will choose.

Both of these facts suggest that the Bypass/Mute button should be white and translucent when enabled.

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 9, 2024

@rollingcookies, we are also discussing aesthetics in this discussion: #309

I think we should have all aesthetic ideas in one place, so that they are easier to follow.

This issue is supposed to be only for multi-row headers.

@chrisgoringe
Copy link
Owner

chrisgoringe commented Nov 9, 2024 via email

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 9, 2024

OK, here: #314

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 19, 2024

This is a mockup, with the new proposed header style ( #361 (comment) )


  • tabs mode

x1b


  • list mode

x1b1

@chrisgoringe
Copy link
Owner

do you think it would look better with the 'advanced' lightning flash to the left of the name so that they are aligned?

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 19, 2024

do you think it would look better with the 'advanced' lightning flash to the left of the name so that they are aligned?

Yes, but then we would have an empty space, if the group doesn't have advanced controls.
And also, I'd like to keep the bypass button next to the name, like on the nodes.

zzz


It may look better if all the group buttons have the same length, in "list mode".
Use the length of the longest button.

This layout is also more convenient, if we need to add more icon buttons in the future.

zzz1

@JorgeR81
Copy link
Author

The bolt icon on the left, could be the best option, depending on how we implement "hidden widgets" ( #143 ).

ComfyUI calls this advanced widgets, and they can be toggled per node.
See PR video:


If a node has advanced widgets, the bolt icon could show up in the node's header, in front of the bypass button.
And we could also hide these widgets, per node.

ComfyUI's advanced widgets will probably not have an icon. I think it will all be done in the right click menu.
And advanced widgets are like advanced controls, but for each node, so we could have the same icon !

So both icons would have the same layout in both headers ( Node and Controller ).

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 19, 2024

This is a mockup of a Controller with FaceDetailer, with "hidden widgets".

We have both buttons in the both headers ( Node and Controller ).

  • The group bypass also controls node bypass.
  • As for the advanced controls, we have 2 alternatives :
    • The group button controls the node buttons ( like bypass )
    • The buttons are independent. ( I think we coud do it this way ).

The FaceDetailer node could have advanced widgets hidden ( the bolt icon is white ).
And the group could also have other nodes hidden ( e.g. a Preview Image node, for the masked face, created by the Detailer ).

I may want to see the masked face image, without seeing all the FaceDetailer widgets.


ad1

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 19, 2024

Here is how it would look, with:

  • Advanced widgets hidden on the FaceDetailer node ( white bolt )
  • "Advanced nodes" ( e.g. Mask preview ) shown in the Controller ( blue bolt )

ad2

@JorgeR81
Copy link
Author

JorgeR81 commented Nov 29, 2024

@chrisgoringe, regarding this "multi-row" header feature, in terms of cost/benefit, this alternative approach may be better:

  • Have a global user setting, to show group buttons in multiple rows, if needed ( like before ) or abbreviate group names, in a single row ( like we do now ).

  • Use the same colors of the bypass/mute button, in the outline of the group button, to indicate the state of each group,
    ( red, purple, and maybe even the pale green color for the ? state).
    This way we can still see the state of non-selected groups, in the Controller.

If possible use a thicker outline, to make it stand out.
The outline would also be dimmer, if the group is not selected.
It could also have a thinner, black, interior outline to give some separation between the colors.


With this alternative approach, the only thing we "lose" it's the ability to change the state for non-select groups.
But that may be confusing for the user, anyways. So it may be even better not to have that option.

And we also have other advantages:

  • This saves header space, in some cases,
  • and I think it's even better at indicating the state for each group, at a glance.

mockup

Group1 selected
Group1 and Group2 bypassed

g1


Group1 selected
Group1 and Group2 muted

g2


Group1 selected
Group3 bypassed

p001


Group1 selected, with some nodes muted/bypassed
Group2 bypassed
Group3 muted

p002

@JorgeR81
Copy link
Author

JorgeR81 commented Dec 6, 2024

I just noticed that Youtube now has a similar look for its buttons.

When the narrator tells us to like the video, the like button it's highlighted in a similar way.

bt

@JorgeR81
Copy link
Author

JorgeR81 commented Dec 10, 2024

With support to Fast Groups Mutter, and Fast Groups Bypasser ( #428 (comment) ), the more complex version of multiline header it's not necessary anymore.

Users that require complex combinations of group mute / bypass, can add these nodes to a separate Controller window.

The only thing we really need, it's an user option to show the full group name ( in multiple lines ), like before.

And, if it's not too hard to implement, button highlighting ( for the mute/bypass states ), would also be cool.

@JorgeR81
Copy link
Author

For the sake of simplicity, I think we can now close this issue, because:

g1

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

No branches or pull requests

4 participants