Shall we coordinate our effort on IC-Light?

[huchenlei]

Hi there, I have native IC-Light implemented here as well: https://github.com/huchenlei/ComfyUI-IC-Light.

Sorry I did not notice you are also building a native node. Several things notable when I am doing the impl:

Conv_in patch

The conv_in patch already happens in https://github.com/huchenlei/ComfyUI-layerdiffuse/blob/151f7460bbc9d7437d4f0010f21f80178f7a84a6/layered_diffusion.py#L34-L96 So you don't need to do that manually and care about replacing the conv module back. The impl just need to patch unet weight including conv_in module weight. The rest will be handled automatically. In the console you are expecting following logs:

WARNING SHAPE MISMATCH diffusion_model.input_blocks.0.0.weight WEIGHT NOT MERGED torch.Size([320, 8, 3, 3]) != torch.Size([320, 4, 3, 3])
Merged with diffusion_model.input_blocks.0.0.weight channel changed from torch.Size([320, 4, 3, 3]) to [320, 8, 3, 3]

c_concat

As conv_in is expecting exact amount of c_concat, we don't need to mess with cond/uncond. Passing c_concat with model_patcher.set_model_unet_function_wrapper should be the easiest method.

VAE ArgMax mode

Diffusers impl' is taking .mode instead of .sample from VAE results. We probably need to do the same in ComfyUI. Related code here: https://github.com/comfyanonymous/ComfyUI/blob/0fecfd2b1a2794b77277c7e256c84de54a63d860/comfy/ldm/models/autoencoder.py#L13-L31 This probably don't affect much on overall quality so can be overlooked.

[kijai]

Hey, shame we did some overlapping work but I'm happy to have your input and you just answered the remaining questions I had! I applied your calculate_weight_adjust_channel-patch and it seems to work straight away. Correct if I'm wrong, but we can't have multiple of unet_function_wrapper? As in wouldn't it be more compatible to use the extra_conds as I already am?

I missed the VAE difference and it probably explains the slight difference to the diffusers outputs, but honestly so far I have preferred the Comfy default VAE, and I want to keep combability with all the batched/tiled vae nodes. Will have to test if it's worth it indeed.

Edit: tested the VAEEncodeArgMax and I'm not seeing any difference at all

[aqidesign]

两位大佬好，你们两位的IC-Light节点包我都用过，感谢你们的贡献。 我测试过一些产品的重新打光，我发现kijai的节点更符合我的预期，主要是更保留原图的材质。具体我也不懂是什么原因，我把图片放在下面给你们参考。

Hello guys, I have used both of your IC-Light node packages. Thank you for your contribution. I have tested the relighting of some products, and I found that kijai's nodes are more in line with my expectations, mainly retaining the materials of the original image. I don’t know the specific reason, so I put the picture below for your reference.

Original picture： huchenlei： kijai：

[huchenlei]

两位大佬好，你们两位的IC-Light节点包我都用过，感谢你们的贡献。 我测试过一些产品的重新打光，我发现kijai的节点更符合我的预期，主要是更保留原图的材质。具体我也不懂是什么原因，我把图片放在下面给你们参考。

Hello guys, I have used both of your IC-Light node packages. Thank you for your contribution. I have tested the relighting of some products, and I found that kijai's nodes are more in line with my expectations, mainly retaining the materials of the original image. I don’t know the specific reason, so I put the picture below for your reference.

Original picture： huchenlei： kijai：

This does not seem right. Can you share the workflow you used and a screenshot to show params? It seems to me that you put FG and BG in reversed position in the workflow. Do not that the input order in batch images matters in the BG workflow.

[huchenlei]

Well, I cannot reproduce the issue in the FG workflow:

[huchenlei]

Hey, shame we did some overlapping work but I'm happy to have your input and you just answered the remaining questions I had! I applied your calculate_weight_adjust_channel-patch and it seems to work straight away. Correct if I'm wrong, but we can't have multiple of unet_function_wrapper? As in wouldn't it be more compatible to use the extra_conds as I already am?

I missed the VAE difference and it probably explains the slight difference to the diffusers outputs, but honestly so far I have preferred the Comfy default VAE, and I want to keep combability with all the batched/tiled vae nodes. Will have to test if it's worth it indeed.

Edit: tested the VAEEncodeArgMax and I'm not seeing any difference at all

I think for unet_function_wrapper I need to check if there is an existing one and calls into the existing wrapper function to make this composable with other extension's wrapper patch. Thanks for pointing that out!

I think the VAE result really does not make such a big diff as most of the times sample can be closely aligned with mode. I am not sure why Lvmin choosed to use mode instead of sample in the diffusers impl. We shall wait for the official paper to see if that is on purpose. If not, probably user can just choose between normal VAE and ArgMax VAE.

[kijai]

