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

[Core] [Math] Fix fposmod() function #19279

Merged
merged 1 commit into from
Jul 4, 2018

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented May 30, 2018

The output of the fposmod function is wrong. According to the documentation, the value "wraps equally in positive and negative". The docs describe correct behavior in the wording, but we can see that this is not the actual behavior with the example output and test code. Code:

	var i = -10;
	while i < 11:
		prints(i, fposmod(i, 5))
		i += 1

Output:

-10 5  # Wrong
-9 1
-8 2
-7 3
-6 4
-5 5   # Wrong
-4 1
-3 2
-2 3
-1 4
0 0    # Correct
1 1
2 2
3 3
4 4
5 0    # Correct
6 1
7 2
8 3
9 4
10 0   # Correct

In the positives, the value wraps on a range of [0, b) (0 inclusive, b exclusive), while in the negatives, the value wraps around the range (0, b] (0 exclusive, b inclusive). This is incorrect behavior. The values should wrap around in the same way in both the positives and negatives. Furthermore, passing the output back into the function should return the same value again.

In fact, the whole implementation of the function is wrong. In theory returning [0, b) means negative values for b would give negative results, but very strange results happen when I pass -5.

-6 -6
-5 -5
-4 -9
-3 -8
-2 -7
-1 -6
0 0
1 1
2 2
3 3
4 4
5 0

This is the correct implementation of fposmod over fmod, always on the range [0, b):

float p_value = Math::fmod(p_x, p_y);
if ((p_value < 0 && p_y > 0) || (p_value > 0 && p_y < 0)) {
	p_value += p_y;
}
return p_value;

The only weird thing, is that the GDscript test code prints negative zero for some reason? I'm probably missing some minor tweak to make it appear as positive zero. EDIT: Adding 0.0 removes the negative sign.

-5 0
-4 1
-3 2
-2 3
-1 4
0 0
1 1
2 2
3 3
4 4
5 0

@aaronfranke
Copy link
Member Author

Can we get this PR merged this week? I believe the code is good as-is. Thanks.

@neikeq neikeq requested a review from reduz June 20, 2018 12:52
@neikeq
Copy link
Contributor

neikeq commented Jun 20, 2018

Although the fix may be right, the change could still cause regressions since the engine uses this function in several places. We need to make sure none of these places was relying on the wrong outputs for correct behavior.

@mhilbrunner mhilbrunner merged commit c538f2f into godotengine:master Jul 4, 2018
@aaronfranke aaronfranke deleted the core-fposmod-fix branch July 8, 2018 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants