rgthree / rgthree-comfy

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

Node style overridden #194

Closed Derjyn closed 6 months ago

Derjyn commented 7 months ago

It seems rgthree has an opinionated override on node styling. I have an extension that defaults node shape to box, but rgthree has it's own style set and nukes that.

It's not a big deal, but with dozens of custom nodes installed and none of the others take this liberty, it'd be great to see rgthree adhere to our own custom stuff rather than taking priority.

rgthree commented 7 months ago

I wouldn't say it's that opinionated; at least not on node shape. It's possible some things are assumed to be using ComfyUI's default styles, but that's only because that's mostly the view I use when building. But as far as node shape goes, that's not set anywhere, nor do I have an opinion on what's used.

It sounds to me like this extension that modifies node shape is simply not compatible with rgthree-comfy itself. That could be the extension's issue, or rgthree-comfy's; hard to say without more info.

The reasons why "dozens of custom nodes" don't change much in the UI is because most are not much more than a simple python file that register a few nodes in that backend to leverage some new modeling or functionality during backend prompt execution. In contrast, rgthree-comfy tries to enhance the in-app experience to make it easier and more fun to use and unlock powerful mechanics to build complex, interoperable workflows. Unfortunately, a lot of Comfy's shipped UI code is incredibly rigid and built on cornered assumptions which make it very hard to do that in some cases. Fast Groups Muter, Reroute, Image Comparer, Power Lora Loader, etc. could never cleanly work in ComfyUI without breaking out of those rigid assumptions.

With that, I'm not too surprised that an extension that attempts to change node shapes across other custom nodes doesn't work with these more advanced nodes. But, I don't have an opinion on what shape is set. If you'd like to share this extension I'm happy to take a look.

Derjyn commented 7 months ago

This is a common script/extension present in several other extensions/custom node packages. For example:

https://github.com/blib-la/blibla-comfyui-extensions

However, I just made a quick *.js script in the ./web/extensions/ folder, box_default.js:

import { app } from "../scripts/app.js";

app.registerExtension({
    name: "DefaultToBox",
    nodeCreated(node) {
        node.shape = 1
    }
});

That's it, super simple. Some other scripts go crazier (mainly to expose toggling features), but the basic logic is the same. It seems if this script isn't working on a given set of nodes, it's because they are overriding it somewhere in their codebase. Could be a matter of order of operations or some other "thing", but at any rate, thought I'd bring it up.

Monolithic custom node packages seem to be the ones that have this "issue", which makes sense. Things get bloated, SC fades away, AT gets put off, etcetera. Eventually a snippet of code that does some random bit sits in a dark alley behind a can, forgotten and unloved.

rgthree commented 7 months ago

Ah, yea; that's the problem. ComyUI invokes that nodeCreated from it's own "ComfyNode" class. But, as I alluded to before, rgthree-comfy has been forced to avoid that implementation because of the instability between ComfyUI pushes and general inflexibility. (ComfyUI is much closer to that monolith you speak of).

Thanks for pointing this out, though. While avoiding ComfyNode is intentional, it's not the intention to disallow scripts like this, and could be possible for rgthree-comfy nodes that avoid the unstable ComfyNode to try to invoke the nodeCreated event just the same.

Derjyn commented 7 months ago

It seems then, that the platform rgthree is base upon, doesn't have the most documented/stable API when it comes to extensibility, haha... It seems this is what we get for API docs:

https://github.com/comfyanonymous/ComfyUI/tree/master/script_examples

Anyhow, thanks for humoring my OCD. It's a trivial thing, but it pulled on a string that brought something to my attention, potentially hinting at bigger underlying issues. Seems it's just a tumbleweed in the wild west!

rgthree commented 6 months ago

Alright, refactored a bunch so extensions' nodeCreated would run as expected on rgthree-comfy nodes.

I did find this funny: that example script you provided doesn't even work on all of ComfyUI's own nodes, like Note and Reroute (for the same reason rgthree-comfy nodes didn't).

It's a pretty big refactor and I did notice a bunch of issues, even having to block some of Comfy's incompatible extensions, but should be for the better in the end. We'll see if anyone's workflows breaks, might have to revert if that's the case.