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

Scalars used as bools should not round to int. (0.1 is coming out as false when it should be true) was: Scalars with abs <= 0.5 are false #1971

Closed
jberkman opened this issue Mar 3, 2017 · 7 comments
Assignees
Labels
bug Weird outcome is probably not what the mod programmer expected.
Milestone

Comments

@jberkman
Copy link
Contributor

jberkman commented Mar 3, 2017

print 0.5000000000000001=true.
False
print 0.500000000000001=true.
True
print -0.5000000000000001=true.
True
print -0.50000000000000001=true.
False

coming from other languages, this was quite a surprise! i'm guessing they are rounded to an int before being checked as a bool rather than being compared to 0.

kOS 1.0.3, KSP 1.2.2, x86_64, win10

@Dunbaratu
Copy link
Member

No they aren't false.

if 0.00001 { print "true".} else { print "false". }
true.
print 0.00001 = true.
False.

Comparing something to "== True" is an abomination that should never ever be done. The definition of "true" is not "is exactly equal to this value called true". The definition of "true" is "is not zero". When you do a comparison to "== true" you're really saying "is this exactly equal to 1?".

I blame newer languages that teach people there is such a thing as a value which can only have values 0 and 1, when there really isn't. Computers generally don't have bit-addressable addressing. You can't get smaller than a whole byte at a time.

@jberkman
Copy link
Contributor Author

jberkman commented Mar 3, 2017

if 0.5 print "true". else print "false".
false
if 0.51 print "true". else print "false".
true
if -0.5 print "true". else print "false".
false
if -0.51 print "true". else print "false".
true

(i stumbled upon this via an if statement, not by comparing to bools)

why is yours behaving differently? are you on mac or linux?

@Dunbaratu
Copy link
Member

Sorry, that came out meaner than it should have.

Yes, the behaviour you describe is weird and wrong, now that I look at it a second time. It's just that failing an "= true" check does not make a number false as was implied by the example. It's entirely correct for 0.5 to not equal True. The bug here is that 0.51 should also not equal True.

You are right that it's rounding first before trying to compare it, which is wrong. C# has a very complex and weird set of conversion and casting rules that don't always fire off in the way you expected them to and not always in the order you expect them to. In our type conversions we probably got one thing wrong that's causing this.

@Dunbaratu Dunbaratu added the bug Weird outcome is probably not what the mod programmer expected. label Mar 3, 2017
@Dunbaratu Dunbaratu changed the title Scalars with abs <= 0.5 are false Scalars used as bools should not round to int. (0.1 is coming out as false when it should be true) was: Scalars with abs <= 0.5 are false Mar 3, 2017
@jberkman
Copy link
Contributor Author

jberkman commented Mar 3, 2017

that's fair. a better example might be:

print 0.5<>false.
False
print 0.51<>false.
True

it's hard to argue that 0.5 should not be not equal to false.

and don't worry i know the feeling when someone posts a bug report that seems so ridiculous it can't be true.

@Dunbaratu Dunbaratu added this to the v1.1.0 milestone Apr 19, 2017
@Dunbaratu Dunbaratu self-assigned this Apr 19, 2017
@Dunbaratu
Copy link
Member

This should be easy enough to fix. It's just a matter of tracing which order the conversions are being done in. (Chances are there's no direct conversion from ScalarDoubleValue to BooleanValue defined, so it's resorting to converting from ScalarDoubleValue to ScalarIntValue to BooleanValue. At least that's my first guess before I dive into the code and test some things out.)

@hvacengi
Copy link
Member

The conversion is handled by ScalarValue's implementaton of IConvertible here: https://github.com/KSP-KOS/KOS/blob/develop/src/kOS.Safe/Encapsulation/ScalarValue.cs#L357-L361 where the value is converted to an integer prior to comparing it. If I recall correctly, the reason why I did that was to eliminate the complications introduced by floating point precision and equal comparisons. Is 0.001 close enough to zero? How about 0.0000001? We will need to establish that cutoff point or risk similar complaints that values that are essentially zero results of other operations (like subtracting 2 numbers of differing precision) do not test as false.

@Dunbaratu
Copy link
Member

I don't think there's any risk at all of those complaints. (Certainly less risk than the risk of someone knowing that it's very wrong for 0.4999 to be treated the same as a zero, which is what's happening now.)
We don't need to establish a cutoff range at all. Only if it's literally exactly zero should it be false. That's how it works in every other language that lets you use a number for its Boolean meaning.

The same people who think in terms of "0 = false, all others = true" will be quite aware of the problems that can make floating point numbers come out to 0.0000001 instead of zero. Generally speaking you'd never use a floating point value as a Boolean in cases where it's something mathematical like that (i.e. the result of adding or subtracting two numbers.) You might use it that way when it's a coefficient or something used as a setting. (i.e. "if pressure then ...." meaning "if there's anything other than total vacuum, then.." a person using it this way would be very surprised to find that a pressure of 0.4999 gets treated as zero. They wouldn't be surprised at all to find that a pressure of 0.000001 doesn't count as false. That's the normal way this sort of thing works.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Weird outcome is probably not what the mod programmer expected.
Projects
None yet
Development

No branches or pull requests

3 participants