WASasquatch / was-node-suite-comfyui

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

Did the Text Concatenate node change? #310

Closed alessandroperilli closed 9 months ago

alessandroperilli commented 9 months ago

I am convinced that the Text Concatenate node used to have an option to separate inputs by newline (linebreak_addition). Now it doesn't seem to offer that option anymore, it ignores newlines that come from each input, and it doesn't accept an empty delimiter field.

But maybe I am wrong and it never had that feature?

WASasquatch commented 9 months ago

There was a PR that rewrote how these worked by @Arcitec I believe. Maybe that can be added back in?

alessandroperilli commented 9 months ago

Yes, I realised that after opening this PR, @WASasquatch, thank you. Reverting that PR or, at least, adding it as an alternative would be ideal. Almost no Text Concatenate node around (and there are dozens) has the linebreak_addition feature and that's why this node was my all-time favorite.

aewne commented 9 months ago

Also seems to eat leading and trailing spaces, making wildcards a bit harder to use. Leading spaces can be delimited or escaped with \ though

Arcitec commented 9 months ago

There was a PR that rewrote how these worked by @Arcitec I believe. Maybe that can be added back in?

@WASasquatch Thank you for tagging me here so I could take a look. This issue can be closed since it's a misunderstanding of how the nodes work now. :)

@alessandroperilli wrote: I am convinced that the Text Concatenate node used to have an option to separate inputs by newline (linebreak_addition). Now it doesn't seem to offer that option anymore, it ignores newlines that come from each input, and it doesn't accept an empty delimiter field.

But maybe I am wrong and it never had that feature?

...

Yes, I realised that after opening this PR, @WASasquatch, thank you. Reverting that PR or, at least, adding it as an alternative would be ideal. Almost no Text Concatenate node around (and there are dozens) has the linebreak_addition feature and that's why this node was my all-time favorite.

The old node ONLY had ONE option for separators, linebreak_addition:

The new node has infinite options:

So insert the Text Concatenate node, set delimiter to \n, and you're done! :+1: :smile_cat:

@aewne wrote: Also seems to eat leading and trailing spaces, making wildcards a bit harder to use. Leading spaces can be delimited or escaped with \ though

Correct. Text concatenation does cleanup of useless surrounding whitespace of the text it's being told to concatenate. That cleanup does not affect wildcards or anything. It simply ensures that the output isn't full of pointless whitespace.

The whitespace cleanup ensures that you get nice and clean text concatenations, such as a, b, c instead of a , b , c.

By wildcards, I'm guessing that you're talking about nodes that take a word such as <scene> and automatically inserting a randomly picked scenery in that location.

Whitespace has no effect on such substitutions. Whitespace is literally useless.

Arcitec commented 9 months ago

@alessandroperilli @aewne Here is a screenshot showing some of the workflow features that the enhanced nodes can now perform:

https://github.com/WASasquatch/was-node-suite-comfyui/pull/306#issuecomment-1866779757

One of the nicest new features: You can now mute/bypass the input-text nodes to silence them in the concatenation nodes. Meaning that you can dynamically build your prompt by simply bypassing or unbypassing various text nodes.

Let's say that you have one text node full of daytime scenery descriptions, and another full of nighttime descriptions. You can then route both to either text concatenate or text list concatenate, and then mute the unwanted variant directly in ComfyUI's GUI without leaving the UI, to easily switch the scene, without having to use any complicated "random prompt/wildcard/scene builder nodes" (which usually involves manually editing text files or CSV files on the hard disk instead).

It's just yet another way you can flexibly use the improved nodes. :)

alessandroperilli commented 9 months ago

@Arcitec, thanks for taking the time to address this during the holidays.

I appreciate the additional flexibility introduced by your PR. I just tested the \n option and it works great. Thank you. Perhaps, it would be helpful to clarify this possibility somewhere in the documentation as it might not be obvious without looking at the code or reading this PR thread.

Re the removal of the leading space, I have this particular scenario:

