doubt on float/double comparing with 0.0 for irrlicht 1.7.3

[wanliqun]

I'm trying to update irrlicht 1.7.2 to the svn version.
When I'm trying to merge my code with the svn version, I found some update like:

In irricht 1.7.2 vector2d.h

Code: Select all

 
        //! Normalize the vector.
        /** The null vector is left untouched.
        \return Reference to this vector, after normalization. */
        vector2d<T>& normalize()
        {
                f32 length = (f32)(X*X + Y*Y);
                if (core::equals(length, 0.f))
                        return *this;
                length = core::reciprocal_squareroot ( length );
                X = (T)(X * length);
                Y = (T)(Y * length);
                return *this;
        }
 

in irrlicht svn(1.7.3 or higher) vector2d.h:

Code: Select all

 
      //! Normalizes the vector.
       /** In case of the 0 vector the result is still 0, otherwise
       the length of the vector will be 1.
         \return Reference to this vector after normalization. */
        vector3d<T>& normalize()
          {
                 f64 length = X*X + Y*Y + Z*Z;
                  if (length == 0 ) // this check isn't an optimization but prevents getting NAN in the sqrt.
                         return *this;
                   length = core::reciprocal_squareroot(length);
 
                   X = (T)(X * length);
                   Y = (T)(Y * length);
                   Z = (T)(Z * length);
                    return *this;
          }
 

my doubt is the float/double comparing here:
if (core::equals(length, 0.f))
if (length == 0 )
Seems 1.7.2 did the right things, Any reason for this update?

[CuteAlien]

Yes, because it was the wrong thing which did lead to wrong results: http://sourceforge.net/tracker/?func=de ... tid=540676

There's 2 things here - first a comparison against 0 (not 0.0 by the way) is valid even for floats. And the reason for this check was not to get around some floating-point inaccuracies (which is pretty much the only reason to use equals instead), but to avoid a division by zero - which does not happen with near-0 values.

So that was the reasoning - it only seems to have produced wrong results for short vectors and has no advantage I could see. Now if you got an example where this fails I will certainly change it again. There was unfortunately no reason given in the log when this got added, so I can't really tell what the point of it was.

Test:

Code: Select all

 
        // trick from http://randomascii.wordpress.com/2012/01/11/tricks-with-the-floating-point-format/
        union Float_t
        {
                Float_t(float f1) : f(f1) {}
                Float_t(int i1) : i(i1) {}
                int i;
                irr::f32 f;
        };
        Float_t x(1);
        irr::core::vector3df v3(x.f, 0.f, 0.f);
        irr::core::vector2df v2(x.f, 0.f);
        v3.normalize();
        v2.normalize();
 

Result is that it's fine even for the smallest possible float - except that we could make vector2d::normalize also use an f64 as length (instead of f32) like the vector3d version does to get even better results.

[wanliqun]

Thanks for the link. That seems to be convincing.