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

Fix rounding accuracy #4241

Merged
merged 7 commits into from
Dec 29, 2022
Merged

Conversation

TPGamesNL
Copy link
Member

Description

  • Removed unused methods
  • Cleaned up file
  • Adjust rounding methods with Skript.EPSILON before handling (lines 169, 184, 199, 206, 216, 226)

Target Minecraft Versions: any
Requirements: none
Related Issues: #4235

@TPGamesNL TPGamesNL added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 1, 2021
@Matocolotoe
Copy link
Contributor

Matocolotoe commented Aug 1, 2021

Well, whatever the fix, due to the limitations of floating-point numbers' representation, there will always be a "bug" somewhere.

function floor_test():
	set {_a} to 3.9999999999
	set {_b} to floor({_a})
	broadcast {_a}.toString()
	broadcast {_b}.toString()

image

@TPGamesNL
Copy link
Member Author

While you could call that behaviour a bug, 3.9999999999 = 4 according to Skript (as seen in Javadoc of Skript.EPSILON), so this is to be expected. We have to draw the line somewhere, and Njol (probs) decided 10^-10 was the width of that line

@Matocolotoe
Copy link
Contributor

Right, I guess it's still better than having inconsistencies occurring at 2 decimal places.

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for this?

@TPGamesNL TPGamesNL requested a review from APickledWalrus July 17, 2022 09:04
Copy link
Contributor

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding some floor function tests

Add test case for floor function
@TPGamesNL TPGamesNL force-pushed the fix/rounding-accuracy branch from fa940db to 56a4d0f Compare December 26, 2022 15:00
@TheLimeGlass TheLimeGlass merged commit ee35033 into SkriptLang:master Dec 29, 2022
@TPGamesNL TPGamesNL deleted the fix/rounding-accuracy branch December 29, 2022 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants