WASasquatch / was-node-suite-comfyui

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

feat: Add "Sort Text" Node #370

Closed thayol closed 5 months ago

thayol commented 8 months ago

Adds a "Sort Text" node.

Node details

Name: Text Sort

Inputs:

Outputs:

Why

Animagine XL recommends sorting the prompt.

An easy test setup: image


Sample inputs and outputs defined in tests/test_WAS_Text_Sort.py

Tests can be run using pytest. The current directory has to be the tests/ folder.

The testing setup is very jank but I have no idea how to set it up for packages without their parent packages. (comfy in this case.)

iiOxygen commented 7 months ago

this has issues with loras and embeddings within the prompt. I believe one way to go on about it is by separating tokens that start with "embedding:" or "<lora" in lines #10212-10214 with something like this:

normal_tokens = [token for token in tokens if not (token.startswith("embedding:") or token.startswith("lora:"))]
special_tokens = [token for token in tokens if token.startswith("embedding:") or token.startswith("<lora")]

sorted_normal_tokens = sorted(normal_tokens, key=WAS_Text_Sort.token_without_weight)

sorted_tokens = sorted_normal_tokens + special_tokens
    return (join_separator.join(sorted_tokens), )

It has similar issues with prompt control. It'd be perfect if there is a way to combine both.

WAS-PlaiLabs commented 6 months ago

@thayol Do you think you can implement the changes to account for embedding?

thayol commented 6 months ago

I've been personally using it since and I have issues around the tokens being half-weighted or dropped. I should probably revisit the whole thing.

Any other pitfalls I should know of? Currently I know of: no weights, (full weight:2), half (weight:0.5), (default weight), embedding:file.pt, (:3:3) (colon in weight name)

WAS-PlaiLabs commented 6 months ago

Probably wildcards __some-wilcard__ and dynamic prompts {a|b|c}

thayol commented 6 months ago

If wildcards are evaluated after sorting (as in during CLIP encoding) then I believe that wouldn't be an issue. If dynamic prompts can have nested weights, it's probably something I'll have to look at separately.

Do we have unit tests? I feel like I could be more confident in the changes if there was something backing the WAS_Text_Sort class that isn't manually tested.

WAS-PlaiLabs commented 6 months ago

I haven't set up any unit tests -- actually, I don't know how. Other people have always set them up on projects and really wasn't involved beyond running the pipelines on new changes. 😬

thayol commented 6 months ago

After hours of thinking I decided to remove the token separating thing. It gets rid of the unsightly RegEx and it fixes half-weighted tokens. The only downside is that if you have multiple tokens in a weight group, they are kept together.

Also, ComfyUI claims that weights multiply with nesting but in my limited testing it only used the innermost group's weight with no attention to anything else.

WASasquatch commented 5 months ago

Thank you!