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.

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

Change or Add inpainting to support new InpaintModelConditioning from ComfyUI #176

Open
tazlin opened this issue Feb 1, 2024 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested

Comments

@tazlin
Copy link
Member

tazlin commented Feb 1, 2024

As of comfyanonymous/ComfyUI@10f2609, ComfyUI now supports a new kind of inpainting which purportedly supports lower denoise strengths for inpainting-specific models (as originally proposed here: comfyanonymous/ComfyUI#2501).

  • Problems with inpainting are a very common end-user support topic - it strikes me as very likely that this alternative implementation may be better received in general.
  • Should we convert the existing inpainting pipeline or add this as an option?
  • Are there any gotchas that might trip users when changing to/adding this?

@Efreak @spinagon @db0

@tazlin tazlin added help wanted Extra attention is needed good first issue Good for newcomers question Further information is requested enhancement New feature or request labels Feb 1, 2024
@spinagon
Copy link
Contributor

spinagon commented Feb 1, 2024

I tested it a bit, the new node seems to work better at both low and high denoise.
It also doesn't wash out the rest of the image (or at least not as much).
I'd just convert inpaint to this, barring any unexpected pitfalls.
Seed values would obviously change, but I don't think anyone cares with inpaint.

@Efreak
Copy link
Contributor

Efreak commented Feb 4, 2024

Three only problem I see with converting is that existing saved settings will no longer work. I recently re-ran a bunch of old generations on newer SDXL models, which is something I do occasionally. I'm not sure how common this is.

I can't personally test this change without it being on a running worker; I've never actually used ComfyUI except via Horde, and I'm also out of runpod credit because I spent 2.5 months worth last month 😢.

@db0
Copy link
Member

db0 commented Feb 5, 2024

I'm OK with switching. Unfortunately maintaining this level of backwards compatibility is not something we can do as a volunteer service. Backwards compatibility is expensive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants