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.

Sign up for GitHub

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

Jump to bottom

Trig functions stringify then unstringify their inputs #2198

Open
adroitwhiz opened this issue Jun 6, 2019 · 0 comments · May be fixed by #2204
Open

Trig functions stringify then unstringify their inputs #2198

adroitwhiz opened this issue Jun 6, 2019 · 0 comments · May be fixed by #2204
Labels
bug developer facing help wanted performance priority 3
Milestone
Backlog

Comments

@adroitwhiz
Copy link
Contributor

Expected Behavior

The words "string" and "trigonometric function" should not be anywhere near each other

Actual Behavior

The current implementations of the sin, cos, and tan functions (but not the asin, acos, and atan functions) stringify inputs to 10 decimal digits then un-stringify them.

This was introduced in #422 so that values which should be 0 but aren't due to rounding error (e.g. sin(180)) are rounded to 0. There's likely a much faster way to do this, though (see this benchmark).
ecc521 added a commit to ecc521/scratch-vm that referenced this issue Jun 10, 2019
@ecc521
Update scratch3_operators.js
6ec974f 
Fix scratchfoundation#2198 and scratchfoundation#2199

Take angle modulus 360 to avoid issues with floating point decimal accuracy. 
Round to 10 digits using Math.round and division instead of stringifying.
@ecc521 ecc521 linked a pull request Jun 10, 2019 that will close this issue
Open
@thisandagain thisandagain added this to the Backlog milestone Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug developer facing help wanted performance priority 3
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Fix #2198 and #2199 ecc521/scratch-vm
3 participants
@thisandagain @BryceLTaylor @adroitwhiz