Throw DeveloperError if normalize fails #3605

hpinkos · 2016-02-18T22:49:07Z

Fixes issue raised in #3433

A DeveloperError is raised when any of the values of normalize are NaN
(For example, when you try Cartesian3.normalize(Cartesian3.ZERO))

Blocked By:

Hole in animated ellipsoid entity #3045 Hole in dynamic ellipsoid
PolylineVolumeGeometry binormals are incorrect #3609 Incorrect PolylineVolumeGeometry binormals
WallGeometry: Check if position heights > 0 if maximumHeight is not defined #3610 Empty wall geometry heights bug
WallGeometry: Bad indices #3611 Bad wall indices

bagnell · 2016-02-19T18:37:25Z

I get 16 test failures:

hpinkos · 2016-02-19T21:01:30Z

Welp, I found a handful of geometry errors this change exposed. They'll have to be fixed before this goes in:

Hole in animated ellipsoid entity #3045 Hole in dynamic ellipsoid
PolylineVolumeGeometry binormals are incorrect #3609 Incorrect PolylineVolumeGeometry binormals
WallGeometry: Check if position heights > 0 if maximumHeight is not defined #3610 Empty wall geometry heights bug
WallGeometry: Bad indices #3611 Bad wall indices

So I'm going to put this issue on hold until I have time fix those things.
I did push some changes to a few other places to handle a zero vector. I'm not sure if they're the best fix.

mramato · 2016-02-19T21:10:18Z

The fact that this exposed 4 bugs already proves it's the right change to make. Thanks @kalmykov for bringing this to our attention! Hopefully we can get these fixed soon since some of them were user reported.

Also, 👍 for use of welp.

shunter · 2016-02-19T21:19:21Z

As a side comment, we should start adding validation like this (within debug pragmas) in many more places. Basically, throw as soon as any calculation anywhere produces a NaN. This would help to work to finally track down the dreaded "invalid array length" in the frustum code, which is where all NaNs eventually end up crashing currently. I know I've discussed this before, but I couldn't find any existing issues by searching.

mramato · 2016-02-19T21:21:51Z

I couldn't agree more. The only potential downside are module users who don't probably perform dead-code elimination and website Sandcastle demos will get a performance hit (but they are already getting that hit now because we have tons of pragmas).

pjcozzi · 2016-02-19T21:26:55Z

The only potential downside are module users who don't probably perform dead-code elimination and website Sandcastle demos will get a performance hit (but they are already getting that hit now because we have tons of pragmas).

Do we have a build step to remove pragmas for these users? Or do you think they just ignore it?

hpinkos · 2016-02-19T21:29:15Z

@shunter I created an issue here: #3613

mramato · 2016-02-19T21:31:24Z

Do we have a build step to remove pragmas for these users? Or do you think they just ignore it?

Pragma specifically is a requirejs thing, so anyone using modules with requirejs should be covered. We can't do it as part of the build because modules are just the Source directory. It has to be done during their apps own compile step (unless we wanted to ship two copies of the source tree). I believe Webpack might have some dead-code elimination features, but I don't think they support the same pragma format out of the box. I'm guessing if someone cared enough they could make it work; but as we see on the forum most of our users just use the minified Cesium.js from the start and don't even bother with the developer build that has error checking. Ultimately that's an education issue we should eventually fix with improved documentation.

pjcozzi · 2016-02-19T21:33:16Z

Ah, I see.

mramato · 2016-08-30T14:29:40Z

@hpinkos would it be possible to work around Incorrect PolylineVolumeGeometry binormals separately at the geometry level and merge this with NaN checks are the Cartesian3 level? It would be really nice to get these checks into master.

hpinkos · 2016-08-30T14:41:04Z

@mramato I might be able to do something... I mean the binormals are already incorrect, I guess making them incorrect in a different way to work around this wouldn't be the worst thing. I probably won't have a chance to really fix the geometry in the near future.

mramato · 2016-08-30T15:14:29Z

I probably won't have a chance to really fix the geometry in the near future.

Yep, that's why I was hoping for a workaround because these nan checks would go a long way to finding other unrelated bugs

mramato · 2016-10-06T13:26:26Z

We really need to get these changes into master sooner rather than later so I marked this for the bug bash. I don't care if we have to hack around PolylineVolumeGeometry to do it; there are several other bugs that would have been caught sooner if these checks were in place in master.

# Conflicts: # Specs/Scene/SkyAtmosphereSpec.js # Specs/Scene/SkyBoxSpec.js # Specs/Scene/SunSpec.js

hpinkos · 2016-10-19T15:27:37Z

@lilleyse I'm getting a whole bunch of ShadowMap test failures. I'm not sure if the tests are bad or if it's a problem with the code.

It looks like at some point sceneCamera.frustum.projectionMatrix has -Infinity in it, which then makes a bunch of things later on in fitShadowMapToScene got to NaN

lilleyse · 2016-10-19T15:42:07Z

Ok I'll take a look now.

hpinkos · 2016-10-19T15:43:26Z

