[feat] Some improvements to dynamic inputs (and proposal for contribution)

[pterrien]

Note

This was implemented in #222

Describe the problem

Currently the dynamic inputs work well, but could be slightly improved with the following features:

• custom separator, instead of default _ (even empty '')
• custom start_index, instead of default 1 (I personally prefer 0, but I understand most people don't)
• allow using non-dynamic / static inputs in addition to dynamic ones

Describe the solution you'd like

I have implemented some light modifications to your code (setupDynamicConnections() and dynamic_connection() in web/comfy_shared.js) to handle the cases above:

• modify the options to add separator and start_index
• allow static inputs to coexist with dynamic ones, with the following drawbacks:
  • input order is not managed, meaning that static ones do not always appear at the top of inputs
    • but I guess dealing with input order would be far more complicated (recreate links?)
  • possible undesired behavior if a static input starts with the same prefix as dynamic ones and there's no separator (eg argument vs arg)
    • because I use a simple startsWith() to filter dynamic ones
    • this could be improved by using a regexp, with a possible (minor) impact on performance

Alternatives considered

No response

Additional context

Salut !

First, a big thank for your work!
Although rather new to Comfy, I got interested with dynamic inputs for my own / personal nodes, so I first discovered the cozy-comfyui repo (very nice introduction), then came across yours which proposes a more advanced solution.
And I must say I'm impressed, except a very minor issue on manual disconnection (totally acceptable), it works perfectly!

I implemented the tiny QoL improvements above for my own use: the first one (separator / start_index) is pretty straightforward, the second one a bit more debatable, as not dealing with input order may confuse users a bit.

No revolution here, they are very simple, but if you think they could be interesting, just tell me and I'll be happy to contribute.
Either by describing the changes in this issue, or even proposing a PR if you want.

Thanks again for sharing your work, cheers!

[pterrien]

I checked setting labels, and could bring another very minor improvement (they are currently discarded).
I may also need some explanation about the nameArray option, as I do not know what it is and want to properly test that, too.

[melMass]

Hi,

Thanks for the write up!
Those are great improvements, mixing static and dynamic is something I once solved but it stopped working at some point, I'd love to bring that back! I'm completely open for a PR!

• nameArray? This is a setting I use in some unpublished mtb nodes to define the dynamic names sequentially.... I use it in my CosyVoice fork too: dynamicAlphabet, where it uses the alphabet (A,B,C,D...) as dynamic input names. Another use case is multi axis stuff, so you could pass [x, y, z, w]: when connecting x, y pops up, when connecting y, z pops up etc etc...

370914061-25630131-2f4a-45bc-80e8-9b5acaf2f1c9.mp4

[melMass]

Oh and FYI I have a lot of unpublished code (mostly js/frontend stuff) that I plan to organize better: use a bundler, probably switch to typescript....
The org just released the types that I've requested ever since the new UI, this will improve DX by 10 for me: https://www.npmjs.com/package/@comfyorg/comfyui-frontend-types

[pterrien]

Great! So I will work on a PR (my first one...), you can expect something by the end of this week the latest.
So to summarize, I will implement all features above (include labels). Regarding nameArray, my modifications should have not impact on it, but I will do my best to ensure this.

And BTW: I'm more comfortable with TS than JS (and I surely prefer good typings), so in the future if you need some help, don't hesitate to ping me. That would be a good exercise for me to understand Comfy better.

[pterrien]

Hi @melMass,
All my sincere apologies, I had some personal issues and couldn't match the deadline I promised.
But that should be better in the following days, in any case the base code is ready, just need to merge it and properly test it with yours.
BTW: how do you want me to provide a test base? Should I add some other Debug nodes (with static inputs for example), and if so as a separate branch not included in the PR? Or should I just ignore that part, and let you test the changes on your own?

[melMass]

No pressure at all!
Thanks.
I think for this nothing beats manual testing unless we have a good "framework" to do so, I'm starting to add tests for Python progressively, but canvas tests are quite involving. The comfy org uses a very slick test pipeline I want to take some inspiration from at some point: https://ci.comfy.org/

[melMass]

(That said it can be also tested with a workflow showing various cases we can try to break)

[pterrien]

Hi @melMass ,

Once again, all my sincere apologies for the time I took to push this PR, but here it is at last! -> #222

The 3 improvements described above have been implemented. You may want to check the JSDoc syntax (not really used to), for the rest it should be good I think.

Note 1: sorry, my prettier config may have slightly messed up with yours, so you may have to re-save the file I'm afraid...

Note 2: my code currently relies on startsWith to identify the dynamic inputs. In some rare specific cases (e.g. if dynamic inputs are prefixed with arg and there is one static input named argument), this may cause issues.

• we could improve that by using a regexp (which won't be perfect but better)
• but this is not very efficient performance-wise, as we'll have to recompile the regexp at each function call

We talked about testing, and I could propose a node you may find interesting. It's a simple string formatter (using Python string.format()), taking any number of inputs, with a single string output:

• connect your inputs (can be string, int, float...)
• setup your template using placeholders; either by input name ({arg0}), or by index ({0})

Example with the template string: I saw a {arg0}, wearing a {1} coat, in a {arg2}:

(do not focus on the inputs, these are custom and not relevant here)

I hope you'll find this PR interesting. Do not hesitate if you want me to update this PR with the regexp, or to create a new one with the string formatter.

Cheers!

[melMass]

Nice thanks for the write up!
And really no worries about the timing.

For point 2 I think it's not a huge constraint to have, you just have to be aware of it!

Regarding testing I will already test all the dynamic nodes of mtb, in case they broke, there is quite a few so it should be helpful. But yes maybe it's worth making nodes using the new options you proposed, I'll prepare that tomorrow.

And btw, it's poorly named but Apply Text Template is just that:

comfy_mtb/nodes/graph_utils.py

Lines 87 to 116 in e5482ae

	# class MTB_ApplyTextTemplate:
	class MTB_ApplyTextTemplate:
	"""
	Experimental node to interpolate strings from inputs.
	Interpolation just requires {}, for instance:
	Some string {var_1} and {var_2}
	"""
	@classmethod
	def INPUT_TYPES(cls):
	return {
	"required": {
	"template": ("STRING", {"default": "", "multiline": True}),
	},
	}
	RETURN_TYPES = ("STRING",)
	RETURN_NAMES = ("string",)
	CATEGORY = "mtb/utils"
	FUNCTION = "execute"
	def execute(self, *, template: str, **kwargs):
	res = f"{template}"
	for k, v in kwargs.items():
	res = res.replace(f"{{{k}}}", f"{v}")
	return (res,)

[pterrien]

Maybe easier than a PR, here is the code for the string formatter node, as you can see it's dead simple:

class FormatString:
    def __init__(self):
        pass

    @classmethod
    def INPUT_TYPES(s):
        return {
            'required': {
                'format': (
                    'STRING',
                    {
                        'default': 'my first arg: {arg0}, second by index: {1}',
                        'multiline': False,
                    },
                ),
            }
        }

    @classmethod
    def VALIDATE_INPUTS(s, input_types):
        for t in input_types.values():
            if t not in ['STRING', 'INT', 'FLOAT', 'BOOLEAN']:
                return False
        return True

    CATEGORY = 'ChangeMe'

    RETURN_TYPES = ('STRING',)
    RETURN_NAMES = ('formatted',)

    FUNCTION = 'run'

    def run(self, format: str, **kw):
        # handle both index and name references
        output = format.format(*kw.values(), **kw)
        return (output,)

With the following JS code, in beforeRegisterNodeDef():

setupDynamicConnections(nodeType, "arg", "*", { separator: "", start_index: 0,});

[melMass]

Ah yours looks better! nice

[pterrien]

Hi @melMass ,

While importing my previous nodes into my new collection (soon to be shared publicly), I came across a bug with the FormatString node above, when setting 'multiline': True.
The reason is that a multiline widget is in fact a DOMWidget, that works differently than standard ones. You can check addMultilineWidget and node.addDOMWidget in the ComfyUI_frontend repo.

I found a solution to this, basically adapting a trick I used in other nodes, where I need to dynamically show / hide a multiline preview. It consists in storing the actual node size, let dynamicConnection() do its job, then resetting the widget positions, and finally recalculating the size: newHeight = oldHeight + newMinimumHeight - oldMinimumHeight.

Unfortunately, I cannot propose a PR, as I switched to Typescript, but if you're interested, here is the code, in nodeType.prototype.onConnectionsChange:

// replace the call to dynamicConnection with this

// keep a track of current size, and also the minimum / computed one
const oldSize = [this.size[0], this.size[1]]
const oldComputedSize = this.computeSize()

dynamicConnection(
	this,
	slotIndex as number,
	isConnected as boolean,
	`${prefix}${options.separator}`,
	inputType as string,
	options,
)

// reset positions of all widgets, they will be recalculated
for (const widget of this.widgets || []) {
	delete widget.y
	delete widget.last_y
	// computedHeight is used by the multiline widgets, it must be reset to force it to take all height
	delete widget.computedHeight
}
// get the new computed size, after adding / removing inputs
const newComputedSize = this.computeSize()
// recalculate the node size: preserve width, and increase / decrease height
this.setSize([
	oldSize[0],
	oldSize[1] + newComputedSize[1] - oldComputedSize[1],
])

return r

PS: I will soon ping you when my repo is created: still need to refine some details + a massive documentation effort. But most of the job is done, and I must say I'm pretty satisfied with the result, with many interesting UI tweaks (and the dynamic outputs work well! \o/).
This will probably be as my new super secret identity (@bugltd), don't be surprised.
In the meantime, if you need some help to migrate to TS, do not hesitate, I can share my env. I believe I found a pretty nice solution, that allows import rewriting and code watch, all automatically (without manually modifying the paths like rgthree does I think).

Once again a long post, sorry!

Hope everything is fine on your side,
Take care,

Pascal.

[melMass]

Thanks Pascal Bug ! I really appreciate those follow ups.
I was bugged by exactly this static widget alignement issue a few days ago so that's really insightful and will save me time to implement it!

For the ts setup I'm almost done locally but I'm still thinking about how to handle "bundling"...
For now I settled with a source branch that I will use as my main branch to work on and setup a CI to bundle TS from this branch into js in main... not certain though, still thinking about it! How are you planning to handle that yourself?

Excited to have a look at your extension when it's out!

[pterrien]

That's great, sincerely happy to help!

How are you planning to handle that yourself?

No CI or such on my side, I have both the TS and JS code in the repo, as I know many people still work with JS and will maybe want to be able to search / copy code from it (and the disk usage impact is negligible).
Although I thought about bundling (w/ Vite especially), it's probably totally overkill in this context, so I stay with the good old tsc and have really no complains in terms of perf.

For the watch / build part, here is an excerpt from my package.json:

"watch": "tsc-watch -p tsconfig.json --onSuccess 'tsc-alias -p tsconfig.json -r build/replacers.js/mock.js'",
"build": "rimraf ./dist/js && tsc -p tsconfig.prod.json && tsc-alias -p tsconfig.prod.json -r build/replacers.js/mock.js",

The trick is to use tsc-alias instead of the more common tsconfig-paths, because it allows having some custom path replacers, in addition to handling the aliases in tsconfig.json.
A tsc-alias replacer is a simple JS file like this one:

/**
 * Replace the paths within the '.mock' folder into the relative path expected at runtime.
 * We use a RegExp to achieve this, could be another approach
 */
const re = /[^'"`]*\/\.mock\//
export default function mockReplacer({
	orig,
	// file,
	// config,
}) {
	if (re.test(orig)) {
		// count the number of '..' before '.mock' folder
		const nbParents = (orig.match(/\.\.\//g) || []).length
		const replaced = orig.replace(re, '../'.repeat(3 + nbParents)) // 3 because of my code structure, you may have to adjust
		return replaced
	}
	return orig
}

And in the .mock/ subfolder of my TS code, I just add some dummy exports like this one (.mock/scripts/app.ts):

import type { ComfyApp } from '@comfyorg/comfyui-frontend-types'
export declare const app: ComfyApp

That's it! No CI burden, no custom python script, no problem! And a VSCode task to launch my watch script on workspace opening makes me an happy coder :)
This approach also allows me to add types that are missing in the official NPM packages (I opened some issues for that). To do so, I cloned the frontend repo and customized the build:types script (just a little, I promise).

Also, I wanted to say: it's thanks to you and the dynamic inputs trick that I discovered all this frontend part... and got totally contaminated... But I really enjoy it (even if I dislike LiteGraph)!
So many many thanks for that!