Hey, shame we did some overlapping work but I'm happy to have your input and you just answered the remaining questions I had! I applied your calculate_weight_adjust_channel-patch and it seems to work straight away. Correct if I'm wrong, but we can't have multiple of unet_function_wrapper? As in wouldn't it be more compatible to use the extra_conds as I already am? I missed the VAE difference and it probably explains the slight difference to the diffusers outputs, but honestly so far I have preferred the Comfy default VAE, and I want to keep combability with all the batched/tiled vae nodes. Will have to test if it's worth it indeed. Edit: tested the VAEEncodeArgMax and I'm not seeing any difference at all

I think for unet_function_wrapper I need to check if there is an existing one and calls into the existing wrapper function to make this composable with other extension's wrapper patch. Thanks for pointing that out!

I think the VAE result really does not make such a big diff as most of the times sample can be closely aligned with mode. I am not sure why Lvmin choosed to use mode instead of sample in the diffusers impl. We shall wait for the official paper to see if that is on purpose. If not, probably user can just choose between normal VAE and ArgMax VAE.

I'm still noticing small differences compared to the diffusers wrapper, but I'm kinda unsure if I just made a mistake in the wrapper itself, as I tend to prefer the comfy native results, or if it's some other difference to diffusers in general that I'm missing.

Do you think there's a downside to using the extra_conds method? That's what comfy uses for IP2P and I figured it'd be well supported way to do this.

[huchenlei]

@kijai

Should we merge repos? I really do not want to have 2 ComfyUI-IC-Light in ComfyUI manager, which can be confusing. I am also reciving issue from user who installed this node pack. https://github.com/huchenlei/ComfyUI-IC-Light-Native/issues/6

[kijai]

@kijai

Should we merge repos? I really do not want to have 2 ComfyUI-IC-Light in ComfyUI manager, which can be confusing. I am also reciving issue from user who installed this node pack. https://github.com/huchenlei/ComfyUI-IC-Light-Native/issues/6

Yeah I have been thinking about that, dunno how to go about it though without causing further confusion.

[huchenlei]

@kijai Should we merge repos? I really do not want to have 2 ComfyUI-IC-Light in ComfyUI manager, which can be confusing. I am also reciving issue from user who installed this node pack. huchenlei/ComfyUI-IC-Light-Native#6

Yeah I have been thinking about that, dunno how to go about it though without causing further confusion.

I have just renamed by repo to ComfyUI-IC-Light-Native so at least users can tell the difference by name. Because the impl is actually not that complicated, I do not expect further change to the node code unless lvmin released new models. I think we can leave the current state for now, and avoid make user migrate from one to another. When lvmin release the new update, we shall deprecate one repo and make users migrate to another repo. WDYT?

I am also not going to add my repo to ComfyUI Manager to avoid confusion.

[kijai]

@kijai Should we merge repos? I really do not want to have 2 ComfyUI-IC-Light in ComfyUI manager, which can be confusing. I am also reciving issue from user who installed this node pack. huchenlei/ComfyUI-IC-Light-Native#6

Yeah I have been thinking about that, dunno how to go about it though without causing further confusion.

I have just renamed by repo to ComfyUI-IC-Light-Native so at least users can tell the difference by name. Because the impl is actually not that complicated, I do not expect further change to the node code unless lvmin released new models. I think we can leave the current state for now, and avoid make user migrate from one to another. When lvmin release the new update, we shall deprecate one repo and make users migrate to another repo. WDYT?

I am also not going to add my repo to ComfyUI Manager to avoid confusion.

That's fine for now, I still want to test if there are practical differences in our patching approaches, so far everything has worked fine but there are some issue reports I don't understand and can't replicate at all.

I added mine to the Manager as I noticed someone copied my wrapper and added it, which is extremely weird as it's 1:1 copy of my code, not marked as fork or having any mention of me, probably should do something about that to avoid even further confusion...

[Lalimec]

Great work fellas! Do u mind explaining what conditioning multiplier in @kijai repo stands for and is there any equivalent method to replicate that in @huchenlei one? Does it also "multiply" the other conditionings that add before that? https://github.com/huagetai/ComfyUI-Gaffer This guy also seems to have some sort of multiplier but uses totally different values i guess. Is it because they are already using the scale_factor of the model? https://github.com/huagetai/ComfyUI-Gaffer/blob/main/iclight.py#L192

Also, there seem to be a little difference in results when a bg is selected. Somehow i got a little blurry backgrounds in @kijai implementation. Apart from that, does it suppose to look this bad lol, am I doing it wrong?

[kijai]

The multiply is just the VAE scale factor indeed, I left it exposed as it seemed useful in some situations. The blurrier background must be because of the cond node doing the auto resizing in latent space, at least I get pixel perfect identical results with both versions if both input images are exactly the same size before encoding them.

It looks like you are encoding the foreground input image for the sampler? You should rather use either empty latent, or just very simply light image encoded.

[Lalimec]

That resize node really has some problems... Blur issue is resolved but the empty latent didnt do anything, tbh i am not sure if that's reason behind that weird plastic look. Generations look like the ones on that old relight paper, like overlaying the light map from the bg to the image according to a normal map. Surely it would be ok with a second pass, but results are not really comparable to the no-bg version. And should I carry this to another issue?

Lol, i also realized we cant use 2 different "load and apply ic-light" nodes. They overwrite each other.