Simsilica / SimMath

A double-based math package similar to JME's float-based math classes.
BSD 3-Clause "New" or "Revised" License
7 stars 4 forks source link

Vec3d violates the hashCode() contract #8

Closed stephengold closed 2 years ago

stephengold commented 3 years ago

The contract is:

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

However, Vec3d.equals() uses == which doesn't distinguish +0.0 from -0.0, even though those values are represented by different bits. As a consequence, the following test fails:

        void testHashZero() {
            Vec3d a = new Vec3d();
            Vec3d b = a.mult(-1);
            if (a.equals(b)) {
                assert a.hashCode() == b.hashCode();
            }
        }
stephengold commented 3 years ago

The current implementation of Vec3d.equals() also violates the reflexive property, since vectors containing NaN values are not equal to themselves.

The contract is:

for any non-null reference value x, x.equals(x) should return true.

Hence testEqualsNanBoth() needs to be revised as follows:

        void testEqualsNanBoth() {
            Vec3d a = new Vec3d(Double.NaN, 2, 3);
            Vec3d b = new Vec3d(Double.NaN, 2, 3);

            assert a.equals(b);
        }        
pspeed42 commented 2 years ago

Just a note about this part: "for any non-null reference value x, x.equals(x) should return true."

Strictly speaking, they DO follow this contract. x.equals(x) will return true because it does an instance check first thing.

It's x.equals(y) that will fail if x and y have "exactly the same fields but some are NaN"... and it's debatable whether or not Quatd and Vec3d should extend their sameness to the values inside. ie: if NaN != NaN then maybe Quatd(NaN) should also not equal totally different instance Quatd(NaN). Debatable but I agree that it's logical that we'd consider them equal.

Though part of me does wonder if this gets us in trouble from a math perspective somewhere down the line.

stephengold commented 2 years ago

Strictly speaking, you're right. The correct argument would be a lot more subtle.

Personally, my worry about implementing equals() using Double.compare() isn't about NaN; it's about negative zero. That's why the MyVector3f class in Heart provides eq() and ne() methods that don't distinguish 0 from -0. Perhaps SimMath should provide analogous methods.

stephengold commented 2 years ago

A related MyVector3f method that I find useful is standardize() which replaces -0 elements with 0 in preparation for hashing.