Nourepide / ComfyUI-Allor

ComfyUI plugin for image processing and work with alpha chanel.
MIT License
186 stars 22 forks source link

Unable to Use ImageScaleBy, the built-in node in ComfyUI #4

Closed ltdrdata closed 9 months ago

ltdrdata commented 12 months ago

When installing this extension, ImageScaleBy becomes a missing node.

EricRollei commented 12 months ago

In my windows portable build with Allor nodes, I'm missing both ImageScale and ImageScaleBy

Nourepide commented 12 months ago

Because ImageScaleBy is replaced by ImageTransformResize (Relative or Absolute). You can disable this, just modify config.json and set transform in override to false.

For more information see Configuration and ImageTransformResize.

I'm thinking of adding notifications about which nodes have been replaced to the console output, because as I feared, the current situation is not particularly intuitive for beginners.

EricRollei commented 12 months ago

If we disable, does it keep both the ImageScale and ImageScaleBy and the ImageTransformResize? I have been making the substitutions but it's not always apparent when I load someone else's workflow what they had there. Now I have workflows saved with both types....

ltdrdata commented 12 months ago

Because ImageScaleBy is replaced by ImageTransformResize (Relative or Absolute). You can disable this, just modify config.json and set transform in override to false.

For more information see Configuration and ImageTransformResize.

I'm thinking of adding notifications about which nodes have been replaced to the console output, because as I feared, the current situation is not particularly intuitive for beginners.

Is there a reason for disabling the built-in node? Not only will people who are not familiar with this setting perceive it as a bug, but it can also lead to compatibility issues as it prevents the usage of workflows that rely on image scale nodes. Disabling the default node, in particular, can be quite critical.

EricRollei commented 12 months ago

I guess it's more than just the 2 ImageScale nodes... My console on comfy startup says that 10 standard nodes was (sic) overridden. What all are they? I think non [Allor]: 10 standard nodes was overridden. [Allor]: 11 modules enabled. [Allor]: 91 nodes was loaded.

If I just delete the Overide section in the config.json what happens? Will I get both sets of nodes, yours and the built-in ones? I wouldn't mind having both your nodes and the built-in ones so long as they don't conflict. { "fonts_folder_path": ["comfy_extras", "fonts"], "modules": { "AlphaChanel": true, "Clamp": true, "ImageBatch": true, "ImageComposite": true, "ImageContainer": true, "ImageDraw": true, "ImageEffects": true, "ImageFilter": true, "ImageSegmentation": true, "ImageText": true, "ImageTransform": true }, "override" : { "postprocessing": true, "transform": true, "debug": true } }

Nourepide commented 12 months ago

If we disable, does it keep both the ImageScale and ImageScaleBy and the ImageTransformResize? I have been making the substitutions but it's not always apparent when I load someone else's workflow what they had there. Now I have workflows saved with both types....

Yes, if you disable override you will have ImageScale, ImageScaleBy, ImageTransformResizeAbsolute and ImageTransformResizeRelative in one time.

Nourepide commented 12 months ago

Is there a reason for disabling the built-in node? Not only will people who are not familiar with this setting perceive it as a bug, but it can also lead to compatibility issues as it prevents the usage of workflows that rely on image scale nodes. Disabling the default node, in particular, can be quite critical.

Yes, it was a difficult decision.

On the one hand, there are arguments for the override to be disabled initially:

Ferniclestix commented 11 months ago

Because ImageScaleBy is replaced by ImageTransformResize (Relative or Absolute). You can disable this, just modify config.json and set transform in override to false. For more information see Configuration and ImageTransformResize. I'm thinking of adding notifications about which nodes have been replaced to the console output, because as I feared, the current situation is not particularly intuitive for beginners.

Is there a reason for disabling the built-in node? Not only will people who are not familiar with this setting perceive it as a bug, but it can also lead to compatibility issues as it prevents the usage of workflows that rely on image scale nodes. Disabling the default node, in particular, can be quite critical.

I cant agree more with this, overriding default nodes is... really silly. just make a folder and put yours in it, people will use whichever one is needed. - this is quite common with custom node packs.

opensourcefan commented 11 months ago

It seems to me that it's an unofficial accepted standard to leave the underlying system untouched when adding on. You're not attempting to create your own version of ComfyUI right, you're adding usability and power to the existing platform. Out of respect for the host system of your feature addition you really should make the separation obvious and clear. It would make your amazing work stand out at the same time.

Please consider doing what most are doing with the addition of features.

zamp commented 11 months ago

Definitely should not override default nodes. It makes this not backwards compatible which is just dumb. I just installed this and quick googling lead me to here so yeah. I thought at first this is just broken and uninstalled it immediately once it broke my already existing workflows.

Nourepide commented 11 months ago

All right, yours took it. In any case, I'm not able to come up with a better solution right now due to lack of time, so I'll just make it disabled by default.

Subsequently, I will write a column in the README about overrides and which nodes can be replaced by others.

Nourepide commented 11 months ago

Now, execute git pull in Allor folder.

This issue will last for some time to make sure that everyone's problem has disappeared and in about a week I will close it.

Please tell me if the update went well for you.