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

Convert widget to input (and back) #243

Merged
merged 6 commits into from
Mar 25, 2023

Conversation

pythongosssss
Copy link
Collaborator

Adds convert options to the node menu
image

And once converted, convert back
image

New primitive node that can be connected to one of these inputs
image

When connected it will copy the current value, numbers will be given a random toggle so it can be used for seeds
image

Links are restricted by type + widget configuration (e.g. you cant connect the same Primitive to seed and width, as width is restricted to steps of 64)

Primitives can be linked to multiple nodes to allow sharing of values

Values are sync'd to the source node, meaning things like LoadImage display the correct value
image

For now it supports text, numbers and combos but can easily be updated to support anything else
image

To quickly add a primitive widget you can double click the input (the circle, not the label)

If anyone has any feedback / could give this some testing as it was quite fiddly to get working!

@GrimblyGorn
Copy link

GrimblyGorn commented Mar 24, 2023

This seems quite handy so far. It took a minute to figure out how to merge the widgets.js file with my copy. Not sure if that's cause mine is newer/older, or from other things I may have merged previously. But, once I got past that it seems fine now.

The double click to add Primitives is very nice. Though I did notice that only works for inputs that were modified/converted. Seemingly, on any others it's not an option. If this could be extended to work for those as well it would be very nice.

Also, when they're double clicked to spawn and auto-connected, it would be handy if their names changed some to reflect that connection better. Such as converting steps to an input on the Sampler, then double clicking to make the Primitive for it, and maybe it pops out with "Steps (Primitive)" or something like that. Just another minor thing to save increments of time, over time. Maybe more of a personal preference too though.

Being able to dynamically shift these nodes from manual input to node inputs on everything is exceedingly more useful than the previous options. This should help reduce overlap on custom nodes being variations of what this achieves. For example, WAS and other, myself included. Have all made various versions of the KSampler already, to externalize various inputs. I have around 5-6 variations currently, mostly for testing. Lol, and with one quick ninja move, this makes those seem silly and less useful now :)

Very handy. Thanks :)

@comfyanonymous
Copy link
Owner

A tiny issue is that the width of the nodes gets resized when converting an input to a widget.

@GrimblyGorn
Copy link

A tiny issue is that the width of the nodes gets resized when converting an input to a widget.

Ya that does kind of bug me too a bit. Not a functional concern so much though. On mine, much of the code block below wasn't present with his, I think.

node.onResize = function (size) {
			if (node.widgets[0].last_y == null) return;
			node.onResize = function (size) {
			
				computeSize(size);
				let y = node.widgets[0].last_y;
				let freeSpace = size[1] - y;

				// Compute the height of all non customtext widgets
				let widgetHeight = 0;
				const multi = [];
				for (let i = 0; i < node.widgets.length; i++) {
					const w = node.widgets[i];
					if (w.type === "customtext") {
						multi.push(w);
					} else {
						if (w.computeSize) {
							widgetHeight += w.computeSize()[1] + 4;
						} else {
							widgetHeight += LiteGraph.NODE_WIDGET_HEIGHT + 4;
						}
					}
				}

				// See how large each text input can be
				freeSpace -= widgetHeight;
				freeSpace /= multi.length;

				if (freeSpace < MIN_SIZE) {
					// There isnt enough space for all the widgets, increase the size of the node
					freeSpace = MIN_SIZE;
					node.size[1] = y + widgetHeight + freeSpace * multi.length;
				}

				// Position each of the widgets
				for (const w of node.widgets) {
					w.y = y;
					if (w.type === "customtext") {
						y += freeSpace;
					} else if (w.computeSize) {
						y += w.computeSize()[1] + 4;
					} else {
						y += LiteGraph.NODE_WIDGET_HEIGHT + 4;
					}
				}

				this.inputHeight = freeSpace;

				// Call original resizer handler
				if (onResize) {
					onResize.apply(this, arguments);
				}
    			}
        	}
		requestAnimationFrame(() => {
			node.onResize(node.size);
			app.graph.setDirtyCanvas(true);
		});

