-
Notifications
You must be signed in to change notification settings - Fork 22
Add parameter string output to Input Parameters node #91
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nodes_selectors.py
(4 hunks)
🔇 Additional comments (3)
nodes_selectors.py (3)
125-126
: LGTM! Class output declarations updated correctly.The RETURN_TYPES and RETURN_NAMES have been properly extended to include the new string output parameter.
136-136
: LGTM! Output tooltip added appropriately.The tooltip for the new parameters string output is consistent with the existing tooltip format.
153-154
: LGTM! New input parameters are well-defined.The new input parameters
string_prefix
andinclude_seed
have appropriate defaults and helpful tooltips. The string_prefix default of ", " is reasonable for parameter concatenation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@alexopus It's been two weeks, any thoughts on this PR yet? |
I've actually started something similar here already: https://github.com/alexopus/ComfyUI-Image-Saver/blob/master/nodes.py#L82 but using a typed container instead of a string for exchange. And also introducing a simpler saver node that accepts only the container as input without the individual parameters (but still with image settings). But I don't have time to work on that at the moment. |
I've now added a saver node that accepts a typed metadata container. In any case, I don't like dealing with comma separated strings as exchange format. Can you describe an example of your desired workflow. Then we can see if we can make it work with the new Metadata structure. |
Firstly, I use this in one of my workflows that has both an initial ksampler and a hiresfix ksampler. Due to this, it is basically hard to get the metadata for both of them under normal circumstances. Now I use Comfyui-Image-Saver just for the input parameters nodes, of course I do just use SD-Prompt-Saver node for the actual saving (mainly because it has some benefits, like using sampler/sched/etc. names as combos instead of inputs which is more compatible with rgthree's context nodes), and that has an extra info input that just accepts a string. Even examining the new sd-prompt-saver nodes, they lack this crucial extra infos input, which to me, is something that is necessary for workflows with a lot of parameters like what I do. My intention with the parameter nodes being able to output a string is simple: collect all necessary information, condense it into a string, and let it be saved, which allows it to be more modular. |
Maybe it's worth making dedicated node(s) just for handling such strings only - create/edit & save them with images. Mixing this string based approach with individual parameters may become confusing. Not sure. |
This is to easily gather a string from given parameters which is useful in workflows with multiple KSampler nodes and we want to get information from those nodes to put into the final image (currently an extra info embedding for the Image Saver node doesn't exist but I'll look into creating a PR for that), or to also be used as a streamlined way to view the given parameters in a workflow.