Skip to content

Add nodes for basic logic and math operation #8024

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jtydhr88
Copy link
Contributor

@jtydhr88 jtydhr88 commented May 9, 2025

implement basic logic operation, see
image
meanwhile, to use boolen value better, also added IF node
image

@LaVie024
Copy link
Contributor

Made a review originally but didn't add anything so I'll post here.

Would it not be worthwhile to add a compare node? The current one in Comfy is just for text, but it would be more useful to have one that handles ints and floats. This already exists in Easy-Use: https://github.com/yolain/ComfyUI-Easy-Use/blob/main/py/nodes/logic.py

So I would suggest looking into adding it.

@jtydhr88
Copy link
Contributor Author

Hi @LaVie024 yes, I plan to add Compare nodes for int and float as well, they will be next in the nodes of Math.

@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

Same problem I had/have with this PR. Add Type Conversion Nodes

You dont need to make single "type" nodes. These are all numerical operations, you should make a dropdown/switch to change "type" rather than add another set for FLOAT or INT or VECTOR, etc...

I explained it in more detail in this RFC: RFC Discussion: More Core Nodes

@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

Hi @LaVie024 yes, I plan to add Compare nodes for int and float as well, they will be next in the nodes of Math.

and I politely suggest you just make a single node for the operation, and not a dozen nodes because of type switching.

@jtydhr88
Copy link
Contributor Author

Same problem I had/have with this PR. Add Type Conversion Nodes

You dont need to make single "type" nodes. These are all numerical operations, you should make a dropdown/switch to change "type" rather than add another set for FLOAT or INT or VECTOR, etc...

I explained it in more detail in this RFC: RFC Discussion: More Core Nodes

Thanks for the feedback, in fact, on my design I am using IO.NUMBER rather than int/float, I almost finish and will push soon, then we could discuss more

@jtydhr88 jtydhr88 requested a review from christian-byrne as a code owner May 19, 2025 16:48
@jtydhr88 jtydhr88 changed the title Add nodes for basic logic operation Add nodes for basic logic and math operation May 19, 2025
@jtydhr88
Copy link
Contributor Author

test workflow
math-test.json

@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

Same problem I had/have with this PR. Add Type Conversion Nodes
You dont need to make single "type" nodes. These are all numerical operations, you should make a dropdown/switch to change "type" rather than add another set for FLOAT or INT or VECTOR, etc...
I explained it in more detail in this RFC: RFC Discussion: More Core Nodes

Thanks for the feedback, in fact, on my design I am using IO.NUMBER rather than int/float, I almost finish and will push soon, then we could discuss more

IO.Number doesnt support Tuple/Vector list type data. IO.Number doesnt have BOOLEAN (which is just a binary bit value).

@jtydhr88
Copy link
Contributor Author

some explanations:

  1. I am using IO.NUMBER rather than INT/FLOAT, which allow mix inputs by INT and FLOAT, however, currently, IO.NUMBER has no widget to let user edit value directly, need to connect with one INT or FLOAT, I create FE issue and will implement it later IO.NUMBER should have widget Comfy-Org/ComfyUI_frontend#3936
  2. From my perspective, I prefer to keep each operation has it own node (Add node, Sub node, etc), rather than combine them into single node with dropdown (Math Opreation node with dropdown 1. add, 2. sub),
    Here are some examples from another node systems.
  • OC render 屏幕截图 2025-05-19 140504
  • Grasshopper in Rhino
    image

@jtydhr88
Copy link
Contributor Author

some screenshots:
image
image
image
image
image
image

@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

2. From my perspective, I prefer to keep each operation has it own node (Add node, Sub node, etc), rather than combine them into single node with dropdown (Math Opreation node with dropdown 1. add, 2. sub),
   Here are some examples from another node systems.