Seemingly this handles resizing the height but not the width. Perhaps the issue could be resolved there somehow?

@pythongosssss
Copy link
Collaborator Author

The add/remove input functions automatically resize the node, i've updated it to store the original value and restore it (unless the new size needs to be bigger)

@pythongosssss
Copy link
Collaborator Author

pythongosssss commented Mar 24, 2023

The double click to add Primitives is very nice. Though I did notice that only works for inputs that were modified/converted. Seemingly, on any others it's not an option.

Yeah, it'll need to be manually implemented for each of the other types for what the default node would need to be

then double clicking to make the Primitive for it, and maybe it pops out with "Steps (Primitive)"

I've updated it so the new node is just the name of the original input, I don't think it needs primitive in the name

@GrimblyGorn
Copy link

Yeah, it'll need to be manually implemented for each of the other types for what the default node would need to be

I'll look into manually adding those as I can later then.

I've updated it so the new node is just the name of the original input, I don't think it needs primitive in the name

Ya that's even better actually. Thanks :)

@GrimblyGorn
Copy link

I didn't test the previous version for this but this new version is a bit click happy on this double click action. That's handy for spawning extras quickly but they should maybe disperse in an area around the first new Primitive, or in a vertical stack possibly. Currently, they stack neatly and you could get extra stacks going all over without noticing if you weren't careful. Not sure that's really an issue so much as an OCD thing though. Also, the Primitives seem to spawn in a crunched form, which is maybe another OCD thing.

I think for clarity and quicker visual referencing the naming in the menus should be more along the lines of "Externalize CFG"/"Internalize CFG" or possibly Extract & Absorb? :)

Something like would make it easier across more types too possibly.

@GrimblyGorn
Copy link

This could possibly use a little timer between spawning the Primitives so they'd be more likely intentional, than accidental. Something small like 50-100 milliseconds likely could work.

The vertical stacking or whatever would still be handy for intentional extras.

Throttle double click
@pythongosssss
Copy link
Collaborator Author

image
They'll now shift by the height of the titlebar if another node exists at the target position
Throttled double click to 300ms, less than that didnt feel like enough and still created "accidental" nodes

@GrimblyGorn
Copy link

This behaves much more reasonably now. On mine, it's stacking correctly but the starting alignment has it overlapping the input edge for the node in question as per the example image below.
2023-03-24 15_41_02-ComfyUI
That could be something to do with the browser I'm using too I suppose. I haven't looked at the code for this much yet to see which it might be.

Also, just to show a visual example of the re-naming I mentioned above here's this too:
2023-03-24 15_40_28-ComfyUI
It could be other things like I mentioned above but I think this gives it quicker navigation. Basically then, you can mentally say E is Out and A is In. Then each time after, just go to E or A and pick your option. Gives it a tree structure of sorts.

@GrimblyGorn
Copy link

Now that I've played with this some more, it seems like it would be useful to have the initial Convert to Input/Extract also actually spawn a primitive when you select it. Personally, it seems more likely I wouldn't have a node already setup to make use of the new input, so having one made available is likely useful more often than not.

The double click function is still useful after, if you clone the newly extended node and want separate primitives for those connections.

Then you could spawn the node, extend it with auto-spawn primitives, clone it, and connect all those primitives, or double click to spawn new ones as needed. Seems way more fluid possibly.

@WASasquatch
Copy link
Contributor

WASasquatch commented Mar 25, 2023

Related to this, you need to make sure node inputs accept their type, and *. It works for inputs, but not when evaluating the input where "INPUT_TYPE" != "*".

This is input for wildcard stuff, like a debug to console which will print anything to console (in my node Suite) and then allow you to use it as a pass-through as well.

@pythongosssss pythongosssss deleted the widget-inputs branch March 26, 2023 11:19
mid-dev-media pushed a commit to mid-dev-media/ComfyUI that referenced this pull request Mar 16, 2024
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

Successfully merging this pull request may close these issues.

4 participants