go-gl / mathgl

A pure Go 3D math library.
BSD 3-Clause "New" or "Revised" License
554 stars 65 forks source link

FloatEqualThreshhold broken ? #51

Open ghost opened 8 years ago

ghost commented 8 years ago

I'm first going to ask you guys to confirm this before marking this as a bug. But shouldn't this FloatEqualThreshold(8.742278e-08, 0, 1e-4) return true ? (because it doesn't)

ghost commented 8 years ago

Ok so i looked up where you guys found the implementation for this function and the original author thinks that it's ok that pure 0 isn't equal to something very very close to zero.

    assertFalse(nearlyEqual(0.00000001f, 0.0f));
    assertFalse(nearlyEqual(0.0f, 0.00000001f));
    assertFalse(nearlyEqual(-0.00000001f, 0.0f));
    assertFalse(nearlyEqual(0.0f, -0.00000001f));

Personally I can't agree with that but I don't want to change the lib if you guys think so.

ghost commented 8 years ago

UPDATE: ok i digged in deeper and realised that epsilon passed to FloatEqualThreshold isnt the max difference between a and b. This should be mentioned and it should be clear how it scales with numbers.

ghost commented 8 years ago

Also, the declaration in java is

public static boolean nearlyEqual(float a, float b, float epsilon) {
    final float absA = Math.abs(a);
    final float absB = Math.abs(b);
    final float diff = Math.abs(a - b);

    if (a == b) { // shortcut, handles infinities
        return true;
    } else if (a == 0 || b == 0 || diff < Float.MIN_NORMAL) {
        // a or b is zero or both are extremely close to it
        // relative error is less meaningful here
        return diff < (epsilon * Float.MIN_NORMAL);
    } else { // use relative error
        return diff / Math.min((absA + absB), Float.MAX_VALUE) < epsilon;
    }
}

while ours is

func FloatEqualThreshold(a, b, epsilon float32) bool {
    if a == b { // Handles the case of inf or shortcuts the loop when no significant error has     accumulated
        return true
    }

    diff := Abs(a - b)
    if a*b == 0 || diff < MinNormal { // If a or b are 0 or both are extremely close to it
        return diff < epsilon*epsilon
    }

    // Else compare difference
    return diff/(Abs(a)+Abs(b)) < epsilon
}

Any reason we do epsilon*epsilon instead of epsilon*MinNormal?

ghost commented 8 years ago

We should add a function that takes a target float, a "biggest difference" and returns a epsilon

slimsag commented 8 years ago

@hydroflame I haven't read everything here -- but you may be looking for relative equality, e.g. https://github.com/azul3d/lmath/blob/master/math.go#L12-L22 (http://realtimecollisiondetection.net/blog/?p=89)

ghost commented 8 years ago

Yeah we still need a clearer documentation