I am well versed in DAG systems; it is the main reason I am pushing on this as a design moment and also why I adopted implicit/dynamic conversions in my own nodes. This is the modern way:

  • Blender (Shader Nodes / Geometry Nodes / Compositor)

    • single inputs/single nodes, dyanmically senses input types.
  • image

  • Houdini (VOPs / SOPs with Attribute VOPs or Wrangles -- VOP nodes are (mostly) typed, but):

    • Most nodes are polymorphic (like Add, Multiply)
    • VEX (code-based side) supports implicit conversion in some contexts
  • Unreal Engine (Material Editor / Niagara / Blueprints)

    • Many nodes handle multiple types (e.g., == can compare bools, ints, etc.)
    • Context-sensitive pins and overloaded functions
  • Unity Visual Scripting (Bolt / UnityVS)

    • Many nodes like Add, Compare, etc. support type polymorphism
    • Inputs adapt to type based on context
    • Behind the scenes: uses C# reflection for type resolution
  • TouchDesigner (Houdini-lite)

    • Type-flexible at the OP (Operator) level
    • DATs/CHOPs/TOPs handle different data types
    • Math operations usually accept compatible types and auto-coerce (e.g., scalar with vector)
  • Notch

    • Nodes can handle various types like Blender (e.g., Add node can mix scalar/vector)
  • Max/MSP / Pure Data

    • Data type flexibility built-in
    • Math/logic nodes auto-handle different scalar types
    • Graphs are loosely typed and dynamic
  • nodi (Python)

    • Minimal node graph with polymorphic types
  • Magma (Thinkbox Krakatoa)

    • High-performance DAG with smart operators

@Amorano
Copy link
Contributor

Amorano commented May 19, 2025

2. From my perspective, I prefer to keep each operation has it own node (Add node, Sub node, etc), rather than combine them into single node with dropdown (Math Opreation node with dropdown 1. add, 2. sub),

Operations I can get behind -- Having a specific operation node (subtract, add, etc...) The input part I am not behind. You dont need to have 12 operations all with different input types.

FOR EXAMPLE, are you really proposing that if I want to multiply a FLOAT with an BOOLEAN* I need 5 nodes? The FLOAT input, the BOOLEAN* input with another node to convert to FLOAT, and then the actual operation node + output?

It should just be the inputs (FLOAT, BOOLEAN, INT) with the operation node (MULTIPLY), without needing to "convert" into another type.

Same for BOOLEAN or future STRUCT/TUPLE/VECTOR support. The conversion(s) to type should be implicit inside the operation node.

Though, again, I would argue that instead of XXX operations (plus more in the future) you can consolidate into a single operation node.

Both are separate design moments.

@jtydhr88
Copy link
Contributor Author

@Amorano thanks for your sharing.
I think the essence of this question is: are we going to provide basic operators from programming languages (+, -, *, or even basic lib methods like math.abs), or do we want to provide a pre-written function, such as function - mathOperation(inputA, inputB, operation).

Of course, we could have different opinions, I think we could throw this question to others and see what they think.

@jtydhr88
Copy link
Contributor Author

just made a simple example node, which has dropdown for operations
image

Using IO.ANY, not restrict input type here.
This is an example for who are interesting on this topic.
Will convert this PR to draft

@jtydhr88 jtydhr88 marked this pull request as draft May 19, 2025 20:52
@webfiltered
Copy link
Collaborator

I agree we should not add unique individual types for every combination of low-level operation. Some nodes should just "adapt" to what they're given (when practical).

Reasoning

Too many nodes (node overload) & UX of making nodes too specific / infuriating to use because you have to manually convert things before adding them.

If we just add these nodes because it's a good first iteration, once they're in they must be supported forever; either as a node or via some auto-convert code. I don't want this to paralyse us or turn this into a major undertaking, but the quickest solution here might start negatively impacting users before we even get very far.

For

We could reduce the impact of node overload by weighting math / other simple ops to the bottom of the default sort list. These are basic concepts, typically easy to remember, and should be easy to find (e.g. search + and first result should be an add node / math node preset to add).

Against

I believe many nodes could 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 two 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.
[reworded from another thread]

@StableLlama
Copy link

As I'm missing all the Python basic functionality I made https://github.com/StableLlama/ComfyUI-basic_data_handling (or just use Basic data handling from the node registry) that aims to give you the full basic Python functionality.

For normal nodes (like maths) it's finished and right now only the nodes are require dynamic inputs are a real work in progress.

So you might want to have a look there as well. And to be honest, I'd have expected that Comfy has this functionality built in. So I'd be happy to join efforts to expand the default Comfy with this functionality (whom ever the author is, as I'm not affected by the not-invented-here-syndrome)

@Amorano
Copy link
Contributor

Amorano commented May 23, 2025

As I'm missing all the Python basic functionality I made https://github.com/StableLlama/ComfyUI-basic_data_handling (or just use Basic data handling from the node registry) that aims to give you the full basic Python functionality.

For normal nodes (like maths) it's finished and right now only the nodes are require dynamic inputs are a real work in progress.

So you might want to have a look there as well. And to be honest, I'd have expected that Comfy has this functionality built in. So I'd be happy to join efforts to expand the default Comfy with this functionality (whom ever the author is, as I'm not affected by the not-invented-here-syndrome)

I dont think there is a lack of people wanting to do the work, the problem is a design philosophy.

Looking at your node pack, you did the same thing (designwise) that was done here

image

You have multiple type nodes for the same type of operations (INT, FLOAT, BOOL). What Webfiltered mentioned above is consolidating things into singluar nodes where it makes sense (Add, Multiple, Divide are all NUMERICAL operations that work across INT, FLOAT, BOOL) so there are not 60 nodes because of a different "type" input.

All your BOOL logic operations also apply to INT and FLOAT. Numbers are numbers. Condensing these down (my goal) helps them be expandable in the future for "new" operations and reduces the need for a bunch of pre-stage convert-to-type nodes.

All that is going to happen, if there is not an alignment to collapse the types, is that people like me, and other existing packs, will continue to be used in-place of 60+ core nodes. I'd rather sunset all the value conversions and supports in my pack (even though its only 6 nodes) so that I can use core nodes, but only if I dont have to use 3x the core nodes to do the same thing.

@StableLlama
Copy link

I fully understand that there's a trade off between a few mighty nodes (i.e. one that can to many things by selecting from a COMBO) and many simple nodes with fixed functionality.

My approach was to offer the full Python API so it's also clear that I have much more nodes than those few for mathematics. In my opinion it's not bad to have many nodes - when they are clearly hierarchically structured.
The advantage is that the resulting graph is compact as you don't need to carry around all the COMBO boxes.
But that's just my reasoning, with other priorities you can come up with a different conclusion.

With my nodes there isn't much overlap between INT and FLOAT (except the obvious) and none with BOOLEAN. For maths itself (+, -, *, / isn't "maths" by Python definition...) I have a node set where I also use NUMBER and thus it's working for INT and FLOAT without any duplication.

@Amorano
Copy link
Contributor

Amorano commented May 23, 2025

I fully understand that there's a trade off between a few mighty nodes (i.e. one that can to many things by selecting from a COMBO) and many simple nodes with fixed functionality.

My approach was to offer the full Python API so it's also clear that I have much more nodes than those few for mathematics. In my opinion it's not bad to have many nodes - when they are clearly hierarchically structured. The advantage is that the resulting graph is compact as you don't need to carry around all the COMBO boxes. But that's just my reasoning, with other priorities you can come up with a different conclusion.

With my nodes there isn't much overlap between INT and FLOAT (except the obvious) and none with BOOLEAN. For maths itself (+, -, *, / isn't "maths" by Python definition...) I have a node set where I also use NUMBER and thus it's working for INT and FLOAT without any duplication.

Then you have (IMO) two design problems, one for node count and the other for locking math to only python operations.

