Uint32 Seed #62
Merged
Uint32 Seed #62
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
July 4, 2023 13:09
Justification: The seed internally is changed between different number types with wildly different max values. This PR normalizes all of them to UInt32. Earlier @pcuenca commented that the reason for the lower precision use of Double was to support the CompactSlider component and make it easier for a user to select a number. In this submission the CompactSlider component has been swapped for a TextField. [Feature extracted from larger PR #59 #59]
|
There's a minor glitch in the UI: the left side of the seed box is slightly clipped. Probably trying to align it with the other controls (see Steps above) will fix it. 
|
|
A random seed is now achieved with a value of 0. If the value is zero then the TextField will clear to empty and the disclosure group title will be "Random Seed" If the value is 1 or more then the disclosure group title will change to "Seed." A tiny stepper has also been included to the right of the TextField indicating that the TextField accepts numeric values and allowing a quick tap or long press to increment/decrement the value. 
|
Align the leading edge of the new TextField for seed value with the other controls.
|
Minor UI glitch fixed with commit 0efb7ba |
added 2 commits
July 16, 2023 20:47
Added a docc compatible comment for PipelineState enum. Internal functions that do no need external access marked as fileprivate.
pcuenca
reviewed
Jul 25, 2023
|
Long numbers are usually truncated. Can we artificially limit the upper range, or do you think it's important to allow any 32-bit value? 
(Also, maybe display seeds without delimiters) |
remove leftover debug comment line Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
personal preference in how to casr/declare type. (style guide needed) Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
remove cast Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
remove seed greater than zero check Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
remove unnecessary cast Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
remove unnecessary cast Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
|
"Long numbers are usually truncated. Can we artificially limit the upper range, or do you think it's important to allow any 32-bit value?" |
- seedBinding in ControlsView doesn't return an empty string but rather just the generation.seed value which won't be a nil value. - In StatusView when displaying the seed value it will be truncated to 3 places (1.134B, etc) - Comment describing the contents of GenerationState.complete(,,,) removed. - Addition of Double.reduceScale and general func formatLargeNumber(UInt32) -> String added to Utils
|
|
Hello, I’ve been using your app as the base for one I’m developing. First than anything Thank You!
To address the issue with the -1 random seed problem, I implemented a shuffle button 🔀. When the button is toggled ON, the field displays "Random," and when it's toggled OFF you can input a specific seed. Clicking the "set" button after generating an image turns off shuffle.
Peace :)
… On Jul 25, 2023, at 11:04 AM, Pedro Cuenca ***@***.***> wrote:
@pcuenca commented on this pull request.
Thanks for your patience! I think this is almost ready to go, I have some minor suggestions, and I think some casts are not necessary.
In Diffusion-macOS/ControlsView.swift <#62 (comment)>:
> @@ -422,5 +421,38 @@ struct ControlsView: View {
modelDidChange(model: ModelInfo.from(modelVersion: model) ?? ModelInfo.v2Base)
}
}
+
+ fileprivate func discloseSeedContent() -> some View {
+ let seedBinding = Binding<String>(
+ get: {
+ generation.seed < 1 ? "" : String(generation.seed)
I think the default should be 0, as it's what we tell the user to type if they want to set a random seed. It can be a bit jarring to type it only to see it disappear.
In Diffusion/Common/Pipeline/Pipeline.swift <#62 (comment)>:
> guidanceScale: Float = 7.5,
disableSafety: Bool = false
) throws -> GenerationResult {
let beginDate = Date()
canceled = false
-
- let theSeed = seed ?? UInt32.random(in: 0...maxSeed)
+// print("Generating...")
⬇️ Suggested change
-// print("Generating...")
In Diffusion/Common/State.swift <#62 (comment)>:
> @@ -17,6 +17,7 @@ let DEFAULT_PROMPT = "Labrador in the style of Vermeer"
enum GenerationState {
case startup
case running(StableDiffusionProgress?)
+ //complete(positivePrompt, optional generated image, seed used to create it, how long it took to generate)
You are right that it's confusing, how about removing the comment and add the names to the enum case instead? (we can do it in a separate PR).
In Diffusion/Common/State.swift <#62 (comment)>:
> @@ -49,7 +50,7 @@ class GenerationContext: ObservableObject {
// FIXME: Double to support the slider component
@published var steps = 25.0
@published var numImages = 1.0
- @published var seed = -1.0
+ @published var seed = UInt32(0)
⬇️ Suggested change
- @published var seed = UInt32(0)
+ @published var seed: UInt32 = 0
Personal preference, sorry :) We state the type and then we initialize.
In Diffusion/Common/State.swift <#62 (comment)>:
> @@ -71,14 +72,14 @@ class GenerationContext: ObservableObject {
func generate() async throws -> GenerationResult {
guard let pipeline = pipeline else { throw "No pipeline" }
- let seed = self.seed >= 0 ? UInt32(self.seed) : nil
+ let seed = self.seed > 0 ? UInt32(self.seed) : nil
Isn't it already UInt32?
In Diffusion/Common/State.swift <#62 (comment)>:
> @@ -71,14 +72,14 @@ class GenerationContext: ObservableObject {
func generate() async throws -> GenerationResult {
guard let pipeline = pipeline else { throw "No pipeline" }
- let seed = self.seed >= 0 ? UInt32(self.seed) : nil
+ let seed = self.seed > 0 ? UInt32(self.seed) : nil
⬇️ Suggested change
- let seed = self.seed > 0 ? UInt32(self.seed) : nil
In Diffusion/Common/State.swift <#62 (comment)>:
> return try pipeline.generate(
prompt: positivePrompt,
negativePrompt: negativePrompt,
scheduler: scheduler,
numInferenceSteps: Int(steps),
+ seed: UInt32(seed ?? 0),
⬇️ Suggested change
- seed: UInt32(seed ?? 0),
+ seed: seed,
In Diffusion-macOS/StatusView.swift <#62 (comment)>:
> @@ -87,10 +87,10 @@ struct StatusView: View {
let intervalString = String(format: "Time: %.1fs", interval ?? 0)
Text(intervalString)
Spacer()
- if generation.seed != Double(lastSeed) {
+ if generation.seed != UInt32(lastSeed) {
⬇️ Suggested change
- if generation.seed != UInt32(lastSeed) {
+ if generation.seed != lastSeed {
In Diffusion-macOS/StatusView.swift <#62 (comment)>:
> Text("Seed: \(lastSeed)")
Button("Set") {
- generation.seed = Double(lastSeed)
+ generation.seed = UInt32(lastSeed)
⬇️ Suggested change
- generation.seed = UInt32(lastSeed)
+ generation.seed = lastSeed
—
Reply to this email directly, view it on GitHub <#62 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHCDUPG5SXPQYGLQ7O4J7BLXR74DVANCNFSM6AAAAAAZ5UYOXU>.
You are receiving this because you are subscribed to this thread.
|
pcuenca
reviewed
Jul 26, 2023
@pcuenca caught an error in logic where the random seed trigger was being tested as "seed > 1" which would result in a seed of 0 or 1 generating a random seed. Changing to "seed > 0" (or seed >= 1) is the correct logic for 0 as random seed.
|
Thanks again @dolmere! 🙌 |
|
Just noticed this feature, awesome work @dolmere! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Justification: The seed internally is changed between different number types with wildly different max values. This PR normalizes all of them to UInt32.
Earlier @pcuenca commented that the reason for the lower precision use of Double was to support the CompactSlider component and make it easier for a user to select a number.
In this submission the CompactSlider component has been swapped for a TextField.
[Feature extracted from larger PR #59 https://github.com//pull/59]