I want to print a newline in multiple parts of certain text that will be printed in the terminal and inside a file. This is to visually separate portions of the aforementioned debug text.

For example, in the following example, I don't want the ------------- to appear next to Generation Parameters, but underneath it.

Screenshot 2023-12-27 at 14 39 25

This was easy to do in the previous implementation of this node, as any empty line in the Text Multiline node was respected by the Text Concatenate node.

Similarly, I want an empty line after Batch Size: 1 and one before Generation Notes:

Now, all these empty lines are ignored. And even if I try to add a \n in the Text Multiline node, it gets ignored.

Screenshot 2023-12-27 at 14 45 34

I assume I'm doing something wrong. I'd appreciate guidance here.

alessandroperilli commented 9 months ago

BTW, and I understand this is not the focus of this PR, if the Text Concatenate node had infinite inputs instead of 4, it would be SO much more flexible :)

aewne commented 9 months ago

My point was not all whitespaces are useless.

Regarding muting/unmuting, my workflow is set up up handle this automatically using switches instead of muting. Doing it manually sounds like a great way to get carpal tunnel syndrome. I've unfucked the affected workflows for now. Anyway, thanks for taking the time to fix the bugs in these nodes.

Arcitec commented 9 months ago

@alessandroperilli I don't really understand the point of having a few empty lines or leading/trailing whitespace, and it only seems useful for debug output to the terminal/console, since the Stable Diffusion neural network itself has zero need for whitespace and it just messes up the text concatenation.

But it's trivial enough to have an optional toggle that passes through all inputs unfiltered (with leading and trailing "empty lines/spaces") if people want to micromanage the output (I don't mean that in a bad way).

I'll add that option now. :)

Arcitec commented 9 months ago

BTW, and I understand this is not the focus of this PR, if the Text Concatenate node had infinite inputs instead of 4, it would be SO much more flexible :)

I agree, but that seems to require JavaScript hooks that rewrite the behavior of the node dynamically, since there's no way to "dynamically add more inputs" in the official ComfyUI Node definition API, from what I can see.

I've only seen rgthree do dynamic inputs, which seems to confirm my suspicion that it's not possible to do that in ComfyUI's own API.

Edit: But if you look at that screenshot I included earlier, you can see some ideas for how to chain together deep text concatenation. Basically, you can send up to 7 texts to a "Text List" node to convert them into a list. Next, you can send up to 4 of those "lists" into a "Text List Concatenate" node, to merge them into 1 list. So that's already 4x7 = 28 text inputs with that layout. Next, you can keep merging more and more lists via chaining extra "Text List Concatenate" nodes. And finally send the ultimate, joined list into "Text List to Text" to make your final text string.

alessandroperilli commented 9 months ago

Yes, the request is purely for the debugging use case. ComfyUI might benefit from a more verbose log. Until that happens, I had to build a little function to print what I want to print. Thanks for implementing the feature!

Trung0246 also does dynamic inputs, but I don't know if they use the same approach used by rgthree.

I saw your screenshot, and I already do that sort of chain. It's not a big problem. It's just a nice way to save horizontal space. The AP Workflow is designed to maintain full visibility of the information flow while trying to save as much space as possible. So, dynamic inputs help a lot.

Arcitec commented 9 months ago

Thanks for implementing the feature!

No worries. Hope @WASasquatch merges it soon. :)

Trung0246 also does dynamic inputs, but I don't know if they use the same approach used by rgthree.

I had a quick look in their python files and don't see anything about dynamic inputs there. So I had another quick look at their javascript. It's a 4000 line meaty blobber of a file, but it seems like the function junction_impl() is responsible for dynamically changing the inputs and outputs.

It also seems like the way they achieve dynamic inputs is as follows:

I could get into the Javascript side and implement dynamic nodes, but I really don't have time for that. I prefer to stick to the official Python API for nodes, to avoid having to maintain Javascript in the future whenever ComfyUI changes.

I sincerely hope ComfyUI adds dynamic nodes as a core feature instead. It's super helpful and would be such an improvement over the current ecosystem of fixed-size nodes.