[set_variable] for floats

[Sapient]

(summary of irc conversation regarding bug #12546 follows)

Background: per AI0867, the current operations were originally integer only... they were recently hacked to accept floats too under certain conditions, but there's still tons of int casting all over the place

Proposal:
case A) string "1" op string "2" --> read both as integers, result also to integer
case B) string "1.0" op string "2" --> read both as floats, result as integer
case C) string "1" op string "2.0" --> read both as floats, result as integer
case D) string "1.0" op string "2.0" --> read both as floats, result as float

option parameter, precision=0,1,2,...etc you can trigger float mode then set the decimal places for the result, and then round to it.

I am not adamant about the return type of the B and C case but I chose it because it seems to be the path of least grief. Accidental 2.9999999 is the thing we really want to avoid, and if by lucky chance we get 3, then it will be treated as int in subsequent operations, so I think it is best to default to int for the final return type in the B and C case... and those rare persons who want a particular level of precision can set it explicitly, then there's no need to guess about any of it. You could even have ADD_FLOAT etc macros that use a default precision level.

Known Risks:
Casting 2.9999999 to int can get you a 2 rather than a 3
(this is the way it works currently)
But, regardless, I'd rather not have seperate <math>_float operations for floats. Just one attribute precision=? would be my preference, and then if you want to automatically add some convenience, with the aforementioned risks, it's an added bonus.

[ilor]

I think automatic convarsion of floating point expressions to ints is a bad idea that would be surprising and contrary to what pretty much any programming language does. I think if floats get involved, floats should end up in the result, with some sort of rounding available to users (floor/ceil/round). We could state that when wml expecting an int encounters a float it will be rounded, but I think we shouldn't do anything of the sort in the actual operations.

[Sapient]

But, rounding is not always the desired behavior... sometimes a divisor and remainder are needed.

For example:
99/10 = 9
99%10 = 1

However, I am also beginning to agree with you about "automatic convarsion of floating point expressions to ints is a bad idea." As I said, I am willing to reconsider the return type of B and C. After further thought, you have convinced me that float is a better return type in that situation.

It may be advisable to change the behavior of lexical_cast<int>(std::string&) so that it rounds instead of truncates floating poit numbers. However, there are other variants such as lexical_cast<size_t>(t_string const&), etc

[AI]

Proposal:
case A) string "5" op string "2" --> read both as integers, integer operation, default precision 0
case B) string "5.0" op string "2" --> read both as floats, float operation, default precision 0
case C) string "5" op string "2.0" --> read both as floats, float operation, default precision 5
case D) string "5.0" op string "2.0" --> read both as floats, float operation, default precision 5

Optional parameter, precision=0,1,2,...etc
If the precision is 0, cast to integer afterwards.

Casting is currently done using a mix of atof, atoi, snprintf and str_cast, I propose replacing the first three of these with atol/atoi/atof for input and str_cast for output. (which is an alias of lexical_cast<std::string>)

Addition of the 'round' operation, whose argument can the following values:
-postive integers: rounds to that many decimals. (123.195 op 1 = 123.2)
-0: rounds to an integer (4.6 op 0 = 5)
-negative integers: rounds to an integer with the absolute value of the operand's zeros (1923 op -2 = 1900)
-ceil: rounds to the smallest integer that is larger or equal
-floor: rounds to the largest integer that is smaller or equal

Possible changes:
-Case B default precision 5, less confusion and mimics C behaviour better.
-In case A, if precision > 0, perform a float operation instead.
-instead of round, the "other operations" described here.

EDIT:
It appears lexical_cast<int> has rather random behaviour if the input isn't a valid integer, while atoi simply returns 0. Changing that part of the specification.

[Sapient]

I like your proposal. This is very good. Please go ahead and implement it when you have time.

[AI]

Update:
case A) string "5" op string "2" --> read both as integers, integer operation
case B) string "5.0" op string "2" --> read both as floats, float operation
case C) string "5" op string "2.0" --> read both as floats, float operation
case D) string "5.0" op string "2.0" --> read both as floats, float operation

Casting is done using atoi/atof and str_cast

Modified operation:
-modulo can now operate on floats (using fmod())

Added operations:
-ipart: assign to the variable, the integer part of the variable referenced (as in to_variable), see modf()
-fpart: assign to the variable, the fractional part of the variable referenced (as in to_variable), see modf()
-round: rounds the the specified digits of precision, negative precision also works. Special values: 'floor' and 'ceil'


Precision was dropped as it's redundant with round.

[SlowThinker]

ipart

-ipart: assign to the variable, the integer part of the variable referenced (as in to_variable)

This is incorrect (and wiki too) in 1.8.5:
ipart requires a value as an input, not a variable:

Code: Select all

{VARIABLE aux1 3.5}
[set_variable] 
	name=aux1_new
	to_variable=aux1 
[/set_variable] 
# aux1_new=3.5

# now let us try an equal scheme with ipart:

{VARIABLE aux2 3.5}
[set_variable] 
	name=aux2_new
	ipart=aux2 
[/set_variable] 
# aux2_new=0 
# apparently this doesn't work

[set_variable] 
	name=aux3_new
	ipart=3.5 
[/set_variable] 
# aux3_new=3 this is how ipart works

I didn't test fpart.

-------------------------------------
ceil

I expected ceil worked like round, but it doesn't. And I think it would be more logical.

Code: Select all

{VARIABLE aux1 23.5}
[set_variable] 
	name=aux1
	round=-1
[/set_variable] 
# aux1=20

{VARIABLE aux2 23.5}
[set_variable] 
	name=aux2
	ceil=-1
[/set_variable] 
 # aux2=23.5 (so no operation) , but I expected 30

{VARIABLE aux3 23.5}
[set_variable] 
	name=aux3
	round=ceil
[/set_variable] 
# aux3=24 so this is how ceil works

I didn't test floor.

----------------------------------------------
Wiki
The wiki was unclear for me, and not only fpart.
I think this should be clarified in the wiki:

ipart doesn't work like to_variable.
Round adjusts a variable, while ipart/fpart creates it from an input value.
Examples would be very beneficial in the wiki.

[SlowThinker]

ipart

Code: Select all

[set_variable] 
	name=aux3_new
	ipart=3.5 
[/set_variable] 
# aux3_new=3 

Additionally the code above creates a container '3' as if the next code was executed:

Code: Select all

{VARIABLE 3.5 ""}

So I misunderstood the function or there is a bug in the ipart code.

[AI]

With regard to ipart/fpart, you understood correctly, my specification is off and the wiki was based (blindly) on that.
Ceil and floor are special values for round. Though your interpretation makes sense, that's not how it works or how it's documented.

[SlowThinker]

Additionally the code above creates a container '3' ...

I think this problem should be corrected, as it can rewrite an existing variable.

[AI]

I can't reproduce that on latest trunk.

[SlowThinker]

...
Additionally the code above creates a container '3' as if the next code was executed:

Code: Select all

{VARIABLE 3.5 ""}

1.9: I can confirm the bug doesn't appear
1.8: with negative numbers the bug prevents savefiles are loaded (containers like [-3] are not acepted)