Skip to content
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

Fix negate node not converting bool to the appropriate float number. #2157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MineBill
Copy link
Contributor

@MineBill MineBill commented Jan 4, 2024

Fixes #2156.

This makes sure that a false boolean is turned into -1.0f so that when the negate operation is run, it will turn to 1(true). Otherwise, a false boolean would be converted to 0 and stay 0 after the negate operation.

@mafiesto4 mafiesto4 added fix visject Visject surface used by materials, particles, visual scripts and more labels Jan 4, 2024
@mafiesto4
Copy link
Member

I'm not sure if it's the best way to fix this. Have you tested other math nodes like Abs if they work fine for this? Like abs(false)=false and abs(true)=true?

@mafiesto4 mafiesto4 added this to the 1.8 milestone Jan 4, 2024
@MineBill
Copy link
Contributor Author

MineBill commented Jan 4, 2024

You are right, this will break other math nodes. But it got me thinking: Are there well defined math operations on bools? Booleans already have their own operations but what exactly "should" happen when we try to apply "math" to them? As demonstrated here, assuming true == 1 and false == 0, doesn't always work but neither does true == 1 and false == -1. Is it maybe a better idea to not allow booleans to be used with math nodes?

@envision3d
Copy link
Contributor

I honestly wasn't even aware that this node was supposed to return a number as the red result port suggested to me that it would be a bool. Is there a problem there? Or was that a false assumption on my part?

@MineBill
Copy link
Contributor Author

MineBill commented Jan 5, 2024

I honestly wasn't even aware that this node was supposed to return a number as the red result port suggested to me that it would be a bool. Is there a problem there? Or was that a false assumption on my part?

Negate is a math node and Flax will treat booleans as floats by turning true to 1 and false to 0 before passing them to any math node. In some cases it will work as "expected", in some cases it will not. The problem is that not all math operations will produce the expected boolean result. The bigger problem is that there is no "formal" definition of what all math nodes should do on boolean inputs. What should atan2(false) return? It's more a problem of expectations.

What you experienced on your issue is not a technical bug. Knowing that Flax turns boolean to floats before performing the negation (and then turning them back to booleans), the result makes sense.

@nothingTVatYT
Copy link
Contributor

Is it maybe a better idea to not allow booleans to be used with math nodes?

I think that would be the most clean way. Most math functions simply don't make sense on a boolean unless you know how Flax handles them internally. That knowledge shouldn't be required though. Somebody with less insight in Flax but a basic math background wouldn't and shouldn't expect that e.g. abs(true) or atan2(false) is anything but undefined.

@mafiesto4 mafiesto4 removed this from the 1.8 milestone Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix visject Visject surface used by materials, particles, visual scripts and more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negate node does not work in an AnimationGraph
4 participants