Vector2/3/4 normalize divide by zero

[brianchirls]

Description of the problem

As a result of #10849 and 381b95a, the normalize method on vector objects no longer checks against divide-by-zero. I understand there was some discussion about whether the check for zero/nan/etc. should happen in divideScalar, and I can understand why you guys decided to leave it out.

But I think it makes less sense to remove the check for normalize, since the divide operation is not directly exposed in that case. Although a normalized zero-length vector is, as far as I can tell, undefined, it is a case that can happen quite often. And this is a pretty significant API change that will require going through a lot of code to manually add back in that check, and it's so far undocumented. It's also something that may not show up immediately in quick tests, since it's a bit of a special case.

I suggest adding back in a divide-by-zero check to normalize, but leaving it out of divideScalar. If you like, you can add a console warning that the check may be removed in the future. And I think it's significant enough to warrant a hotfix to r85.

At the very least, please update the documentation and release notes for r85.

Thanks

Three.js version

• Dev
• r85
• ...

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• macOS
• Linux
• Android
• iOS

[mattdesl]

Yup, agreed. Just got hit by this.

For what it's worth, the isFinite check (on x, y, z) that used to exist in multiplyScalar was actually crawling to the top of my profiler in some physics/performance-critical code. It makes a lot more sense to me, performance and API wise, just to check against zero in normalize() and maybe also similar functions like setLength and clampLength.

[WestLangley]

I am not a fan of returning incorrect answers without warning, but if you feel it is a reasonable trade-off in this case, one option to handling the zero-vector case is:

normalize: function () {

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

},

[brianchirls]

@mattdesl I did notice the discussion on isFinite being super slow, and I think checking against zero should be sufficient.

[mrdoob]

@WestLangley looks like a fair trade-off to me.

[manthrax]

I just got hit with this as well moving from r83 to r85.

If an object does a lookAt something else that lies along its own "up" vector, the result will be NaNs.. that then propagate through the pipeline resulting in the NaNs later throwing an exception in my AudioListener.

I agree with the choice to remove the isFinite check in divideScalar and adding a check in normalize() where the cost is already amortized somewhat by the cost of the sqrt.

I'm adding this here in case the keywords help someone else figure out this problem.

What helped me track down where this was happening was putting a bunch of "if( isNaN(value) )debugger;" checks in the setters for Vector3, Euler, Quaternion, and Matrix4.

Also wanted to mention, there is a test inside Matrix4.lookAt that attempts to detect the case of
"// eye and target are in the same vertical"
And then nudge the "z.z += 0.0001;", in a attempt to fix the situation.. but this still fails if the lookAt is along the z axis, with an up vector of 0,0,1.
So maybe that logic could bear some scrutiny....

I'm sort of wondering if it would be worthwhile to make some kind of debug mode that injects different getters and setters for those methods at runtime, and/or does extra validation, based on a debug flag? I'm guessing probably not but, tracking down the source of those NaNs was a little painful.. and it's also interesting as a more general question... for instance some kind of lint-like debug mode for validating types and checking for NaNs/Infinities, strings being passed in, etc. ;)
It would be neat to have an extra level of assurance that there aren't weird type conversions going on in the runtime... and I've seen some things like that in the wild in other peoples codebases...
anyway, just thinking out loud.

Cheers!

[WestLangley]

but this still fails if the lookAt is along the z axis, with an up vector of 0,0,1.

Right. The logic fails if the user has set .up to be the z-axis.

@manthrax Are you setting z-up for a camera -- or for other objects -- in your app? If so, do you mind explaining your use case?

[manthrax]

I am setting z-up on my camera and animated characters.

Mostly because I prefer z-up, and also because it simplified some logic in our codebase that integrated with existing floorplan layout tools.. and ammo.js for physics. I think the former could be refactored to be more up-agnostic but I am more nervous about changing the up I'm using in ammo.js. mostly because of a distant hazy memory I have of bullet physics making slight adjustments on the z position of objects to resolve interpenetration, as an institutionalized hack.

z-up also seems like a more natural up than y to me, since I sometimes use simple scene partitioning schemes based on a grid, although I can see a philosophical argument for y-up as well, namely screenspace aligning with untransformed world space.

I am however trying to make our app, up agnostic as much as possible.

x-up, however, I feel is an abomination, and should never be discussed. :)

edit: Also I just remembered another constraint that I ran into a while ago, which was picking a coordinate system used by a lot of public domain environment maps and skyboxes. But in retrospect, I can't remember if z-up ended up being aligned with those, or if I had to do extra legwork to get them to work. I remember I I had to tweak the water/reflection shader I'm using to work with z-up.

I'd also be interested to hear if you had an opinion on the selection of up vector, and any supporting reasoning?

[manthrax]

This actually reminds me of something I wanted to ask about... I was thinking of just storing a single global Vector3 as my global up vector, and then just assigning that to each object.. such that setting that single vector would instantly change Up for all instantiated objects... but this also gave me a sort of spidey sense tingle, as perhaps being a bad practice.
What do you think? @WestLangley

[WestLangley]

I am setting z-up on my camera and animated characters.

Do your animated characters use .up for anything? Are they calling lookAt()?

I was thinking of just storing a single global Vector3 as my global up vector
That is how it is implemented. You can change it:

THREE.Object3D.DefaultUp.set( 0, 0, 1 );

[manthrax]

Excellent! Thanks :) Hadn't noticed that! Also, was wondering if you had any preference/justification for a specific up axis?

[WestLangley]

I have no preference.

Are you able to answer my last question?

[manthrax]

No I am not using lookAt directly on my animated characters at present.
However in a different project that does movement along splines for many small static meshes, I use it extensively.

[manthrax]

I made a JSFiddle showing some of this new counterintuitive behavior, and also what seems like it might be a bug in DirectionalLightHelper as well...

First I set Object3D.DefaultUp.set(0,0,1)

Then I make a DirectionalLight, and a DirectionalLightHelper moving along the X axis, to eventually line up looking down the Z (DefaultUp) axis.

At the point that it is looking exactly down that axis, the DirectionalLightHelper breaks, and its values are full of NaNs...

Additionally, the DirectionalLightHelper, doesn't seem to be aiming down the correct axis that the light is pointing along... I animate a cube at the halfway point between light and 0,0,0, to illustrate that point...

Additionally when the NaN problem kicks in, I check for it, and flash the cube...

Let me know if any of this is unclear... of if I'm just using DirectionalLightHelper wrong?

http://jsfiddle.net/5wgjvLnz/19/

edit: Updated with new issues... Leaving post here in case it provides context for someone else having problems. Feel free to delete it if needed.

#11436 and
#11437

@WestLangley :)

[WestLangley]

@manthrax This is getting off-topic. Would you please move your comment to a new post and file a bug report. Thanks.

[damienmortini]

Just been hit by this as well, are we actually applying @WestLangley fix in the next release?

Thanks 🙂

EDIT: forget what I said, just saw the pull request in the other thread 👌

[mrdoob]

#11253