Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DiffusionImage #64

Merged
merged 16 commits into from Aug 1, 2023
Merged

DiffusionImage #64

merged 16 commits into from Aug 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 4, 2023

Addition of DiffusionImage Model extracted from 59 #59

dolmere added 2 commits July 4, 2023 23:16
DiffusionImage
b53f8db 
Addition of DiffusionImage Model extracted from 59 #59
Update
f13f117 
Latest DiffusionImage updates to support drag and drop and copy.
pcuenca
pcuenca reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you please explain the use-cases that this will enable?

@pcuenca pcuenca mentioned this pull request Jul 12, 2023
Closed
dolmere and others added 2 commits July 12, 2023 18:05
@pcuenca
Update Diffusion/Common/DiffusionImage.swift
04220b0 
Docc compatible comment style.

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
@pcuenca
Update Diffusion/Common/DiffusionImage.swift
12dbcfa 
typo fixes in descriptive note

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
@ghost
Copy link
Author

ghost commented Jul 12, 2023

Sorry for not making the high level description of this PR!

Motivation:
Create a single model object to represent a generated image.

Discussion:
The primary object created inside of this application is an image file. This new class is a Model representing a single instance of a generated image.
This instance captures the details used to generate this image, or the "recipe" of settings run through the diffusion pipeline.
This instance allows us to inspect or re-use this "recipe" content elsewhere within the application.

Drag and drop support:
By representing the generated image in an encapsulated class we also gain the benefit of being able to use the instance of the object itself as a data provider for drag and drop and share operations.

Batch image support:
By making the new DiffusionImage class aware of its own state the SwiftUI code can observe the class for changes and update the UI when the image is "waiting" to run, "generating" in the pipeline, or "complete" and ready to display.

Saved images: (line 189+)
With this addition there is a new feature added to the application which is to save the generated image into a temporary folder. Note: it is up to the app itself to cleanup its own custom temp folder! What this enables is a history of generated images and will enable a gallery of images to be added later. This user managed gallery of their created images is NOT included with the current PRs.

dolmere added 6 commits July 12, 2023 18:53
Merge branch 'main' into diffusion-img
40fe279
Merge branch 'main' into diffusion-img
37d9fcd
Better comments
f02d9d0 
Fleshing out docc compatible comments in new class. Also refactored two identical functions with different names.
Merge branch 'main' into diffusion-img
7e6d355
Manual merge fix
624fa0c 
Fix from recent manual merge
Custom local enum created
e425fc4 
Diffusion_StableDiffusionScheduler created to get around a problem in a future PR for copy/paste and drag/drop with preserved "recipe" data about how the image was created.
pcuenca
pcuenca reviewed Jul 30, 2023
View reviewed changes
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I think we could simplify the code (and, more importantly, make it clearer) if we could isolate the OS differences in just a few helper functions, and then write the main class without conditional compilation.

If it's possible to do so, I'd even use a separate file for the OS extensions, something like DiffusionImage+OS.swift or something you think is descriptive. save, pngRepresentation, etc. would go there.

Do you think that approach is feasible?

Review suggestions
a43ac8a 
This is a consolidated commit with the suggestions submitted in review.
- Move as much platform specific code as possible out into target specific files.
- Move save() and pngRepresentation() into separate platform specific files
- Move platform specific protocol code out into separate file.
pcuenca
pcuenca reviewed Jul 31, 2023
View reviewed changes
Less platform checks
fe9c9d3 
New Utils_iOS and Utils_macOS allow platform specific utility functions to live inside the target file structure.
- added CGImage.fromData(Data) funcs for macOS/iOS
- added new func inside StableDiffusionScheduler enum.
- renamed Diffusion_StableDiffusionScheduler to StableDiffusionScheduler
pcuenca
pcuenca reviewed Aug 1, 2023
View reviewed changes
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for iterating! 🙏 I just got a couple of minor comments before merge.

dolmere and others added 4 commits August 1, 2023 09:25
@pcuenca
Update Diffusion/Common/DiffusionImage.swift
6f473cb 
remove comment with answered question

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
@pcuenca
Update Diffusion-macOS/DiffusionImage+macOS.swift
0e60208 
remove comment from self descriptive code

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
@pcuenca
Update Diffusion-macOS/DiffusionImage+macOS.swift
2806d74 
remove comment from self descriptive code

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
@pcuenca
Update Diffusion/Common/DiffusionImage.swift
acc3f24 
more concise comment

Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
pcuenca
pcuenca approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!! 🙏

@pcuenca pcuenca merged commit 4eab476 into huggingface:main Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@pcuenca