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

Utils.makeInt32Range is implemented incorrectly #24

Open
Barteks2x opened this issue May 6, 2021 · 2 comments
Open

Utils.makeInt32Range is implemented incorrectly #24

Barteks2x opened this issue May 6, 2021 · 2 comments

Comments

@Barteks2x
Copy link
Contributor

This has been found by DaPorkchop_ while working on another project

This method:

public static double makeInt32Range(double n) {
if (n >= 1073741824.0) {
return (2.0 * n % 1073741824.0) - 1073741824.0;
} else if (n <= -1073741824.0) {
return (2.0 * n % 1073741824.0) + 1073741824.0;
} else {
return n;
}
}

has been originally ported from the following C code in libnoise:

  inline double MakeInt32Range (double n)
  {
    if (n >= 1073741824.0) {
      return (2.0 * fmod (n, 1073741824.0)) - 1073741824.0;
    } else if (n <= -1073741824.0) {
      return (2.0 * fmod (n, 1073741824.0)) + 1073741824.0;
    } else {
      return n;
    }
  }

But because of operator precedence in java, the java code is actually equivalent to this

    public static double makeInt32Range(double n) {
        if (n >= 1073741824.0) {
            return ((2.0 * n) % 1073741824.0) - 1073741824.0;
        } else if (n <= -1073741824.0) {
            return ((2.0 * n) % 1073741824.0) + 1073741824.0;
        } else {
            return n;
        }
    }

Where in the original C code, modulo should take precedence.

For values of n between k * 1073741824 and k * 1073741824 + 536870912 for integer k >= 1 (and similar for negative range) this just happens to output the same values, but otherwise for n > 1073741824 the result will be always negative (and similarly for n < -1073741824 the result will be always positive) instead of repeating the entire range from -1073741824 to 1073741824.

Changing this now may be considered a breaking change as there could be terrain generators that would generate different terrain with this fixed.

@kashike
Copy link
Contributor

kashike commented May 6, 2021

One solution is to deprecate the existing method, and introduce a new one with the proper implementation. Another is to change this on a breaking version.

@zml2008
Copy link
Member

zml2008 commented Jul 28, 2021

We already have the beginnings of a 'compatibility mode' in #20 -- that needs to be changed to not modify global state, and the compatibility mode could additionally include this behaviour change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants