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

Fix #2198 and #2199 #2204

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix #2198 and #2199 #2204

wants to merge 2 commits into from

Conversation

ecc521
Copy link

@ecc521 ecc521 commented Jun 10, 2019 •
edited
Loading

Fix #2198 and #2199

Take angle modulus 360 before multiplying my Math.PI to avoid issues with decimal accuracy when the angle measure gets large. (#2199)
Round to 10 digits using Math.round and division instead of stringifying (#2198). This should result in some performance improvements.

@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 changed the title Update scratch3_operators.js Fix #2198 and #2199 Jun 10, 2019
@adroitwhiz
Copy link
Contributor

adroitwhiz commented Jun 10, 2019 •
edited
Loading

Thanks for fixing this! MathUtil.tan still stringifies + unstringifies, however--might want to get that fixed in this PR as well.

@ecc521
Copy link
Author

ecc521 commented Jun 10, 2019 •
edited
Loading

Done.

@thisandagain thisandagain added this to the July 2019 milestone Jun 13, 2019
@BryceLTaylor BryceLTaylor modified the milestones: July 2019, Overdue Jul 13, 2020
@AmazingMech2418
Copy link
Contributor

This seems good, I think. Only one thing that might be good is adding the modulus to the tangent as well, since that seems to have the same sorts of issues. Otherwise, I think this is ready to merge! Although it's apparently been ready to merge for almost 2 years now...

@ecc521
Copy link
Author

ecc521 commented Jun 6, 2021

@AmazingMech2418: Unless I'm missing something, tangent already does that (see MathUtil.tan - expand the second file being changed).

@AmazingMech2418
Copy link
Contributor

@AmazingMech2418: Unless I'm missing something, tangent already does that (see MathUtil.tan - expand the second file being changed).

Yeah, I just saw that. You are correct.

@ecc521
Copy link
Author

ecc521 commented Jul 20, 2022

@cwillisf
Any reason this hasn't been merged yet?
This should be a very easy PR to review and merge, as it is 3 lines of code changes, and doesn't change any behaviors (beyond improving performance trivially and reducing calculation errors with large numbers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cwillisf cwillisf

Labels
pr - needs review
Projects
None yet
Milestone
Overdue
Development

Successfully merging this pull request may close these issues.

Trig functions stringify then unstringify their inputs
7 participants
@ecc521 @adroitwhiz @AmazingMech2418 @paulkaplan @thisandagain @BryceLTaylor @cwillisf