Kosinkadink / ComfyUI-Advanced-ControlNet

ControlNet scheduling and masking nodes with sliding context support
GNU General Public License v3.0
619 stars 61 forks source link

Refactor load_controlnet to use original loading function & some minor fixes #4

Closed artventuredev closed 1 year ago

artventuredev commented 1 year ago

Hi, I simplified the control.py to reuse original comfy load_controlnet instead of rewrite the whole function to reduce maintenance effort. This PR also includes some small other changes:

Kosinkadink commented 1 year ago

Hey, thanks for the PR. The timing node is gonna be immensely helpful, it was one of the nodes I was gonna work next, so thank you for making it! The M1 chip bug is interesting, do you know if there is any noticeable slowdown from not using the *= operator on non-Macs? I would assume not, but python does python things.

As for the negative index support, while the version in this PR will work for typical use, a small change needs to happen to properly support sliding context windows like in the AnimateDiff-Evolved repo, where the latents passed in won't represent the actual full latents. I've included the fix for that in the comment. Once that's fixed, the PR looks good and I'll merge.

artventuredev commented 1 year ago

The M1 chip bug is interesting, do you know if there is any noticeable slowdown from not using the *= operator on non-Macs? I would assume not, but python does python things.

I did 5 consecutive runs on my Linux system and there's no different in the runtime, so I guess there're no impact or it should be minimal.

As for the bug, the interesting part is the x[0] *= weight works just fine, but x[1] *= weight (index > 0) does nothing. The more interesting part is I was unable to replicate this issue with my small script below. So I'm not sure what can be the cause for this.

import torch

tensor = torch.randn(2, 320, 64, 64).to('mps')
tensor[1] *= 0
(torch.min(tensor[1]) == 0).item() // True
Kosinkadink commented 1 year ago

Gotcha, I'll merge since technically there is no functional different. It would be good to revisit this and find out why this happens, or if something else (like potentially batch indexes not updating in the nodes) made it appear as if it was not working, unless you used a debugger and stepped through it to verify the multiplication actually didn't do anything.

artventuredev commented 1 year ago

unless you used a debugger and stepped through it to verify the multiplication actually didn't do anything.

Yeah I did that. Here's an example, x[1] refuses to update:

image

I also setup a simple workflow. Where I set strength to only batch_index 1 but both 2 images got applied (the *= 0 part)

image

Kosinkadink commented 1 year ago

Yeah, that's crazy weird. I do not own any Apple hardware, so thanks for finding the issue!