Thanks! Don't have to worry about the other failing tests, I'm trying to fix those. I just have no idea what's going on with those shadow map test.

hpinkos · 2016-10-19T16:33:14Z

@bagnell this is the one test I was looking into: DataSources/KmlDataSource NetworkLink: Heading and pitch can be undefined if the camera is in morphing mode

hpinkos · 2016-10-19T19:15:36Z

Alright, all the test except the ShadowMap test are failing.
#3609 (PolylineVolumeGeometry tangets/binormals) is not fixed, but I worked around it for the specs

@lilleyse let me know when you have the ShadowMap tests fixed. Getting this change in master will be hugely beneficial for preventing future bugs.

mramato · 2016-10-19T19:20:51Z

Thanks @hpinkos huge 👍 from me! @lilleyse if you could get to this as part of the bash, that would be a huge help in tracking down some of the other issues (which are better exposed by these fixes).

pjcozzi · 2016-10-19T19:26:53Z

Source/Core/PolylineVolumeGeometry.js

+ try {
+ geometry = GeometryPipeline.computeBinormalAndTangent(geometry);
+ } catch (e) {
+ console.log('Unable to compute tangents and binormals for polyline volume geometry');


Are we sure about this change? It is atypical.

It's definitely atypical, but I don't have a better solution that won't take a ton of time. This geometry type really needs a major rewrite, but I don't think it's worth spending the time to do that right now. I think the benefits of getting this PR merged outweigh the downside of this change.

Is this to workaround tests? Would it be better to put this in the tests?

It prevents the geometry from failing if you request the tangents and binormals, both in and out of the tests.

Ah, OK. Leave it, but maybe remove the console.log or use something like deprecationWarning to avoid flooding the console.

Also use a debug pragma.

We could use oneTimeWarning and log out the url to the GitHub issue, so if people run into it (doubtful) they can comment or +1 it.

lilleyse · 2016-10-19T19:31:57Z

I figured out the ShadowMap NaN issues, but there is something else going on with terrain shadows so I'm going to open a separate shadows fix PR rather than add to this one.

mramato · 2016-10-20T14:52:29Z

Source/Core/CorridorGeometry.js

@@ -678,8 +678,9 @@ define([
 var scratchCartographicMax = new Cartographic();

 function computeRectangle(positions, ellipsoid, width, cornerType) {
- var length = positions.length - 1;
- if (length === 0) {
+ var cleanPositions = arrayRemoveDuplicates(positions, Cartesian3.equalsEpsilon);


Something weird is going on with this change. Run the below code in this branch and it will crash, however remove the first point (which is a duplicate) and everything works. So many we need to re-use the deduplicated version in other places in this file?

var viewer = new Cesium.Viewer('cesiumContainer'); var redCorridor = viewer.entities.add({ name : 'Red corridor on surface with rounded corners and outline', corridor : { positions : Cesium.Cartesian3.fromDegreesArray([ -16.41549000000001,28.44423000000001, -16.41549000000001,28.44423000000001, -16.41505000000001,28.44430000000001, -16.41507000000001,28.44430000000001, -16.41473000000001,28.44460000000001, -16.41429000000001,28.44452000000001, -16.41430000000001,28.44453000000001, -16.41428000000001,28.44452000000001, -16.41416000000001,28.44434000000001, -16.41415000000001,28.44434000000001, -16.41397000000001,28.44408000000001 ]), width : 1, height:0, material : Cesium.Color.RED.withAlpha(0.5), outline : true, outlineColor : Cesium.Color.RED } }); viewer.zoomTo(viewer.entities);

hpinkos · 2016-10-20T18:06:23Z

@mramato anything else? I think this is ready to go now. I also tested all the sandcastle examples and everything worked there.

mramato · 2016-10-20T18:21:49Z

Source/Core/IntersectionTests.js

@@ -219,6 +219,15 @@ define([
 if (!defined(v1)) {
 throw new DeveloperError('v1 is required.');
 }
+ if (!defined(p0)) {
+ throw new DeveloperError('v1 is required.');


Error messages are incorrect.

mramato · 2016-10-20T18:23:19Z

Source/Core/PolylineVolumeGeometry.js

+ try {
+ geometry = GeometryPipeline.computeBinormalAndTangent(geometry);
+ } catch (e) {
+ deprecationWarning('polyline-volume-tangent-binormal', 'Unable to compute tangents and binormals for polyline volume geometry');


Can you use oneTimeWarning here instead. It's the same thing but it's confusing to refer to this as deprecated.

mramato · 2016-10-20T18:25:21Z

Source/Scene/Camera.js

- var nUnit = Cartesian3.normalize(Cartesian3.cross(qUnit, eUnit, scratchCartesian3_4), scratchCartesian3_4);
+ var eUnit;
+ var nUnit;
+ if (Cartesian3.equalsEpsilon(qUnit, Cartesian3.UNIT_Z, CesiumMath.EPSILON10)) {


Since random epsilon values have bitten us in the past, is there any ryhme or reason for 10 over other values?

10 seems sufficiently small? We picked 10 in other places.

"We use it in other places" is fine for now.

mramato · 2016-10-20T18:25:28Z

Specs/Core/Cartesian2Spec.js

+ }).toThrowDeveloperError();
+ });
+
+


mramato · 2016-10-20T18:25:42Z

Specs/Core/IntersectionTestsSpec.js

@@ -181,18 +181,21 @@ defineSuite([
 });

 it('lineSegmentTriangle throws without p0', function() {
+ //TODO


mramato · 2016-10-20T18:26:40Z

Specs/Core/OrientedBoundingBoxSpec.js

@@ -338,6 +338,9 @@ defineSuite([
 Matrix3.multiplyByVector(axes, tang, tang);
 Matrix3.multiplyByVector(axes, binorm, binorm);
 Cartesian3.cross(tang, binorm, n);
+ if (Cartesian3.magnitude(n) === 0) {
+ return;


Can you use return undefined; here for consistency with the rest of the function.

The rule we tend to use (but doubt we've ever codified) is that we use return undefined; in cases where the function returns actual values sometimes as well.

Add it to the Coding Guide to keep our flurry of non-code PRs.

mramato · 2016-10-20T18:27:45Z

I'm way more excited about this than I probably should be. Just those comments and this is good to go.

hpinkos · 2016-10-20T18:33:06Z

@mramato anything else?

hpinkos added 2 commits February 18, 2016 17:43

Fix noramlize

85db79c

spelling

461db40

hpinkos mentioned this pull request Feb 18, 2016

Fix Cartesian.normalize to return Cartesian.ZERO for zero vector #3433

Closed

hpinkos added 2 commits February 19, 2016 13:42

Merge branch 'master' into normalizeZeroVector

3a106f4

Zero normaliztion cleanup

57c160a

hpinkos mentioned this pull request Feb 19, 2016

Check for Infinity and NaN values in calculations #3613

Closed

hpinkos added 2 commits March 1, 2016 10:48

Merge branch 'master' into normalizeZeroVector

532f88c

fix scene transforms spec

bde9b37

mramato mentioned this pull request Jun 23, 2016

PolylineVolumeGeometry binormals are incorrect #3609

Open

mramato added the bug bash label Oct 6, 2016

mramato mentioned this pull request Oct 6, 2016

Zoom Crash from Touch Input on Viewer Initialization #4177

Closed

hpinkos added 2 commits October 19, 2016 10:28

Merge branch 'master' into normalizeZeroVector

d9566ae

# Conflicts: # Specs/Scene/SkyAtmosphereSpec.js # Specs/Scene/SkyBoxSpec.js # Specs/Scene/SunSpec.js

fix some tests

25469f2

hpinkos added 3 commits October 19, 2016 15:01

workaround polyline volume geometry

f555309

fix remaining specs

8980929

remove debug statement

e116ab2

pjcozzi reviewed Oct 19, 2016

View reviewed changes

deprecation warning

9417123

mramato reviewed Oct 20, 2016

View reviewed changes

lilleyse mentioned this pull request Oct 20, 2016

Fix ShadowMap NaNs #4470

Merged

hpinkos added 4 commits October 20, 2016 13:13

Merge branch 'master' into normalizeZeroVector

5809025

Merge branch 'master' into normalizeZeroVector

4ba8776

Merge branch 'master' into normalizeZeroVector

f1826e6

actually use cleaned points in computeRectangle

7a3d630

mramato reviewed Oct 20, 2016

View reviewed changes

Specs/Core/Cartesian2Spec.js

}).toThrowDeveloperError();

});

Copy link

Contributor

mramato Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

mramato reviewed Oct 20, 2016

View reviewed changes

cleanup

01fa306

mramato merged commit bcd4503 into master Oct 20, 2016

mramato deleted the normalizeZeroVector branch October 20, 2016 18:41

Throw DeveloperError if normalize fails #3605

Throw DeveloperError if normalize fails #3605

Conversation

hpinkos commented Feb 18, 2016

bagnell commented Feb 19, 2016

hpinkos commented Feb 19, 2016

mramato commented Feb 19, 2016

shunter commented Feb 19, 2016

mramato commented Feb 19, 2016

pjcozzi commented Feb 19, 2016

hpinkos commented Feb 19, 2016 • edited Loading

mramato commented Feb 19, 2016

pjcozzi commented Feb 19, 2016

mramato commented Aug 30, 2016

hpinkos commented Aug 30, 2016

mramato commented Aug 30, 2016

mramato commented Oct 6, 2016

hpinkos commented Oct 19, 2016

lilleyse commented Oct 19, 2016

hpinkos commented Oct 19, 2016

hpinkos commented Oct 19, 2016

hpinkos commented Oct 19, 2016

mramato commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Oct 19, 2016

Choose a reason for hiding this comment

hpinkos commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Oct 20, 2016

hpinkos commented Oct 20, 2016

hpinkos commented Feb 19, 2016 •

edited

Loading