WASasquatch / was-node-suite-comfyui

An extensive node suite for ComfyUI with over 210 new nodes
MIT License
1.23k stars 178 forks source link

Problem with image_bounds logic for images with shape length of 4 #253

Closed LoneOutpost closed 1 year ago

LoneOutpost commented 1 year ago

There's a problem in image_bounds in the class WAS_Image_Bounds where a line is assuming "image" is an iterable instead of a tensor. This is a simple fix, but since I don't have access to do the pull request, here's the solution:

The line: bounds = [(0, img.shape[1] - 1, 0, img.shape[2] - 1) for img in image]

Should be: bounds = [(0, image.shape[1] - 1, 0, image.shape[2] - 1)]

WASasquatch commented 1 year ago

Images should always be a iterable instance of tensors. They should always be batched. Even if just 1 image. The idea he is eventually getting these nodes someone PRed to handle more than one image.

LoneOutpost commented 1 year ago

I realize that now after going through the logic a bit more. It's just there is a lot wrong with these bounded nodes as I commented in another ticket. This fixed the one problem but uncovered a bunch more problems as it is accounting for some times accounting for batches of 1 and other times not. Some times it just squeezes the batches to ignore them. It's a little ugly in there, I'm assuming these worked at one point, but things have moved on since the original PR.

I guess the corrections should actually be: bounds = [(0, img.shape[0] - 1, 0, img.shape[1] - 1) for img in image] but since all of the batches are going to be the same size I'm not quite sure this isn't just single tuple.

WASasquatch commented 1 year ago

Yeah when these were made masks werent batched, and always shape [H, W], then now we have batched masks in shape [N, H, W].

People wanted batch image support which naturally means batched masks to match those images. And the code is sorts just laid out confusingly so I think I just broke them trying to wrap my head around them. I haven't had much time to return to them.