avm2: Implement Math methods in Number #21018
Conversation
jarca0123
force-pushed
the
math
branch
2 times, most recently
from
e5a86b5 to
04de8f1
Compare
|
For |
core/src/avm2/globals/math.rs
Outdated
| let n = args[0].as_f64(); | ||
| let p = args[1].as_f64(); | ||
| let n = args[0].coerce_to_number(activation)?; | ||
| let p = args[1].coerce_to_number(activation)?; |
There was a problem hiding this comment.
I think these two lines can still use as_f64, as the argument is already coerced to a Number by the ActionScript-side function signature
core/src/avm2/globals/math.rs
Outdated
| args: &[Value<'gc>], | ||
| ) -> Result<Value<'gc>, Error<'gc>> { | ||
| let input = args[0]; | ||
| let num = input.as_f64(); |
There was a problem hiding this comment.
nit: this can just be
let input = args[0].as_f64();
| return Ok(f64::NAN.into()); | ||
| } else if val > cur_max { | ||
| } else if val > cur_max | ||
| || (val == cur_max && !val.is_sign_negative() && cur_max.is_sign_negative()) |
There was a problem hiding this comment.
That's an oddly specific condition. Why does it have to be here? Can't it be somehow generalized? (Similarly the min one.)
There was a problem hiding this comment.
It handles signed zero cases. In max, positive zero takes priority, while in min, the negative zero takes priority. Rust cannot compare positive zero with negative zero, so that's why it is needed.
core/src/avm2/globals/math.rs
Outdated
| let n = args[0].coerce_to_number(activation)?; | ||
| let p = args[1].coerce_to_number(activation)?; | ||
|
|
||
| // If y is NaN, the result is NaN. |
There was a problem hiding this comment.
Comments refer to values as x and y whereas code refers to them as n and p. This should be unified somehow (it's better to use the names from docs if possible I guess).
| ) -> Result<Value<'gc>, Error<'gc>> { | ||
| let input = args[0]; | ||
| match input { | ||
| Value::Integer(i) => Ok(i.abs().into()), |
There was a problem hiding this comment.
Why do we have dedicated Integer cases? How is that observable from AS?
There was a problem hiding this comment.
There is a pushbyte instruction in the test from_avmplus/as3/Types/Number/abs, which (if I understand it correctly) pushes a byte (and therefore an integer) to the abs function. And Ruffle does seem to separate Value::Number and Value::Integer.
There was a problem hiding this comment.
However, the test verifies (when 1 is passed or when Number(-0.0) is passed), if it returns int. In other cases, it returns Number.
There was a problem hiding this comment.
I don't think this should need a special handling per-method. Just like @Lord-McSweeney said, there should probably be a "if the float is integral*, generate a Value::Integer" in impl From<f64> for Value instead. (for avmplus reference, see AvmCore::doubleToAtom which does exactly that)
(*and within 1<<28 range)
IMO adding missing methods to Number and adding new logic to core Value should go into separate PRs?
jarca0123
force-pushed
the
math
branch
2 times, most recently
from
12a7265 to
2d3d1f3
Compare
danielhjacobs
added
A-avm2
jarca0123
force-pushed
the
math
branch
2 times, most recently
from
27b2e08 to
991fee1
Compare
| // Note that Flash Math.round always rounds toward infinity, | ||
| // unlike Rust f32::round which rounds away from zero. | ||
| let ret = (x + 0.5).floor(); | ||
| Ok(ret.into()) |
There was a problem hiding this comment.
This should probably also return Ok(ret.into()) directly- as Adrian said, it'd be better to keep the Number -> int optimization a general optimization and implement it in a separate PR
Lord-McSweeney
added
the
waiting-on-author
Also fixes some tests.