-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add Segmentation Ordering for Yolo segmentation #289
base: master
Are you sure you want to change the base?
Conversation
Allow yolo segmentation to sort by left-right, top-bottom, largest-smallest, and reverse that order. Params added to Regional Segmentation Group
stupid format-on-save
thank you so much @aimerib. what will be the new sorting command for this e.g. modifying this one for only inpainting maximum segment
|
The command stays the same, and under your parameters list, you should have a |
actually: |
but i want to only inpaint first face is that not possible? |
I thought the goal was to inpaint the largest face, right? This will detect the largest, no matter the position. If you want only the first found face, then you keep the index. If the use-case is different, do you mind giving me a more detailed scenario so I can mimic it locally? |
it is like this lets say it found 3 faces only inpaint first one sorted by max area and thank you so much |
So, for this scenario: use your regular |
Awesome this solves my issue But what if someone needs to inpaint 2 different person with different prompt what they can do? |
I think I mentioned in the closed issue, but my code only returns the first match. There's room for improving on that exact scenario. I figured that implementing just first match initially was a low-risk way to test that this works. Then we can add the sorting to the scenario where the user passes an index as well. I'm shooting for minimal code changes until we validate that this works well and doesn't break anything. |
Awesome make sense. I am waiting merge to test asap |
@@ -11,13 +11,17 @@ def INPUT_TYPES(cls): | |||
"model_name": (folder_paths.get_filename_list("yolov8"), ), | |||
"index": ("INT", { "default": 0, "min": 0, "max": 256, "step": 1 }), | |||
}, | |||
"optional": { | |||
"sort_order": (["left-right", "top-bottom", "largest-smallest", "default"], ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heck is "default"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I change that to none? Or just add a blurb that says "default - if an index is passed, segment the index, if no index is passed, segment all found masks" or something similar?
Basically default is, just do what this would do if nothing was set. Maybe that's redundant because if not toggled nothing is passed? I'm open to suggestions on proper wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no "default", there's some form of order, always. the code before this PR was explicitly always left-to-right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I will simply set left-right as the "default" and remove default
as an option. That will better represent the logic that already exists. Makes things easier that way, and less confusing.
@@ -57,6 +69,32 @@ def seg(self, image, model_name, index): | |||
masks = masks[sortedindices] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the sorting code in this function appears to be unmodified? Why is your code not editing the one thing it's meant to edit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that if you used sorting segment, in this current implemention, it ignores the index, and return the first mask after the sort. So the largest (or smallest), top-most (bottom-most), etc. If no sorting is used, then the normal behavior as it used to be is used. I was focusing on @FurkanGozukara usecase for testing this sorting logic, but it wouldn't be hard to make it so you can sort, and also pass and index, so getting the second largest segment, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that if you used sorting segment, in this current implemention, it ignores the index, and return the first mask after the sort. So the largest (or smallest), top-most (bottom-most), etc. If no sorting is used, then the normal behavior as it used to be is used. I was focusing on @FurkanGozukara usecase for testing this sorting logic, but it wouldn't be hard to make it so you can sort, and also pass and index, so getting the second largest segment, for example.
i think this would be best case so people can both inpaint with any index with any prompt and still can sort by largest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's literally existing sort logic here, just. Replace it with customizable sorting. Furkan's case is already covered, it just sorts in the wrong order for what he wants
@@ -40,6 +44,14 @@ def seg(self, image, model_name, index): | |||
else: | |||
masks = masks.data.cpu() | |||
masks = torch.nn.functional.interpolate(masks.unsqueeze(1), size=(image.shape[1], image.shape[2]), mode="bilinear").squeeze(1) | |||
|
|||
if sort_order != "default": | |||
boxes = results[0].boxes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most yolo models don't have boxes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh... thanks for the tip. I'll figure out a clean way to handle that scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again literally just use the existing sorting logic instead of building from scratch, it already accounts for different yolo model variants
src/Text2Image/T2IParamTypes.cs
Outdated
SegmentationSortOrder = Register<string>(new("Segmentation Sort Order", "How to sort segments when using '<segment:yolo->' syntax.\nleft-right, top-bottom, largest-smallest, or by-name.\nBy-name requires a pre-processing step to remap segment IDs to a consistent order.", | ||
"default", Toggleable: true, IgnoreIf: "default", GetValues: _ => ["left-right", "top-bottom", "largest-smallest", "default"], Group: GroupRegionalPrompting, OrderPriority: 4 | ||
)); | ||
ReverseSegmentationOrder = Register<bool>(new("Reverse Segmentation Order", "If checked, reverses the order of segments when using '<segment:yolo->' syntax.\nOnly applies if 'Segmentation Sort Order' is enabled. When enabled, 'left-right' becomes right-left, 'top-bottom' becomes bottom-top, and 'largest-smallest' becomes smallest-largest.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could just be integrated into the first parameter options rather than being separate. less confusing that way too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So first parameter options become: ["left-right", "right-left", "top-bottom", "bottom-top", "largest-smallest", "smallest-largest"]
and lose the param for reverting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeh
… supports indexes
I believe this new commit should hopefully address most concerns. Primarily, simplify params, support indexes, don't use boxes and use the existing sorting, just expand with custom options. |
i just locally merged and tested index works as expected smallest largest sort works too amazing |
I'm not sure when smallest to largest would be helpful, but it made sense to add it as an inverse. Depending on the model you use, it might detect a small face on a piece of bread if you don't setup the threshold correctly. Well, now you could target that face too... hahaha |
ok there is an edge error case - i think when no face is found
|
Oh! I will fix that right away! Good catch! |
@aimerib also if there is only 1 face will index 1 still work right? like
|
It should, but I will test it while I'm fixing the other error you found |
Basically, we already did ordering before, just always left-right, so using index 1 with only one face found would return that face |
@FurkanGozukara what was the specific segment command you used in the prompt that generated that error? I see you used the segmentationordering: "largest-smallest" so I'm trying with that too, but having a hard time hitting the same error |
@aimerib i found another bug when sorting enabled, it can't find any face but when it is disabled it can find here example
sorting disabled works sorting enabled |
Thank you! I was able to reproduce this error. I've pushed a new commit that should account for the error you were finding. Can you check both scenarios you described above when you have a moment? I really appreciate the help testing these cases! |
@aimerib excellent job the case that was giving error previously didnt give any error i am gonna do more testing now hopefully |
@@ -40,22 +43,46 @@ def seg(self, image, model_name, index): | |||
else: | |||
masks = masks.data.cpu() | |||
masks = torch.nn.functional.interpolate(masks.unsqueeze(1), size=(image.shape[1], image.shape[2]), mode="bilinear").squeeze(1) | |||
|
|||
sortedindices = self.sort_masks(masks, sort_order) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the wrong place, it was in the correct place previously, please move it back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really sorry, but I am having a super dense moment here. I don't think I see it. Do you mean where it used to exist before, in the block
if index == 0:
result = masks[0]
for i in range(1, len(masks)):
result = torch.max(result, masks[i])
return (result, )
elif index > len(masks):
return (torch.zeros_like(masks[0]), )
else:
sortedindices = [] ### Here?
for mask in masks:
sum_x = (torch.sum(mask, dim=0) != 0).to(dtype=torch.int)
val = torch.argmax(sum_x).item()
sortedindices.append(val)
sortedindices = np.argsort(sortedindices)
masks = masks[sortedindices]
return (masks[index - 1].unsqueeze(0), )
Or where I had it in my previous commit where I was sorting boxes and ignoring masks?
I don't mean to make things difficult, I just have had very very few sleep with the newest phase the kiddo is in, so I would really appreciate a little pointer in the right direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where it was before your PR is the correct place for it to be. Index 0 and index longer than mask count do not require sorting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, that makes sense. Thank you. I'll fix this.
src/Text2Image/T2IParamTypes.cs
Outdated
@@ -628,6 +628,9 @@ static List<string> listVaes(Session s) | |||
SaveSegmentMask = Register<bool>(new("Save Segment Mask", "If checked, any usage of '<segment:>' syntax in prompts will save the generated mask in output.", | |||
"false", IgnoreIf: "false", Group: GroupRegionalPrompting, OrderPriority: 3 | |||
)); | |||
SegmentationSortOrder = Register<string>(new("Segmentation Sort Order", "How to sort segments when using '<segment:yolo->' syntax.\nleft-right, top-bottom, bottom-top, largest-smallest, or smallest-largest.\nYou can also use indexes to specify a segment in the given order.\nExmaple: <segment:yolo-face_yolov9c.pt-2> when largest-smallest, will select the second largest face segment.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you listed the options in description here but excluded right-left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, indices
would be the plural of index, but this should just be use an index
src/Text2Image/T2IParamTypes.cs
Outdated
@@ -628,6 +628,9 @@ static List<string> listVaes(Session s) | |||
SaveSegmentMask = Register<bool>(new("Save Segment Mask", "If checked, any usage of '<segment:>' syntax in prompts will save the generated mask in output.", | |||
"false", IgnoreIf: "false", Group: GroupRegionalPrompting, OrderPriority: 3 | |||
)); | |||
SegmentationSortOrder = Register<string>(new("Segmentation Sort Order", "How to sort segments when using '<segment:yolo->' syntax.\nleft-right, top-bottom, bottom-top, largest-smallest, or smallest-largest.\nYou can also use indexes to specify a segment in the given order.\nExmaple: <segment:yolo-face_yolov9c.pt-2> when largest-smallest, will select the second largest face segment.", | |||
"left-right", Toggleable: true, IgnoreIf: "left-right", GetValues: _ => ["left-right", "right-left", "top-bottom", "bottom-top", "largest-smallest", "smallest-largest"], Group: GroupRegionalPrompting, OrderPriority: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrderPriority 4 is identical to SegmentMaskBlur
- also why this is in between mask params?
... also why does the existing code have overlapping order priorities lol.
Probably move this down to 5.5
by the way i generated over 2k images had no errors with last version |
@mcmonkey4eva can you finalize this? perhaps fix the remaining part? I really would like to show this at next tutorial. Thank you so much all |
@FurkanGozukara I'll work on finishing with the feedback and submit a PR at some point tonight or tomorrow morning at the latest. Sorry about the delay. |
still waiting :/ thank you so much |
Yeah, it will be a while longer. There was a massive hurricane here this week, and my family and I had to evacuate out of state. Our town is still full of flooding so it will be a while before I can look at this again. I’m thinking at least after this weekend. |
@aimerib thanks i hope you get better asap without any issues |
shown this in new tutorial i hope gets merged as soon as possible : https://youtu.be/FvpWy1x5etM |
@FurkanGozukara I am heading back home this weekend, so either this sunday, or this coming week I will work on the feedback left in this PR and work towards getting it merged. So cool to see this out in the wild already! |
@aimerib awesome ty looking forward to it |
@mcmonkey4eva perhaps you could complete this? I am about to make a video about inpainting , segmentation and yolo masking. Showing this would be great :( i plan to show a multi face - multi gender image as well like this one |
Allow yolo segmentation to sort by left-right, top-bottom, largest-smallest, and reverse that order.
Params added to Regional Segmentation Group