rgthree-comfy icon indicating copy to clipboard operation
rgthree-comfy copied to clipboard

Missing a possibility to convert widgets to inputs.

Open schoenid opened this issue 10 months ago • 13 comments

On automated tasks it is required to have inputs, which can be set by operatars from the calculation of node outputs. As example, if a checkpoint model is of type 'Flux', the following Lora chain needs a possibility, to be disabled, if of a different type like SD15 or enabled, if of the same type. A lora chain of a different type will not work, throws errors and slows down the system.

Image The picture shows the problematic, where a Flux checkpoint has been loaded, but the SD15 Lora chain needs to be disabled manually

Standard nodes in ComfyUi do have the possibility, to convert widgets to inputs.

It would be nice, if "rgthree-comfy" nodes would also have this possibility, as example for the 'Toggle All' widget in 'Power Lora Loader', or for each toggle widgets in 'Fast Muter' and 'Fast Bypasser' (or at least to have a convertable 'Toggle All)' for these).

This is a feature request.

schoenid avatar Feb 26 '25 14:02 schoenid

The alternative is, to use a lot more nodes, than wold be really required, as if in the example the "Power Lora Loader" would have the possibility, to convert the 'Toggle All' widget to a boolean input:

Image

The 'Control Bridge' before and the two 'Any Switch'es after the 'Power Lora Loader' could be left away.

schoenid avatar Feb 26 '25 14:02 schoenid

With Control Bridges it's possible to get a 'Disable' of the Lora Loader without switches on the output. The Outputs can be chained to the next stages input. But it looks not so orderly. It would better suite a horizontal arrangement.

Image

With switches it's possible to get more order, but more nodes are required. The Control Bridges have to be configured for 'Mute'.

Image

So if a "Toggle All" input should be realized, also an option widget for muting or bypassing would be helpful (not configured as input).

schoenid avatar Feb 26 '25 17:02 schoenid

Another thing are 'Fast Muter' and 'Fast Bypasser'. I would like to mute or bypass the "Dual Clip Loader" and the "VAE Loader" if these are not used, because I've got loading times for both, even if these are not used. I've checked this by manually mute it. If these are in a mute state, the whole process starts faster.

Image

schoenid avatar Feb 26 '25 17:02 schoenid

As I realized today it's not possible to use Control Bridges to turn off the Lora Loaders. The problem is with delays, which forces the UI to skip the job and going into an uncontrollable state, by adding more queues and others.

Image

So the only possibility I found at the moment to have 3 kinds of Lora types is to load them all and to select the one, which is required, but causing a lot of warnings in the console.

Image

schoenid avatar Feb 27 '25 11:02 schoenid

So if a "Toggle All" input should be realized, also an option widget for muting or bypassing would be helpful (not configured as input).

Muting or bypassing is not required, if set to off. I've realized, that the signals are sloped trough, then it's possible to chain all Lora Loaders together.

schoenid avatar Feb 27 '25 13:02 schoenid

Momentary situation:

Image

A relatively large framework is required, to select between different Loras (each for one type of checkpoint)

schoenid avatar Mar 01 '25 10:03 schoenid

With a small changement, the situation is more optimized:

Image Looks much better, isn't it?

The difference: Just to include in the node "enable": ("BOOLEAN", {"defaultInput": True}),

As example:

class RgthreePowerLoraLoader:
  """ The Power Lora Loader is a powerful, flexible node to add multiple loras to a model/clip."""

  NAME = get_name('Power Lora Loader')
  CATEGORY = get_category()

  @classmethod
  def INPUT_TYPES(cls):  # pylint: disable = invalid-name, missing-function-docstring
    return {
      "required": {
        "model": ("MODEL",),
        "clip": ("CLIP",),
        "enable": ("BOOLEAN", {"defaultInput": True}),
      },
      # Since we will pass any number of loras in from the UI, this needs to always allow an
      "optional": FlexibleOptionalInputType(any_type),
      "hidden": {},
    }

  RETURN_TYPES = ("MODEL", "CLIP")
  RETURN_NAMES = ("MODEL", "CLIP")
  FUNCTION = "load_loras"

  def IS_CHANGED(self):
    return float("nan")

  def load_loras(self, model, clip, enable, **kwargs):
    """Loops over the provided loras in kwargs and applies valid ones."""
    for key, value in kwargs.items():
      key = key.upper()
      if key.startswith('LORA_') and 'on' in value and 'lora' in value and 'strength' in value:
        strength_model = value['strength']
        # If we just passed one strtength value, then use it for both, if we passed a strengthTwo
        # as well, then our `strength` will be for the model, and `strengthTwo` for clip.
        strength_clip = value['strengthTwo'] if 'strengthTwo' in value and value[
          'strengthTwo'] is not None else strength_model
        if enable:
          if value['on'] and (strength_model != 0 or strength_clip != 0):
            lora = get_lora_by_filename(value['lora'], log_node=self.NAME)
            if lora is not None:
              model, clip = LoraLoader().load_lora(model, clip, lora, strength_model, strength_clip)

    return (model, clip)

