feat: support random seed before generation #93
Conversation
|
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. |
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: |
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 |
|
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? |
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. |
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 |
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.
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? |
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. |
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 |
|
Yes lets move this discussion to #278 |
|
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 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. |
Not yet. Let me rebase. |
But the point of that conversation is literally pertaining to whether the PR was appropriate to begin with, or should be handled another way. It directly pertains to the nature of this PR and a track record of it's discussion of desired implementation. As like we have also mentioned, having both, is ill-advised and would create end-user confliction and possible issue reports for nothing. |
|
@WASasquatch The project maintainer asked you to move the conversation elsewhere. @comfyanonymous Please mark these comments as off topic. |
|
@jn-jairo Okay, it's been rebased and it works with the new Primitive and Input/Widget conversion settings. |
I just tried, and it works, that is what I need for my workflow. |
|
I'd like to clear the air and come to an agreement with @WASasquatch on this subject. I don't want to sound draconian, combative, petty, or even like I'm going against GitHub etiquette. First, I don't want to stifle any discussion. I'm glad to see so many of us interested in this project and discussing how it can be improved. That's why I suggested that the global settings conversation continues—just not here. The issue is about scope. The topic of this PR is "supporting random values before generation". It's a relatively small scope that adds the option to generate random values before image generation rather than just after or never. (As we now have a Primitive node that can represent integers other than seeds, this goes beyond just seed generation.) Whether or not we have global seed generation options is outside the scope of this issue since it is a separate topic, one that this PR is not addressing. If the consensus is to wait to merge this PR until deliberation has finished on whether we should have global seed generation settings, that is fine with me. We can add a comment that this PR is on hold and link to the global seed discussion. But this is not the place for that discussion. In fact, the only reason that discussion is here is because comfy asked that intriguing, yet tangential, question in their first comment. But it is outside of the scope of this PR because it does not answer the question "should we have an option to generate random values before image generation?" Now, let me address the comment I made about not implementing global seed generation settings in this PR, as I don't want anyone to get the wrong impression. What I mean by that is, this PR is only concerned with adding the option to allow random value generation before image generation. If code is committed to the main branch that adds global seed generation options, I'd be happy to rebase and update this PR to accommodate the updated codebase. What I will not do is implement global seed generation settings in this PR because that is not within its scope. What I will do is add the before generation feature to any existing global settings should they exist before this PR is merged. @WASasquatch I get what you're saying about not wanting to have discussions split up. But having a discussion about global settings in a PR that doesn't address that issue is not following GitHub etiquette since it's inflating the scope of the PR. We should follow the pattern of separation of concerns so that we can focus our conversations. This thread should be for reviewing the code in the PR and its affect on the codebase. The discussion about whether we should have global seed settings should separate. I hope that clears things up. I don't want to promote any negativity in this community, and I'm sorry if anything I said came off that way. |
|
@comfyanonymous There are still two outstanding issues with this PR. I've updated the description with checkboxes for them, but I'll list them here too.
|
|
You can patch workflows before and right after they are loaded with Line 653 in 754553f |
This is how these discussions work everywhere when the argument is whether the PR is relavent because of something, like comfy originally raised, which is a solid point which pertains to the whole existence of this PR. It's directly related to whether this PR should be merged. Is it a good idea to merge it because there could be a global setting. Cross referencing posts on this debate is negligence imo. Currently most of the debate and information is here. |
|
To be clear @jordanbtucker it was rude to put words in your mouth, but no offense, it seems clear you want to maintain a positive veiw and conversation about your PR and not detract from it's appeal to merge. Which I think is off-top. PRs come with rebuttal on whether they're relevant or needed VS other proposed ideas of stuff on the roadmap. And to further clarify, I am on board with your idea, just do not like cross referencing ideas to keep things, clean, positive, or whatever it may be. The points raised are directly made against the PR itself, not something else. It's whether the PR would be needed over a global setting, but I think that using both for different tasks is important. Your idea to split things is odd, cause what if it was a established idea by someone else, like me, for example. Do I just put in a challenging PR that directly works against your PR? No, we should discuss in your PR, cause the ideas would conflict. |
Thanks for the tip. This PR is now backward compatible with workflows created before the time this gets merged. If you're okay with calling |
|
Okay, I think I finally understand what everyone is talking about when they say "global setting" and the confusion is due to the fact that I'm being dense. 😳 When comfy suggested the setting to be global instead of per-node, I took that to mean that all seed generation settings would be moved out of nodes and into a global settings panel. And that never made sense to me. What comfy probably meant is that nodes would still have a toggle to turn randomness on or off, but there would be another global setting that would be a toggle for whether the random value would be set before or after image generation. With this viewpoint, the conversation makes a lot more sense and I can see why it's directly relevant to this PR. I think I completely missed the point and that's why it didn't make sense to me to be talking about global settings when all I wanted was to change this one little quirk. 😁 @WASasquatch and everyone, please forgive my stupidity. I'm open for discussion on whether this before/after setting should be global, now that I think understand it, and I think it makes sense to continue that conversion here. Please correct me if I'm missing any other crucial points of discussion. 🙏 |
Yes that's exactly what I meant, sorry for the confusion.
Instead of calling graphToPrompt you can iterate on the nodes like this: https://github.com/comfyanonymous/ComfyUI/blob/master/web/scripts/app.js#L660 |
|
is there anything preventing this from being merged? I find the "after generation" random number generation weird or maybe I'm doing it wrong and there's a better workflow. |
|
This is covered by a setting now |
This PR adds the ability to generate a random seed before the workflow is queued.
after generationfor backward compatibility.beforeQueuedcallback.after generation.trueandfalsefor backward compatibility with existing workflows.after generationwhen it seestrueandoffwhen it seesfalse, but I haven't figured that out yet.graphToPrompttwice. It feels hacky, so maybe there's a better way to implement that part.TODO
truevalues should be changed toafter generation, andfalsevalues should be changed tooff.graphToPromptreally needs to be called twice or if there is a better way to inspect and update the random widgets before generation.