danielmansson / Unity.Mathematics.FixedPoint

Fixed-point extension of Unity's C# math library based on FixedMath.Net and Unity.Mathematics.
MIT License
157 stars 37 forks source link

GetHashCode() broken for all vector types #11

Open aroman opened 3 years ago

aroman commented 3 years ago

Hi there,

First of all, thank you for the library! It has saved me quite a lot of work, and is generally working very well for my needs.

However, one issue I ran into is that this library does not implementGetHashCode() correctly for vector types (fp works fine, other types e.g. fp2 do not).

Specifically, the hashing function for vector types only considers the fractional part of the fixedpoint value; that is, it ignores the decimal part entirely and only considers the integral part in the hash. So, it's quite useless as a hash function, as any vector whose components are integral values will have the same hash code.

Digging a bit into the code, this happens because the fp type stores its data as a long (RawValue), which is an 8-byte type. However, the hash algorithm by Unity.Mathematics involves converting the component data to uint, which is only 4 bytes:

public static uint hash(fp2 v)
 {
    return math.csum(fpmath.asuint(v) * uint2(0x6E624EB7u, 0x7383ED49u)) + 0xDD49C23Bu;
}

So, when casting the fp to a uint, the upper 4 bytes of the long (containing the integral part of the FixedPoint value) are silently truncated: https://github.com/danielmansson/Unity.Mathematics.FixedPoint/blob/d44836cab621f299d6d1bfa275daa437aafc739b/Unity.Mathematics.FixedPoint/fpmath.cs#L28

In my code I was able to work around this with an extension method. it's probably not the best implementation, but it works fine in my testing. Perhaps it's helpful to you or someone else:

public static int HashFp2(fp2 value)
{
    return CombineHashCodes(value.x.GetHashCode(), value.y.GetHashCode());
}

public static int CombineHashCodes(int a, int b)
{
    unchecked
    {
        var hash = 17;
        hash = hash * 31 + a;
        hash = hash * 31 + b;
        return hash;
    }
}

I should note that this works because the fp hash function is NOT broken, since it just calls the C# built-in GetHashCode() for the backing RawValue long :

public override int GetHashCode()
{
    return m_rawValue.GetHashCode();
}

https://github.com/danielmansson/Unity.Mathematics.FixedPoint/blob/master/Unity.Mathematics.FixedPoint/fp/fp.cs#L1003

Finally, I'll note that the right way to solve this is probably to base this library on Unity.Mathematic's double-based codegen, rather than the float based one. The former has a hash algorithm which is able to handle 8-byte values like double and long, apparently with this clever method:

internal static uint fold_to_uint(double x)  // utility for double hashing
{
    LongDoubleUnion u;
    u.longValue = 0;
    u.doubleValue = x;
    return (uint)(u.longValue >> 32) ^ (uint)u.longValue;
}

I'm not 100% sure how to make a PR that would do that, since I think it might break everything 😅