ComfyUI
ComfyUI copied to clipboard
comfyanonymous
feat: support random seed before generation
This PR adds the ability to generate a random seed before the workflow is queued.
- Converts the random value widget from a toggle to a combo with the following values:
- after generation
- before generation
- off
- Sets the default value to
after generationfor backward compatibility. - Adds a
beforeQueuedcallback. - Updates the default graph to use
after generation. - Supports the original value of
trueandfalsefor backward compatibility with existing workflows.- I'd like to have the UI update the value to
after generationwhen it seestrueandoffwhen it seesfalse, but I haven't figured that out yet.
- I'd like to have the UI update the value to
- I'm not sure about running
graphToPrompttwice. It feels hacky, so maybe there's a better way to implement that part.
TODO
- [x] When a workflow from before this PR is loaded,
truevalues should be changed toafter generation, andfalsevalues should be changed tooff. - [ ] Investigate whether
graphToPromptreally needs to be called twice or if there is a better way to inspect and update the random widgets before generation.
It works.
Does it make sense for this kind of seed behaviour to be a setting that can be adjusted per node instead of a global setting?
I think this should be on a per-node basis. I would definitely use it that way, at least.
Would it make sense to let the user decide weither it's a global setting or not by making a node "RandomNumberGenerator", and adding an integer (that has a different type than INT) as an input to KSamplerAdvanced for setting the seed?
-> like that, if I want to share a seed, I can simply connect multiple KAdvancedSampler to the same RandomNumberGenerator.
If I want independant seeds, I can create 1 RandomNumberGenerator per KAdvancedSampler.
I think this would give a lot more control for handling seeds.
like that, if I want to share a seed, I can simply connect multiple KAdvancedSampler to the same RandomNumberGenerator.
I often use the same seed for the first and second pass (upscaled), and also other values, so that would be very useful.
It would be better to add the ability to connect nodes for primitive types (int, float, string ...) and create nodes for values (ValueString, ValueInt, ValueFloat, ...) and add a randomize option in the numeric nodes.
This is definitely a must, and should have been this way from the get go, cause when you generate, it immediately switches out your seed, so you're left looking to PNG info. It just kinda seems backwards. You should know your seeding going into the gen.
I implemented an hijacked sampler node that reuses the original one to use 2 latent inputs (1 for the image, the other for the noise) and that reuses the original code as much as possible.
I also implemented a latent noise generator, and a random number generator to go with it.
Let me know if you want them, I can share them here later.
One cool thing you can do with them is generate offset noise towards a certain color to make the sampler more likely to generate that color, for example.
Though I think these nodes should be in ComfyUI from the get go instead of having to make them myself, and that we shouldn't have to rely on tricks like this to make it work.
Here's what it looks like:
I implemented an hijacked sampler node that reuses the original one to use 2 latent inputs (1 for the image, the other for the noise) and that reuses the original code as much as possible.
I also implemented a latent noise generator, and a random number generator to go with it.
Let me know if you want them, I can share them here later.
One cool thing you can do with them is generate offset noise towards a certain color to make the sampler more likely to generate that color, for example.
Though I think these nodes should be in ComfyUI from the get go instead of having to make them myself, and that we shouldn't have to rely on tricks like this to make it work.
Here's what it looks like:
This is actually smart, and more conducive with what is actually going on, on the diffusion side as apposed to the random seed int which is just a input for the noise.
When we get optional inputs the official ksampler definitely needs a seed input and noise input. I love this.
I called the RandomNumber just Seed in WAS Node Suite, and that may be better since this currently works off a fixed output, and not definable like range for a "Random Number" system.
Because every input can potentially be "random" because of: https://github.com/comfyanonymous/ComfyUI/pull/243
I think the random behaviour should be adjusted as a global option especially if there are more options added than just "random" like "increment" or "decrement".
What do you think?
Because every input can potentially be "random" because of: #243
I think the random behaviour should be adjusted as a global option especially if there are more options added than just "random" like "increment" or "decrement".
What do you think?
Shouldn't it just be a mode then with the node? global settings for something you may want on for one node, and off for another is silly business.
Are you really going to mix "random before every gen" and "random after every gen" on the same workflow?
Are you really going to mix "random before every gen" and "random after every gen" on the same workflow?
Why not?
You're making a node network, these are things you leave as options, as it's for custom workflows, whatever they me, especially what you might not envision.
I'm just asking because I want to get it as correct as possible the first time because if it needs to be changed later it will be a pain.
I'm just asking because I want to get it as correct as possible the first time because if it needs to be changed later it will be a pain.
Yeah that's true. I am just thinking it may be considered an annoying switch to have to go into settings? A place that's otherwise hidden. Also a workflow description/tutorial/post may unintentionally lead a person to think things that aren't true for them cause of user preferences.
While this does kinda seem like a preference thing, on a logic side there could be a reason for both. I honestly can't think of a reason, but there could be. Maybe a memory node that retains seeds or something and compares seeds (why? I don't know, but people have been doing ridiculous stuff with seeds, let alone with SD back-end in general)
It comes down to different ways of seeing things. I see the UI as a queue where what you see is what is sent directly to the queue when the "queue prompt" button is pressed.
I understand why many people see it the other way though.
maybe it's just me, but wouldn't it be possible to add both? Both a global setting, and settings on individual nodes. The global setting could have priority over every seed if it's set to "override node behavior" or something. By default, it would be set to "set seed manually for every node". If it's set to "override node behavior", then the "Random seed after every gen" text would be greyed out. Would that be possible?
this is something i was thinking about so i'll say it here since there's discussion about global settings.
how about handling global setting as a node? it wouldn't connect to anything (maybe as the first thing that gets executed?) and would basically be always active (if placed, if not placed then default settings could apply instead). that way you can transfer workflows to other people and still be sure they'd execute the exact same way.
an implementation like this would also fit in much better in the node based nature of this project imo
this is something i was thinking about so i'll say it here since there's discussion about global settings.
how about handling global setting as a node? it wouldn't connect to anything (maybe as the first thing that gets executed?) and would basically be always active (if placed, if not placed then default settings could apply instead). that way you can transfer workflows to other people and still be sure they'd execute the exact same way.
an implementation like this would also fit in much better in the node based nature of this project imo
Would be a great way to share a whole workspace and workflow. Especially considering any additions that may turn up in global setting that may impact workflow usage.
maybe it's just me, but wouldn't it be possible to add both? Both a global setting, and settings on individual nodes. The global setting could have priority over every seed if it's set to "override node behavior" or something. By default, it would be set to "set seed manually for every node". If it's set to "override node behavior", then the "Random seed after every gen" text would be greyed out. Would that be possible?
Wouldn't that cause confliction? User reports bug their seed isn't working right to find they set a global setting that conflicts with node settings? Or wonders why settings are locked not taking note of a global setting?
While I encourage discussion, I don't think this is the right place for it unless you're talking about this PR. Maybe we can move this tangential conversation elsewhere?
Are there changes you want me to make to this PR, or can it be merged?
While I encourage discussion, I don't think this is the right place for it unless you're talking about this PR. Maybe we can move this tangential conversation elsewhere?
Are there changes you want me to make to this PR, or can it be merged?
It's directly related to the PR and whether it should be merged, or not, based on global setting.
This PR is only adding the possibility to randomize the seed before the generation. Weither it is a global setting or node-based is another matter that should be addressed on its own.
I've created an issue to continue this discussion #278.
This PR is only adding the possibility to randomize the seed before the generation. Weither it is a global setting or node-based is another matter that should be addressed on its own.
I've created an issue to continue this discussion #278.
And the discussion is whether or not that should be the case. I don't know what the confusion is or why you are being combative. Do you feel threatened that your PR won't be merged or something? But the conversation is literally about if it should, or if it shouldn't and just be a global setting. It shouldn't be a anywhere else but this PR to prevent confusion and cross posting
I don't need spam notifications for two topics regarding the same thing. That's ridiculous
It's his pull request and he politely asked use to move the discussion elsewhere so lets do that. You also won't get spam notifications from this topic because discussion will happen in the other one.
It's his pull request and he politely asked use to move the discussion elsewhere so lets do that. You also won't get spam notifications from this topic because discussion will happen in the other one.
It's a PR to a project, it isn't "his" PR request. And we should be following PR and Github etiquette. Not making special cases cause someone is annoyed they're getting notifications. Pull Requests are the fundamental unit of how we progress change, and track how we got there. We shouldn't have discussion split out places, especially when it's regarding a PR. This is literally what this discussion is for.
I'll continue to follow PR and Github etiquette.
@WASasquatch You're putting a lot of word in my mouth that I never said. I'm not being combative. I'm not annoyed by the notifications. And I'm not worried this PR won't be merged.
It's simply that the changes you are discussing are changes that I'm not going to make because they are out of the scope of my original implementation and goal for this PR. If someone wants to fork this and add global settings, that is fine by me, but I'm not going to be the one to do it. So I don't see the point in discussing it here. Either review this PR as it was originally intended or I will close it. I will not be implementing global settings in this PR.
Let's say I set it to before, queue the prompt and I liked the result, but I want to change things after the sampler but I want to keep the same seed, if I set it to off will it make the sampler run again? Because I don't want to run the sampler again, if it doesn't run the sampler again then it is exactly what I need for my workflow.
@jn-jairo It will not run the sampler again.
Great, does it work with the new widget to input? So I can share the seed.
Great, does it work with the new widget to input? So I can share the seed.
Not yet. Let me rebase.