-
Notifications
You must be signed in to change notification settings - Fork 5.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
feat: support graph control #3658
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: storyicon <storyicon@foxmail.com>
Seems mildly related to #2666 |
agreed. |
It is great to see other PRs actively pushing for the implementation of such requirements in the community. This PR does indeed intersect with the lazy evaluation mentioned in #2666, but the changes in #2666 are a bit massive. I am a little concerned about whether it can be merged as scheduled. |
You mirrored my concerns much more articulately. |
But then there are already lots of solutions for this -- if this is proposed to be core, then the PR2666 that cover's this should be looked at, by you, to make sure it doesnt blow up. Making partial fixes in the meantime doesnt feel correct.
Have you all actually pulled and tested 2666? Unless and until people do that and bubble up the concerns, its just going to be going on the last best. From what I understand that IS a future PR that will make it into the system -- this was said multiple times at the summit. So, check it, push any problems into there so the two people doing the work can update it and not actually break things. My personal tests give it a 100% thumbs up, and I am not the biggest fan of major change, you can see my comments in that thread. But I dont feel a middle patch doing it yet another way is a good idea. |
I agree with you, a lot of things fall into middle patches for larger issues that are or should be getting work behind the scenes. Part of it is a bit of obfuscation as to what kind of roadmap is being followed. A bit more clarity in communication could go a long way. It pains me every time I see someone put quite a bit of work into something that is a well though out middle patch of something that is going to get a complete rewrite, or that will most likely be entirely replaced by something already in the works. |
This commit aims to support graph control, allowing for the execution or skipping of certain branches of the graph based on conditions. I believe that as ComfyUI gains popularity, the demand for graph control will increase, especially in the field of portraiture, where it may be necessary to execute different subsequent processes depending on whether the input image contains multiple people, a single person, or no one. Alternatively, when the user does not provide a mask, a simpler process may be executed. This commit, by modifying recursive_execute and introducing the GraphControl node, supports this branching logic.
Since the input boolean value is true, the Preview Image node above is executed, while the Preview Image node below is skipped. This is a simple use case, but in actual work, it may be connected to a much more complex workflow.
I believe that the implementation of this feature in this PR is already sufficiently simple, and the logic evaluation nodes can be entirely left to the community to create. Some possible use cases include:
I believe that the existence of this capability is necessary, as it will greatly enrich the possibilities of ComfyUI. If you have any other ideas, feel free to discuss them at any time.