Vector2/3/4: Avoid divide-by-zero

[WestLangley]

As discussed in #11241.

This leave the zero-vector unchanged under these methods --and does so without warning.

I am not sure I feel that comfortable with it, but it appears by consensus to be an acceptable trade-off.

/ping @Mugen87 for review

[mrdoob]

/ping @brianchirls @mattdesl

[brianchirls]

Looks good.

[aardgoose]

FWIW - I was hit by this in two cases because of a model with 0 length line segments. One case being raycasting silently breaking and producing incorrect results.

[WestLangley]

Like I said, this change will hide errors: if you normalize the zero-vector, you will get the zero-vector.

Raycaster will generate incorrect results with a zero-length direction vector. Direction vectors must be unit-length in three.js.

[WestLangley]

/ping @bhouston Do you have an opinion one way or the other? I am indifferent to merging this, or leaving the code as-is.

/ping @mattdesl

[WestLangley]

/ping @bhouston @mattdesl

I would like to know what you guys think. I proposed this out of courtesy, not because I am in favor of it.

I will defer to the consensus and either merge it or close it.

[bhouston]

We should protect against divide by zeros when normalizing in a similar way to how it is handled in Industrial Light and Magic's Imath library:

https://github.com/openexr/openexr/blob/develop/IlmBase/Imath/ImathVec.h#L1191

I am confused why we are calling the more complex divideScalar( length || 1 ), everywhere instead of the higher level normalize() that has the divide by zero check inside of it. I like to have the check inside normalize() and just use that function everywhere it makes sense.

I think most of these divideScalar( length || 1 )s can be replaced with normalize().

[WestLangley]

OK, closing this.

[brianchirls]

@WestLangley: wait, why is this closed? It seems like there's some consensus here that a change should be made, even if there's some slight disagreement over the exact implementation.

[WestLangley]

@brianchirls The original issue is still open. :)

[mrdoob]

@WestLangley said:

OK, closing this.

Hmm... As far as I understand, @bhouston was in agreement with this PR?

[WestLangley]

My understanding was that @bhouston suggested a divide-by-zero check should go in normalize() instead.

Also, as I said, I was not a fan of my own PR anyway... I know that is somewhat unusual. :)

[mrdoob]

Hmm...

normalize: function () {

	return this.divideScalar( this.length() || 1 );

}

Isn't that, effectively, a divide-by-zero check in normalize()? Or do you mean throwing an error?

[WestLangley]

Isn't that, effectively, a divide-by-zero check in normalize()?

Yes, but it silently returns an incorrect answer if length is zero. I think throwing an error or warning is better.

I am confused why we are calling the more complex divideScalar( length || 1 ), everywhere ...
I think most of these divideScalar( length || 1 )s can be replaced with normalize().

I understand @bhouston's confusion now. We are not calling it everywhere -- only in methods that need to reuse the value of length. It is also being called in normalize(), as @bhouston suggests.

At the cost of some inefficiency, the test can be localized to normalize() only.

[Ludobaka]

@WestLangley said :

I understand @bhouston's confusion now. We are not calling it everywhere -- only in methods that need to reuse the value of length. It is also being called in normalize(), as @bhouston suggests.

I've already seen some 3D engine having length as a return value of normalize(). It has no side effect in any ThreeJS current code and allows to avoid using divideScalar(length || 1) when you need to keep the length for later computations. Latest example I've seen is Ogre3D, a C++ library.

With this, we could keep the test on normalize() and even improve code readability.

As the discussion seems to continue, I await your approval before contributing on this to avoid making various pull requests about the same problem. For now, I will just patch my fork.

[WestLangley]

@Ludobaka We return this so the methods can be chained.

It has no side effect in any ThreeJS current code

We use this pattern in the library:

vector.normalize().multiplyScalar( value );

Changing the return value at this point would lead to backward-compatibility issues.

[WestLangley]

@mrdoob Restored deleted branch and reopened. If you would like to merge this, I can make any additional changes you request in a follow-up PR.

[mrdoob]

Looks good!

[mrdoob]

Thanks!