Skip to content

Allow reroute to have in/out in different orientation#445

Closed
Davemane42 wants to merge 4 commits into
Comfy-Org:masterfrom
Davemane42:master
Closed

Allow reroute to have in/out in different orientation#445
Davemane42 wants to merge 4 commits into
Comfy-Org:masterfrom
Davemane42:master

Conversation

@Davemane42
Copy link
Copy Markdown
Contributor

took into consideration old workflows and convert the node after its configured.
also handle the old vertical/horizontal setting.

added reroute to the canvas right-click for easy access.

image
image

@omar92
Copy link
Copy Markdown
Contributor

omar92 commented Apr 8, 2023

nice one thanks

@pythongosssss
Copy link
Copy Markdown
Member

Nice!

Found a couple of small issues:

  1. Add reroute -> Show Type = Crash this.getDisplayName() is undefined
  2. Have workspace prior to this version with a reroute where the name is showing
    image
    Update to this version
    image
    Double text

@Davemane42
Copy link
Copy Markdown
Contributor Author

@pythongosssss Double text should be fixed, cant really reproduce the undefined function

@pythongosssss
Copy link
Copy Markdown
Member

Hmm, odd I can reproduce it on a fresh colab instance by:

  1. Add a reroute
  2. Right click, Show Type -> Crash
    image

Also crashes via

  1. Add a reroute
  2. Right click, Show Type By Default
  3. Add a reroute -> Crash, cant add reroutes anymore
    image

@Davemane42
Copy link
Copy Markdown
Contributor Author

@pythongosssss I was thinking it was a colab only bug but i loaded up a fresh install with the branch and it just worked, no error.

@comfyanonymous
Copy link
Copy Markdown
Member

I get the exact same bug as pythongosssss. It happens in both firefox and chromium for me on your exact branch and a clean browser.

@Davemane42
Copy link
Copy Markdown
Contributor Author

Davemane42 commented Apr 10, 2023

I've looked at the code again and i cant really tell what it is. ComfyUI is my first foray into Javascript and i really dont know what m doing wrong here.

tested with edge, chrome and firefox with no error

I can see the 2 errors and ill rework these part after a night sleep but its all guessing on my part after this

vivaldi_ddVIOET1x1.mp4
git clone https://github.com/comfyanonymous/ComfyUI
cd ComfyUI
git fetch origin pull/445/head
git checkout -b pullrequest FETCH_HEAD

@asagi4
Copy link
Copy Markdown
Contributor

asagi4 commented Apr 10, 2023

I tested this PR and extended it a bit. I allowed setting the orientation of input/output to any direction and if there's overlap they just swap.

I also made it possible to add multiple slots to the reroute node (input 1 goes to output 1, 2 to 2, etc).

In on mobile now so I can't post the code, but I can get you a diff later.

The multi-output support could probably be used to implement the "bus" functionality requested elsewhere too if you just add a mode that copies one input to all outputs.

@asagi4
Copy link
Copy Markdown
Contributor

asagi4 commented Apr 10, 2023

@Davemane42 This branch has a couple patches on top of yours: https://github.com/asagi4/ComfyUI/tree/input-orient

It's probably not perfect, but I wasn't able to immediately break things.

I worked around getDisplayName returning nulls by just making sure it never does... This probably changes the title display behaviour a bit, but at least it doesn't explode.

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.

5 participants