I use plenty of math nodes in VFX flows that have nothing to do with just Python Math.

@StableLlama
Copy link

for locking math to only python operations.

Hm, I don't understand. How should e.g. a square root work without clamping the input to IO.NUMBER?
And one custom node set (or even better: a ComfyUI supplied node set) with doing Python operations doesn't prevent any other nodes to support non-python operations.

@Amorano
Copy link
Contributor

Amorano commented May 23, 2025

for locking math to only python operations.

Hm, I don't understand. How should e.g. a square root work without clamping the input to IO.NUMBER? And one custom node set (or even better: a ComfyUI supplied node set) with doing Python operations doesn't prevent any other nodes to support non-python operations.

sqrt of 1 is still a value. i.e.:

import math
math.sqrt(True)  # Returns 1.0

That is just "python" maths, and maths in reality.

Booleans are merely 0 or 1 values. 0 or 1 is still numeric. TRUE + TRUE = 2

INT and FLOAT can also do XOR, NAND, AND, OR, etc... ops. They are just bits. We XOR and OR images all day long, otherwise you wouldnt have a ton of blendmodes for image work.

@StableLlama
Copy link

You are just using an implicit cast. And using a cast my node set can do that as well:
image

Actually that's not an issue about nodes, that's an issue of the ComfyUI type system. It would need duck typing to make the node universal.

@Amorano
Copy link
Contributor

Amorano commented May 23, 2025

You are just using an implicit cast. And using a cast my node set can do that as well:

Actually that's not an issue about nodes, that's an issue of the ComfyUI type system. It would need duck typing to make the node universal.

Except that is literally what Webfiltered mentioned he didnt want to do i.e. having explicit casts for types and even where I started the conversation at the top.

I have literally already done dynamic value nodes for all my math stuff to avoid having a bunch of node type casts. This is not some unknown thing in the world of coding for the last 20+ years -- it is easily achievable here as well.

Comfy is not using some exotic types.

image
image

This is easy to do -- even in the "comfy" system.

I get that maybe they dont want 20+ ops, but we sure as heck can do more than 1:1 and type conversions.

@webfiltered
Copy link
Collaborator

And using a cast my node set can do that as well:
image

This includes the exact thing I do not want; nodes for primitive type conversion. I want to add proper type conversion support into the infrastructure; it should not require adding nodes at all.

Or put another way, making nodes purely for primitive type conversion is a hack; only required because an artificial type system has been added on top, and type conversion was never included. It will just be the node equivalent of syntactic sugar, but it is pretty fundamental imo.

@StableLlama
Copy link

In an ideal world (i.e. long term or perhaps even mid term?) we would have two things:

  1. Type hierarchy with type traits
  2. simple templates (in the sense of C++ templates)

With 1. you can define generic operations and what they require and then any signal that fulfills that can be connected.
And with 2. you could e.g. define that the type of input A must be the same as input C as well as output B. Or more simple and in out case, we could have a generic "addition" node where we say that input A and B as well as the output must be the same. This would then allow to add two INT and the output will also be INT. Or two FLOAT and the output is FLOAT. Although it's the same node.
It might even work (together with 1.) to add a INT and a FLOAT and the result will be FLOAT.

But right now we don't even have a real NUMBER, it's just an official hack. And to fake 2. you'd need custom JavaScript code.

@Amorano
Copy link
Contributor

Amorano commented May 24, 2025

But right now we don't even have a real NUMBER, it's just an official hack. And to fake 2. you'd need custom JavaScript code.

Again, a bunch of people (me included) have already done the generic part of 1. It works fine, Python already has duck typing. That isnt a "hack" that is how Python works. If you mean the "input type hack" that also isnt a "hack" you can put multiple types on inputs with a comma list for the type input -- that has been in ComfyUI since a year+.

For number 2, yes, use JS (or the new frontend make it TS). That is not an actual problem, unless you are saying you want to make it all in Python?

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