-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Switch the version of litegraph
used to litegraph.ts
#1310
Conversation
Wanted to include this idea/note I posted on another issue here; RE: litegraph bugs/limitations and a potentially better alternative:
|
Agreed, I once tried to implement a script workflow using litegraph, but its lack of customization and well-documented API made it a bad development experience that I had to give up. Then I used xyflow, which did almost all of my customization needs, along with a modern front-end development experience. |
@YunYouJun One thing that litegraph seems to be able to do is to 'execute' the graphs; but in my exploration of xyflow / reactflow / etc, I haven't yet come across a good simple example of how to 'execute' a graph made with them. Do you have any good examples of this; or is it more of a 'manually implement based on your specific needs' sort of thing? Originally I would have expected that for each node I could implement something like a 'transformer' function, that would then process the data flowing through the graph; but I'm thinking maybe that's not the case? ChatGPT seems to suggest that it's up to us to implement the execution flow separately: Details
|
I used xyflow api to implement a set of task nodes execution mechanism by myself. I used mitt to build an event management mechanism. When the node completes, the event information about the completion of the node is emitted. When we click This is an example can preview: |
@YunYouJun Ah true. That makes sense. Thanks! :) This was another node UI project + graph execution project I came across that looks pretty interesting/useful as well:
|
Cool, it looks more like it was designed specifically for node workflows. Perhaps using it directly can save some development time. Unfortunately, I already have a lot of customization for xyflow, which gives me no incentive to try it in the short term. |
@YunYouJun is your example open source? Would love to look into it in more depth. |
Sorry, it's still in development and I'm not ready to open source it just yet. |
This would need a total rebuild against current code before it could be mergeable |
This PR is for migrating the version of LiteGraph used by ComfyUI to litegraph.ts.
NOTE: There will be some amount of incompatibilities with existing frontend extensions with this approach, I would encourage testing them to see what has to be changed before deciding this is a good idea. All core extensions have been migrated already. And of course please test your existing workflows because something will probably break sooner or later!
No compilation is needed if you check out this PR, the intention is to have
litegraph.core.js
precompiled in this repo and tracklitegraph.ts
changes in a separate repo (no submodules). Also I left ComfyUI using JavaScript to avoid compilation and keep things simpleBenefits:
litegraph.ts
to those in the ComfyUI development community that want to help support the library into the futureextends
. (When compiling for use with ComfyUI it outputs a single .js file like before)ContextMenu
constructor in litegraph.litegraph.ts
was fixed. Since litegraph's implementation of subgraphs depends on its own internals, using that implementation would mean modifying litegraph itselflitegraph.ts
/ComfyBox is the ability to add graph-internal logic using node-based events. So there wouldn't be a need to implement branching nodes on the backend to have conditional workflows anymore. This can open up a lot of future possibilities like conditionally turning off nodes based on workflow state, firing notifications when jobs finish, and so onDrawbacks:
litegraph
, and the codebase oflitegraph.ts
has diverged significantly from upstreamimport
statements to the top of each file sinceLiteGraph
and its classes are no longer globals. The changes are relatively minor from what I've handled so far and all core extensions were migrated save for any bugs I missedlitegraph.ts
requires a TypeScript toolchain and a compilation step (but not for any changes local to ComfyUI's own JavaScript)To build
litegraph.ts
for ComfyUI:comfyui
branchpnpm install
in the root directory oflitegraph.ts
bash build.sh
withgit-bash
or similarout/litegraph.core.js
toComfyUI/web/lib
and replace the existing versionThe version of
litegraph.core.js
in this PR is precompiled to match the latest changes