It works, but there are some small problems if a node is freshly loaded and not yet configured.

Probably the JS has to be dapted too. (... which I can't, because I do understand absolutely nothing, how this works.)

schoenid avatar Mar 01 '25 10:03 schoenid

Sure, some things are missing, like if no input is connected, it should default to "Enable" (true). Like it is at the moment, a fresh node has an "enable" widget, altough the {"defaultInput": True} should configure it as input. Sure, it can be used too and can be converted. I don't know, how to declare it as optinal input, because it conflicts with the already defined flexible input definition.

schoenid avatar Mar 01 '25 10:03 schoenid

The Input can be defined as "enable": ("BOOLEAN", {"forceInput": True}),, this solves the unexpected behaviour and provides an input instead of a widget, but prevents from convert it to a widget.

ref: https://github.com/comfyanonymous/ComfyUI/pull/318

schoenid avatar Mar 02 '25 09:03 schoenid

Hey Daniel. Thanks for diving into this. This is an interesting problem though, admittedly, it starts to bloat the purpose of the node quite a bit (my nodes are actually meant to make ComfyUI more comfortable, not necessarily more powerful, even if they sometimes go hand in hand).

The problem with trying to reuse the existing Power Lora Loader's toggle lies in the fact that it's a client-side toggle which executes its logic before sending the workflow to the client. There's no easy way to manipulate the workflow while it's running. Even the "Control Bridge" is just really a hack: It stops the execution completely, then manipulates the UI to mute/bypass/enable, then re-sends the entire workflow again hoping everything prior will be cached by the backend (making it seem like it's doing it all in the same execution).

Now, you have solved this issue there with your "enabled" input idea. However, I don't think I would put it in the Power Lora Loader since it starts to conflate what "enabed" means. Currently, there's only one layer of "enabled" which is the current UI status of enabled/mute/bypass. Introducing a new way to "bypass" a node via an input but which is not the way any other node in the system does it starts to feel more confusing and less comfortable.

Now, I did have a pretty cool, robust, UI automation node that would have solved this but, unfortunately, I lost it before making it to the git repe a long while back. Maybe I'll try to bring it back.

rgthree avatar Mar 02 '25 21:03 rgthree

... make ComfyUI more comfortable ...

Now, you have solved this issue there with your "enabled" input idea. However, I don't think I would put it in the Power Lora Loader since it starts to conflate what "enabed" means.

You're right. The "enabled" input idea is a bit bustling, because of the many "Toggle" options ...

Just as idea too ... It would help, if the "Toggle All" option could be converted to an input, because this is pratically the same.

I see the complications, because it's lying in the client-sides code and not in the Python script. The "Toggle All" should only be visible, if any Lora has been added, so it's difficult to select it for an input conversion before. Just some thoughts: Is it possible to hide an input widget on the UI, but keep it's ability for conversion on the context menu (I didn't find a possibilty yet ...)?

schoenid avatar Mar 26 '25 17:03 schoenid

Another possibility would be, to add a selection for different sd versions, like in the example below.

Image

The sd version is already available trough the model, like in the following snippet:

    sd_version = kwargs["sd_version"].upper()
    motype = model.model.latent_format.__class__.__name__
    if sd_version == "SD1": sd_version = "SD15"
    if motype == "FLUX": motype = "OTHER"
    if sd_version != "ALL":
      if motype != sd_version: 
        return (model, clip)

The sd_version could also trigger the main "OFF".

The problem I have with the Power Lora Loader is, that I'm often need to switch between SD15, SDXL and FLUX. Then the Loras have to be loaded from different sources. Loading the wrong kind of Loras leeds to errors. So I need a logic, to select the 'right' Lora Loader and this is everthing else than comfortable.

schoenid avatar Apr 23 '25 19:04 schoenid

Or even better: You've got already the Lora info built in. Why not implement an option in the settings, for an automatical filtering by sd version?

schoenid avatar Apr 23 '25 21:04 schoenid