SpongePowered / noise

Noise generation library for Java, based on the libnoise C++ library. It is used to generate coherent noise, a type of smoothly-changing noise. It can also generate Perlin noise, ridged multifractal noise, and other types of coherent noise.
MIT License
113 stars 15 forks source link

Utils.makeInt32Range is implemented incorrectly #24

Open Barteks2x opened 3 years ago

Barteks2x commented 3 years ago

This has been found by DaPorkchop_ while working on another project

This method:

https://github.com/SpongePowered/noise/blob/213f04e0fc78bd19160c5700981b0620f45e9f98/src/main/java/org/spongepowered/noise/Utils.java#L114-L122

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 commented 3 years ago

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 commented 3 years ago

We already have the beginnings of a 'compatibility mode' in https://github.com/SpongePowered/noise/pull/20 -- that needs to be changed to not modify global state, and the compatibility mode could additionally include this behaviour change.