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

[Mono] Rename Fposmod to PosMod, fix output #19231

Merged
merged 1 commit into from
Jul 4, 2018

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented May 28, 2018

#18364 (comment)

  1. None of the other float-returning methods start with the letter F

  2. It's not PascalCase if you count pos and mod as words

  3. The pos distinction is not necessary because for "non-positive modulus" also known as "remainder" there's already an operator in C# called %. People using the Mathf function would only ever be looking for something that wasn't in the C# language. In the interest of clarity and somewhat keeping consistency with the C++ math, I've kept the word Pos.

  4. PosMod() looks a lot cleaner than Fposmod().

Also, I've changed the code inside the method. In my tests, this version works much better. The previous version of the code produces incorrect output for some values.

As someone who uses this function a lot in my code, I would definitely appreciate this change.

@neikeq
Copy link
Contributor

neikeq commented May 29, 2018

There is nothing in the name Mod() that tells me it's any different from the modulo operator. I'm in favor of PosMod() or PositiveMod().

@aaronfranke
Copy link
Member Author

aaronfranke commented May 29, 2018

% is not a Modulo operator. In C languages, % performs a Remainder operation. Explanation

Does it help to have inline documentation? Editors such as MonoDevelop can read this. We could add this for other methods later, if we want.

Also, I've realized it would be useful to have an integer version of this method, so I've added one.

@aaronfranke aaronfranke force-pushed the mono-fposmod-to-mod branch 3 times, most recently from 464f2ee to e9627b1 Compare May 29, 2018 20:15
@aaronfranke
Copy link
Member Author

aaronfranke commented May 29, 2018

By the way, test code:

Console.WriteLine("Common use cases, where b is positive");
Console.WriteLine("4 Mod 5 = " + Mod(4, 5));
Console.WriteLine("5 Mod 5 = " + Mod(5, 5));
Console.WriteLine("6 Mod 5 = " + Mod(6, 5));
Console.WriteLine("-6 Mod 5 = " + Mod(-6, 5));
Console.WriteLine("-5 Mod 5 = " + Mod(-5, 5));
Console.WriteLine("-4 Mod 5 = " + Mod(-4, 5));
Console.WriteLine("Rare use cases, where b is negative");
Console.WriteLine("4 Mod -5 = " + Mod(4, -5));
Console.WriteLine("5 Mod -5 = " + Mod(5, -5));
Console.WriteLine("6 Mod -5 = " + Mod(6, -5));
Console.WriteLine("-6 Mod -5 = " + Mod(-6, -5));
Console.WriteLine("-5 Mod -5 = " + Mod(-5, -5));
Console.WriteLine("-4 Mod -5 = " + Mod(-4, -5));

Output:

Common use cases, where b is positive
4 Mod 5 = 4
5 Mod 5 = 0
6 Mod 5 = 1
-6 Mod 5 = 4
-5 Mod 5 = 0
-4 Mod 5 = 1
Rare use cases, where b is negative
4 Mod -5 = -1
5 Mod -5 = 0
6 Mod -5 = -4
-6 Mod -5 = -1
-5 Mod -5 = 0
-4 Mod -5 = -4

Output should be increasing, but looping around range ([0, b)), with increasing input (a).

With old Fposmod() code:

Common use cases, where b is positive
4 Mod 5 = 4
5 Mod 5 = 0
6 Mod 5 = 1
-6 Mod 5 = 4
-5 Mod 5 = 5  # Wrong
-4 Mod 5 = 1
Rare use cases, where b is negative
4 Mod -5 = 4  # Wrong
5 Mod -5 = 0  
6 Mod -5 = 1  # Wrong
-6 Mod -5 = -6  # Very Wrong
-5 Mod -5 = -5  # Wrong
-4 Mod -5 = -9  # Very Wrong

@aaronfranke aaronfranke changed the title [Mono] Rename Fposmod to Mod [Mono] Rename Fposmod to Mod, fix output May 29, 2018
@KellyThomas
Copy link
Contributor

KellyThomas commented May 30, 2018

The example from the docs indicates that it should return within the range (0, b].

http://docs.godotengine.org/en/3.0/classes/[email protected]#class-gdscript-fposmod

@aaronfranke
Copy link
Member Author

@KellyThomas This is inconsistent with the positive values, and thus with the wording "wraps equally in positive and negative". I've submitted another PR to fix the C++ code: #19279

@aaronfranke aaronfranke force-pushed the mono-fposmod-to-mod branch from e9627b1 to 600410d Compare June 10, 2018 08:37
[Mono] Rename Fposmod to PosMod
@aaronfranke aaronfranke force-pushed the mono-fposmod-to-mod branch from 600410d to 5b2b23c Compare June 27, 2018 05:58
@aaronfranke aaronfranke changed the title [Mono] Rename Fposmod to Mod, fix output [Mono] Rename Fposmod to PosMod, fix output Jun 27, 2018
@aaronfranke
Copy link
Member Author

aaronfranke commented Jun 27, 2018

@neikeq I've changed the function name to PosMod instead of Mod as you requested.

Since you've already concluded that the fix is correct, we should merge so that we have a consistent C# API before Godot 3.1 Alpha on July 1st. Unlike in the C++ code, nothing depends on this function (yet).

Copy link
Contributor

@neikeq neikeq left a comment

Choose a reason for hiding this comment

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

Looks good to me. However, if this code is a mirror of the new implementation being introduced in #19279, then I would prefer both PRs to be merged at the same time to avoid problems.

@neikeq neikeq merged commit d4f860c into godotengine:master Jul 4, 2018
@hpvb
Copy link
Member

hpvb commented Jul 27, 2018

Cherry picked into 3.0.6

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.

5 participants