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

After updating Comfy, reroute nodes don't look right.

Open artbymbesares opened this issue 9 months ago • 25 comments

I just updated ComfyUI, but the reroute nodes now look like this (I attached an image to show what I mean). I updated the nodes too but it still looks the same. Any idea why?

Image

artbymbesares avatar Mar 21 '25 16:03 artbymbesares

same...

murphylanga avatar Mar 21 '25 17:03 murphylanga

Bummer. Not too surprising as Comfy continues to improve their codebase the risk of breaking rgthree-comfy increases.

Can you file an issue with comfy frontend for visibility?

rgthree avatar Mar 22 '25 00:03 rgthree

same

Flow-two avatar Mar 22 '25 05:03 Flow-two

I posted it as a discussion in the Comfy frontend and added some information. Please support it so it gets noticed. https://github.com/Comfy-Org/ComfyUI_frontend/discussions/3186

Um, I know it's a different topic, but rgthree, take a look at Physssss's thumbnails when you get a chance and see how they solved the problem with subdirectories. Maybe you can adopt it in your Lora Powerloader. At the moment, I have to disable the subdirectory function for you so that it works in Physssss Loader.

murphylanga avatar Mar 22 '25 08:03 murphylanga

Video for temporary fix, and how you can support the tip for support to the Comfy UI frontend team. https://youtu.be/LhwuFXLFzMY

murphylanga avatar Mar 22 '25 17:03 murphylanga

After the recent v0.3.27 update, many other rgthree nodes broke

futuranewworld avatar Mar 22 '25 18:03 futuranewworld

After the recent v0.3.27 update, many other rgthree nodes broke

I was kind of afraid of something like that. If the developers aren't careful, many will turn away from Comfy, and their excellent work might only be noticed by a few people. That would be a real shame. Then there would be a lot of "offshoots" instead of one large, unified community.

murphylanga avatar Mar 22 '25 19:03 murphylanga

Let's see what happens. The request has already been closed with the following answer: https://github.com/Comfy-Org/ComfyUI_frontend/pull/3151

murphylanga avatar Mar 22 '25 19:03 murphylanga

Ok, things getting worst for frontend 1.15.0 reroute nodes make the workflow load fail. The JS code is line 301 column 87 of reroute.js, the type property doesn't exist. The code needs to check if the type exist. I manually disabled the code to load the workflow. The new reroutes renders the rgthree reroutes obsolete.

set-soft avatar Mar 24 '25 11:03 set-soft

Ok, things getting worst for frontend 1.15.0 reroute nodes make the workflow load fail. The JS code is line 301 column 87 of reroute.js, the type property doesn't exist. The code needs to check if the type exist. I manually disabled the code to load the workflow. The new reroutes renders the rgthree reroutes obsolete.

https://github.com/rgthree/rgthree-comfy/issues/449

huchenlei avatar Mar 24 '25 15:03 huchenlei

Ok, things getting worst for frontend 1.15.0 reroute nodes make the workflow load fail. The JS code is line 301 column 87 of reroute.js, the type property doesn't exist. The code needs to check if the type exist. I manually disabled the code to load the workflow. The new reroutes renders the rgthree reroutes obsolete.

#449

I understand it, the problem is the change in the structures, the type removed. A similar problem affects "Anything Everywhere" nodes. Migrating to the native reroutes is a good idea. Removing the classical reroute nodes is a bad idea, the native nodes doesn't fully replace the reroute nodes. Changing the data structure in a minor release is also a bad idea.

set-soft avatar Mar 24 '25 15:03 set-soft

Ok, things getting worst for frontend 1.15.0 reroute nodes make the workflow load fail. The JS code is line 301 column 87 of reroute.js, the type property doesn't exist. The code needs to check if the type exist. I manually disabled the code to load the workflow. The new reroutes renders the rgthree reroutes obsolete.

#449

You should definitely keep RGthree's custom redirect nodes running. These custom nodes are always worth a look. They offer many useful features for the proper management of large workflows. They also provide the ability to disable or bypass groups and nodes with the click of a mouse and then automatically route the desired items to the routing nodes. I consider these custom nodes to be among the best there are in the Comfy world, and I know a lot of them. I have workflows with over 2000 nodes that you can operate step by step, use SDXL and Flux, Kolors, etc. together to edit images, switch on optimizations one by one, etc. Since your optimization work, more and more functions have been lost. Fortunately, I have kept an old version with which everything works perfectly. In my opinion, rushing optimizations is not a good idea. I can sense the mood of the users. Instead of inspiring more people, more and more are turning away in frustration and annoyance. Take a look at all the great things that have been developed by the community and try to adopt some of them instead of just pushing your own ideas like a company, without thinking outside the box.

Image

murphylanga avatar Mar 24 '25 16:03 murphylanga

Just a note: to avoid the crash when loading a workflow using rgthree reroute I applied the following patch:

diff --git a/web/comfyui/reroute.js b/web/comfyui/reroute.js
index 316b64f..49583a7 100644
--- a/web/comfyui/reroute.js
+++ b/web/comfyui/reroute.js
@@ -298,7 +298,10 @@ class RerouteNode extends RgthreeBaseVirtualNode {
     }
     onConnectionsChange(type, _slotIndex, connected, _link_info, _ioSlot) {
         if (connected && type === LiteGraph.OUTPUT) {
-            const types = new Set(this.outputs[0].links.map((l) => app.graph.links[l].type).filter((t) => t !== "*"));
+            const types = new Set(this.outputs[0].links
+                                  .map((l) => app.graph.links[l])
+                                  .filter((link) => link && link.type !== "*" && link.type !== undefined)
+                                  .map((link) => link.type));
             if (types.size > 1) {
                 const linksToDisconnect = [];
                 for (let i = 0; i < this.outputs[0].links.length - 1; i++) {

This skips links with undefined type.

set-soft avatar Mar 24 '25 17:03 set-soft

I understand it, the problem is the change in the structures, the type removed.

@set-soft are you able to upload a workflow that is failing, so we can take a look? LLink.type should never be undefined.

The motivation for moving to native reroutes is relatively straightforward. Using nodes to perform the function of a reroute was a great workaround to a very common issue, however it does not scale with features, and it is not sustainable.

The issue is that instead of a single link from the input to the output, all the segments added by reroutes have their own individual links. This complexity constrains everyone when adding new features, as any new features must now account for a network of links.

p.s. Many issues have been resolved in the last few days, e.g. linear & straight link modes. Work is still continuing on this. Please feel free to open issues for any specific regressions that come up.

webfiltered avatar Mar 24 '25 18:03 webfiltered

I understand it, the problem is the change in the structures, the type removed.

@set-soft are you able to upload a workflow that is failing, so we can take a look? LLink.type should never be undefined.

Seems to happen during the reroute migration, I get:

Image

Here is one failing:

Wan_I2V_3xUpscale_Frame_robotic_auto.json

The motivation for moving to native reroutes is relatively straightforward. Using nodes to perform the function of a reroute was a great workaround to a very common issue, however it does not scale with features, and it is not sustainable.

The issue is that instead of a single link from the input to the output, all the segments added by reroutes have their own individual links. This complexity constrains everyone when adding new features, as any new features must now account for a network of links.

The native reroutes are very interesting, but I think the "node reroutes" has a particular use that isn't covered by native reroutes: using them as "i/o ports" for node groups. I guess this will be less important once sub-graphs are available, but I like to put reroutes at the edges of groups, so moving a group, or reusing it, becomes cleaner.

p.s. Many issues have been resolved in the last few days, e.g. linear & straight link modes. Work is still continuing on this. Please feel free to open issues for any specific regressions that come up.

Ok

P.S. I found problems with "Anything Everywhere" nodes, trying to figure out the source of the problem

set-soft avatar Mar 24 '25 18:03 set-soft

With the RGthree redirections you can control the direction of input and output. (This only works up to Frontend 1.12.11). Or, for example, to separate manually between two route accounts. For example, to get prefabricated templates into a workflow and simply integrate them (you can use them to define labeled docking points). I can also load workflows with the RGthree accounts with v1.15.0, but as already written, twisting the inputs and outputs at the reroute nodes no longer works.

murphylanga avatar Mar 24 '25 20:03 murphylanga

using them as "i/o ports" for node groups

@set-soft To my eyes, the simplest solution here would be to just have a node that does this. It doesn't sound like it must be a reroute node.

@murphylanga Native reroutes automatically swivel to the direction of the link. https://github.com/Comfy-Org/litegraph.js/pull/846 is also currently being trialled (not finalised and not remotely optimised).

webfiltered avatar Mar 25 '25 08:03 webfiltered

using them as "i/o ports" for node groups

@set-soft To my eyes, the simplest solution here would be to just have a node that does this. It doesn't sound like it must be a reroute node.

I agree it doesn't have to be a reroute, but I think some sort of node should be available for this. In fact a triangular node that just bypasses its input to the output will be better. As I'm an electronics engineer a "port" (rectangular input and triangular output) is the ideal. But the main point is: if classic reroutes are removed, some sort of node that can be used as intermediate point should be available.

@murphylanga Native reroutes automatically swivel to the direction of the link. Comfy-Org/litegraph.js#846 is also currently being trialled (not finalised and not remotely optimised).

My 2 cents: I migrated a couple of workflows to the native reroutes and they work well to replace rgthree reroutes, at first sight they look easier to use, will see after more use. One thing that I miss: sometimes is hard to get straight lines because the curve is affected by the previous segment, so it looks like extra reroutes are needed just for aesthetic purposes. Nothing really important.

set-soft avatar Mar 25 '25 12:03 set-soft

using them as "i/o ports" for node groups

@set-soft To my eyes, the simplest solution here would be to just have a node that does this. It doesn't sound like it must be a reroute node.

@murphylanga Native reroutes automatically swivel to the direction of the link. Comfy-Org/litegraph.js#846 is also currently being trialled (not finalised and not remotely optimised).

First, thank you for your continued hard work on improving this incredible tool. I wanted to share some observations while migrating workflows to the latest version (v1.15.1), in case they might help future refinements.

The RGThree reroute nodes still function, but the output ports can no longer be rotated, which disrupts workflow readability. While I’ve started using the new built-in reroute nodes in parallel, manually replacing hundreds of legacy reroutes is quite time-consuming. Is there an automated method or script to assist with bulk migration? Additionally, the new reroutes require pre-existing connections between nodes to insert them, which adds extra steps compared to RGThree’s implementation.

If I may, I’d like to respectfully highlight RGThree’s custom nodes as a potential source of inspiration. Their reroute logic, for example, offers unique advantages like flexible port rotation and the ability to insert reroutes without pre-existing connections—features that significantly streamline workflow organization. Exploring their work might reveal even more user-centric innovations (e.g., dynamic labeling, modular shortcuts) that align with Comfy UI’s goal of balancing power and comfort.

I completely understand the complexity of development and prioritize stability above all. My current setup works perfectly, and I’ll continue using it regardless. My intent is only to humbly share ideas, not to overstep. Comfy UI has already revolutionized my workflow, and I’m deeply grateful for your dedication.

Thank you again for considering this perspective!

murphylanga avatar Mar 25 '25 15:03 murphylanga

Additionally, the new reroutes require pre-existing connections between nodes to insert them, which adds extra steps compared to RGThree’s implementation.

The floating reroute can still be created the same way as before Image Image

While I’ve started using the new built-in reroute nodes in parallel, manually replacing hundreds of legacy reroutes is quite time-consuming. Is there an automated method or script to assist with bulk migration?

Bulk migration is proposed in #449. Currently the reroute node in core is removed and bulk migrated to the new reroute.

huchenlei avatar Mar 25 '25 16:03 huchenlei

The floating reroute can still be created the same way as before

Awesome! @huchenlei @webfiltered though I think there's still a bug in the update that doesn't update not-fully-connected reroutes to these empty reroute.

As in these ComfyUI Reroute nodes:

Image

Became this:

Image

Which would surely upset a lot of folks. You'll probably want to better handle updating those disconnected reroutes when updating.

rgthree avatar Mar 26 '25 00:03 rgthree

Also, @huchenlei @webfiltered, it looks like another bug in the update. Updating reroute nodes create a corrupt workflow (that then gets saved). Most of the time this can be kinda benign, but may as well make sure you're cleaning things up correctly. rgthree-comfy has a workflow corruption monitor (opt-in) and a separate "link fixer," which you can get to by going to http://127.0.0.1:8188/rgthree/link_fixer/ after installing rgthree-comfy. On there, open the browser dev console (F12) and drag in a "new reroute converted" workflow and you'll see the issues listed in the console.

From what I can tell, it looks like the update routine does NOT correctly update the origin node's output's links field's array with the correct link id (it still points to a link to a reroute node that no longer exists). LiteGraph will often draw the workflow correctly, but can very easily coin-flip on dropping connections, changing connectione, etc. (this is why I created the link_fixer). It also doesn't fix the corrupt data, even if it draws it correctly, so images generated will persist the bad workflow links, etc. (Also, anyone who has rgthree-comfy's data corruption enabled would immediately see an alert that their workflow has corrupt data if they had any ComfyUI re-routes).

I would urge you to fix this and the previous comments' issues before releasing the auto-update.

rgthree avatar Mar 26 '25 01:03 rgthree

Okay, back to the filed issue: If you pull the latest rgthree-comfy your Reroute (rgthree) nodes' inputs and outputs will be positioned correctly!

There's two issues I could not figure out yet: 1) The newer ComfyUI forces the 10px reroute nodes to be no smaller than 30px, which is unfortunate but at least doesn't break anything; and 2) While the quick reroute keyboard shortcuts seem to work just fine, the connecting noodle no longer updates as expected (it remains connected to the original node after inserting a rgthree-comfy reroute, even while the new reroute node is currently "selected" and, further, adding another rgthree-comfy reroute via keyboard shortcut correctly attaches it to that last reroute).

I'm considering this mostly mitigated at this point. Maybe I'll make a label for that.

rgthree avatar Mar 26 '25 02:03 rgthree

@rgthree I can come back to this with answers to more sections, but just quickly; to make a link come "from" a reroute (i.e. the last link 'segment' between the reroute and the node input, you can set link.parentId to the reroute.id.

webfiltered avatar Mar 26 '25 03:03 webfiltered

Thanks for the tip @webfiltered ! Looks like parentId is for native reroutes; maybe I'll extend the quick shortcuts to work for native ones too. But, what I was referencing was breaking the node-based rgthree-comfy reroutes.

If I could have your ear perhaps there's a solution and I'll explain what broke with the ComfyUI update, what I thought should fix it, but is wonky, and what actually fixes it (but is against the typed link). (I can put this in a new bug somewhere too, but since I have your attention, I'll put it here for now.)

The TL;DR of all of this is having a mechanism to updating the _currently dragging noodle's starting position/slot/node while being dragged would be great. Here's what I've found..

Previous behavior @main

Here's what we used to be able to do (I'm clicking the output and while dragging using the keyboard shortcuts to place new rgthree-comfy reroute nodes, move them, rotate them, resize them, and continue all w/o lifting up your drag). This all works fine in the current main release.

https://github.com/user-attachments/assets/353061b6-4636-440a-875c-3d3489d97727

Now, that was all done by manually creating the node, manually connecting it together, and then updating the current link in app.canvas.connecting_links when a new reroute was inserted which was controlling the drawing of the current link.

Currently broken behavior @latest

Unfortunately, that is now broken because app.canvas.connecting_links no longer plays the part it used to. Currently, we get the following which actually still mostly works, except the dragging noodle doesn't update to the latest node.

https://github.com/user-attachments/assets/1227591d-bb30-435d-b74f-2109ead4cd51

Working Fix, (but typing issue)

Now, I've found that there's now a LinkConnector which I can get the renderLinks and update that in a similar fashion to the old connecting_links. This actually works almost as well (with only the current noodle's direction not updating if rotating the node with a shortcut):

https://github.com/user-attachments/assets/e4ddf688-b85d-4fff-bb05-52e36fa9b181

Note, this works perfectly fine, but it is updating fields in the renderLinks items that are marked as read only:

Image

Should be working fix? But not..

So then, I thought, "well, maybe instead of updating the noodle's fields directly, we can use the link connector to reset the drag and dragNewFromOutput." This is the one that makes the most sense, frankly:

Image

I would have thought it would work just the same/similarly as the working fix above, but it had an unexpected issue... While the noodle did move to the next node as expected, when using shortcuts to update the position/size/rotation of the reroute like before, the noodle's position did not update like it had done in the previous fix. I'm not sure why those are different?

https://github.com/user-attachments/assets/79bc0ac0-e7bd-404d-8997-00e43450e4f4

Maybe that's a "bug" and dragNewFromOutput should be behaving the same way as the drag from your mouse. If so or even if not, though, what do you think about a solution to either update an existing dragging noodle (or, stopping and starting a new one, but with it being able to know when it's starting slot is moving..)?

rgthree avatar Mar 27 '25 03:03 rgthree