Skip to content

Add Type Conversion Nodes #8181

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

Closed
wants to merge 4 commits into from
Closed

Conversation

LaVie024
Copy link
Contributor

@LaVie024 LaVie024 commented May 18, 2025

Considering that there is an open PR for logic nodes, as well as new string nodes added, it only makes sense that these nodes should be native to Comfy for the ease of manipulation of types and such. I've decided to add some features to these nodes that current custom node implementations are lacking, mainly with floats and bools.

Self-Explanatory Nodes

I do not feel like these ones in specific need much of explanation as they are very simple type conversions.
2025-05-18_13-31

Float to All

There are essentially three ways we can convert a float into an int:

  1. Truncation
  2. Rounding
  3. Banker's Rounding (round-half-to-even)

This essentially allows for users to decide which one they would like to do for a specific float when converting to an integer. For a string, we allow the user to choose how many decimal points to round to for the string, or to just display the full float.
2025-05-18_13-32

String to Number

Converts a string into both an integer and a float, has a simple check to ensure that the string is going to be able to be converted into a float before it begins. Has the same rounding options as Float to Integer.
2025-05-18_01-58

String to Combo

I have only ever seen one implementation of this, in Comfyroll Studio to be precise. The idea of this node is simple: allow widgets that contain lists to be broken out and given a string input. This allows for more flexibility when working with these widgets.
2025-05-18_02-03

@Amorano
Copy link
Contributor

Amorano commented May 18, 2025

Way too many nodes IMO. Single node can cover all this.

@LaVie024
Copy link
Contributor Author

LaVie024 commented May 18, 2025

Way too many nodes IMO. Single node can cover all this.

I could see how some of the nodes could be combined... I don't see how they could all be combined.
Int to Float/Int to String/Int to Bool -> Int to All
Float to Int/Float to String/Float to Bool -> Float to All

Other than that I fail to see how it could all be condensed into one node, that just seems like a large headache of a node.

@LaVie024
Copy link
Contributor Author

Condensed some of the nodes. I don't feel like the String nodes should be combined due to their special functionalities that make it more of a headache to deal with. I at least condensed the int and float nodes to just one node specifically.

@Amorano
Copy link
Contributor

Amorano commented May 18, 2025

Way too many nodes IMO. Single node can cover all this.

I could see how some of the nodes could be combined... I don't see how they could all be combined. Int to Float/Int to String/Int to Bool -> Int to All Float to Int/Float to String/Float to Bool -> Float to All

Other than that I fail to see how it could all be condensed into one node, that just seems like a large headache of a node.

I made one called the VALUE node. It is in my old pack up until 1.7x. I have since been* updating for the new frontend changes and havent iterated it up yet.

Wasnt a "headache" at all.

Removed StringToBool because it is just too similar to the already existing Compare node.
Copy link
Collaborator

@webfiltered webfiltered left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this could all be condensed and clarified into a proper, simple primitive type conversion system. I'd rather avoid adding a utility node for every type conversion, or having conversion nodes with three or more outputs.

If this can be done with expedience, we can avoid adding individual nodes, and needing to maintain them / migration code for them forever.

@PrometheusDante
Copy link

Would the solution take up more nodes/space than using the ImpactConvertDataType? That is my current converter of choice for all the basic types.
Untitled-1

@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

Would the solution take up more nodes/space than using the ImpactConvertDataType? That is my current converter of choice for all the basic types. Untitled-1

The problem is that is still static to 4 output types. It should not be locked to ANY output types so that it can support future type output.

@jtydhr88
Copy link
Contributor

hi @LaVie024 , likely we are doing some duplicate work since I also have convert node in my PR here #8024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants