Divide-by-0 bugs in Vector*.Normalize

1

Hi, I’ve just started using MonoGame today (but I used XNA previously, and I’ve many years as a C++ dev). I think I found some bugs in Vector2/Vector3, was just looking for some feedback before I make a pull-request.

Vector2 Normalize() has a divide-by-0 bug:

public void Normalize()
{
    float val = 1.0f / (float)Math.Sqrt((X * X) + (Y * Y));
    X *= val;
    Y *= val;
}

It’s easy to see if X and Y are 0 then val will be NaN, and then X and Y are both set to NaN. You have to check for NaN/0. I’d suggest:

public void Normalize()
{
    float val = 1.0f / (float)Math.Sqrt((X * X) + (Y * Y));

    if (float.IsNaN(val))
    {
        val = 0;
    }

    X *= val;
    Y *= val;
}

Does that seem reasonable? Vector3 and Vector4 also contain the same bug, where ‘factor’ can be 0. Vector3 looks like:

Distance(ref value, ref zero, out factor);
factor = 1f / factor;

And Vector4 looks like:

DistanceSquared(ref vector, ref zeroVector, out factor);
factor = 1f / (float)Math.Sqrt(factor);

I also find it bizarre that all 3 implementations use 3 different ways of achieving the same result; can anyone shed some light on that?

2

I thought about this some more and remembered that a unit vector must always have a length of 1, and of course the length of (0, 0) is 0, so normalising (0, 0) is essentially undefined. I guess that’s why it’s implemented as such.

The differences in implementation as still annoying me though.

3

Vector2 Normalize() has a divide-by-0 bug:

If I remember correctly that matches XNA behavior. Normalizing a zero length vector will result in a NaN vector. It is always been the responsibility of the caller to be sure a vector is not zero length.

Looking at our unit tests it seems there isn’t one specifically for Normalize()…

https://github.com/mono/MonoGame/blob/develop/Test/Framework/Vector2Test.cs

So at the very least a PR that adds the correct unit tests on normalize that verifies/fixes things to XNA behavior would be welcomed.

I also find it bizarre that all 3 implementations use 3 different ways of achieving the same result; can anyone shed some light on that?

Different people implemented them. I don’t see a problem with making them all like the Vector2 implementation without internal calls to distance methods. We would need to see new unit tests added to these as well.

4

Yep I agree, if that’s the expected behaviour then so be it. I’ll have a go with some unit tests then, thanks for the help :slight_smile:

1 Like
5

I don’t think that just leaving a float as NaN is a good idea. The problem is that in Farseer physics (or any other sufficiently complex code) there are a lot of steps to solving something, so when I detect a NaN near the top it takes me around half an hour to dig into it and find exactly which place was first causing the NaN. I can’t imagine any scenario where a developer wanted to end up with a float value of NaN. Thankfully I am using my own fork of monogame so I can implement it however I like but it’s still a pain to have to dig in and find these things when I just want it to work so I can work on my game.

I hope you don’t take this as a personal criticism as I really appreciate all the work you have done on Monogame, Thank You Tom.

6

I totally understand. It is all technical criticism and I learned a long time ago to not to be personally offended when someone criticizes code. Code is meant to be criticized and changed as needed.

I agree… I wouldn’t have made the same choice as the XNA team there. The problem is changing the behavior of Nomalize() even if it is just an improvement can break existing games that depend on the bad behavior.

The best thing we can do is document this bad behavior well on the method and add like NormalizeSafe() that has the preferred better behavior.

7

huh… How is this different from the “regular” cant divide by zero problem? I mean, that’s what this is right? Not a bug, so much as a fact of life?