-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throw DeveloperError if normalize fails #3605
Changes from 12 commits
85db79c
461db40
3a106f4
57c160a
532f88c
bde9b37
d9566ae
25469f2
f555309
8980929
e116ab2
9417123
5809025
4ba8776
f1826e6
7a3d630
01fa306
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -219,6 +219,15 @@ define([ | |
if (!defined(v1)) { | ||
throw new DeveloperError('v1 is required.'); | ||
} | ||
if (!defined(p0)) { | ||
throw new DeveloperError('v1 is required.'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error messages are incorrect. |
||
} | ||
if (!defined(p1)) { | ||
throw new DeveloperError('v1 is required.'); | ||
} | ||
if (!defined(p2)) { | ||
throw new DeveloperError('v1 is required.'); | ||
} | ||
//>>includeEnd('debug'); | ||
|
||
var ray = scratchLineSegmentTriangleRay; | ||
|
@@ -605,9 +614,11 @@ define([ | |
var position = ray.origin; | ||
var direction = ray.direction; | ||
|
||
var normal = ellipsoid.geodeticSurfaceNormal(position, firstAxisScratch); | ||
if (Cartesian3.dot(direction, normal) >= 0.0) { // The location provided is the closest point in altitude | ||
return position; | ||
if (!Cartesian3.equals(position, Cartesian3.ZERO)) { | ||
var normal = ellipsoid.geodeticSurfaceNormal(position, firstAxisScratch); | ||
if (Cartesian3.dot(direction, normal) >= 0.0) { // The location provided is the closest point in altitude | ||
return position; | ||
} | ||
} | ||
|
||
var intersects = defined(this.rayEllipsoid(ray, ellipsoid)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ define([ | |
'./CornerType', | ||
'./defaultValue', | ||
'./defined', | ||
'./deprecationWarning', | ||
'./DeveloperError', | ||
'./Ellipsoid', | ||
'./Geometry', | ||
|
@@ -32,6 +33,7 @@ define([ | |
CornerType, | ||
defaultValue, | ||
defined, | ||
deprecationWarning, | ||
DeveloperError, | ||
Ellipsoid, | ||
Geometry, | ||
|
@@ -163,7 +165,13 @@ define([ | |
} | ||
|
||
if (vertexFormat.tangent || vertexFormat.binormal) { | ||
geometry = GeometryPipeline.computeBinormalAndTangent(geometry); | ||
try { | ||
geometry = GeometryPipeline.computeBinormalAndTangent(geometry); | ||
} catch (e) { | ||
deprecationWarning('polyline-volume-tangent-binormal', 'Unable to compute tangents and binormals for polyline volume geometry'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use |
||
//TODO https://github.com/AnalyticalGraphicsInc/cesium/issues/3609 | ||
} | ||
|
||
if (!vertexFormat.tangent) { | ||
geometry.attributes.tangent = undefined; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2846,8 +2846,15 @@ define([ | |
var qUnit = Cartesian3.normalize(q, scratchCartesian3_2); | ||
|
||
// Determine the east and north directions at q. | ||
var eUnit = Cartesian3.normalize(Cartesian3.cross(Cartesian3.UNIT_Z, q, scratchCartesian3_3), scratchCartesian3_3); | ||
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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since random epsilon values have bitten us in the past, is there any ryhme or reason for 10 over other values? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 10 seems sufficiently small? We picked 10 in other places. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "We use it in other places" is fine for now. |
||
eUnit = new Cartesian3(0, 1, 0); | ||
nUnit = new Cartesian3(0, 0, 1); | ||
} else { | ||
eUnit = Cartesian3.normalize(Cartesian3.cross(Cartesian3.UNIT_Z, qUnit, scratchCartesian3_3), scratchCartesian3_3); | ||
nUnit = Cartesian3.normalize(Cartesian3.cross(qUnit, eUnit, scratchCartesian3_4), scratchCartesian3_4); | ||
} | ||
|
||
// Determine the radius of the 'limb' of the ellipsoid. | ||
var wMagnitude = Math.sqrt(Cartesian3.magnitudeSquared(q) - 1.0); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -303,6 +303,13 @@ defineSuite([ | |
expect(cartesian).toEqual(expectedResult); | ||
}); | ||
|
||
it('normalize throws with zero vector', function() { | ||
expect(function() { | ||
Cartesian2.normalize(Cartesian2.ZERO, new Cartesian2()); | ||
}).toThrowDeveloperError(); | ||
}); | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whitespace |
||
it('multiplyComponents works with a result parameter', function() { | ||
var left = new Cartesian2(2.0, 3.0); | ||
var right = new Cartesian2(4.0, 5.0); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,18 +181,21 @@ defineSuite([ | |
}); | ||
|
||
it('lineSegmentTriangle throws without p0', function() { | ||
//TODO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? |
||
expect(function() { | ||
IntersectionTests.lineSegmentTriangle(new Cartesian3(), new Cartesian3()); | ||
}).toThrowDeveloperError(); | ||
}); | ||
|
||
it('lineSegmentTriangle throws without p1', function() { | ||
//TODO | ||
expect(function() { | ||
IntersectionTests.lineSegmentTriangle(new Cartesian3(), new Cartesian3(), new Cartesian3()); | ||
}).toThrowDeveloperError(); | ||
}); | ||
|
||
it('lineSegmentTriangle throws without p2', function() { | ||
//TODO | ||
expect(function() { | ||
IntersectionTests.lineSegmentTriangle(new Cartesian3(), new Cartesian3(), new Cartesian3(), new Cartesian3()); | ||
}).toThrowDeveloperError(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use The rule we tend to use (but doubt we've ever codified) is that we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add it to the Coding Guide to keep our flurry of non-code PRs. |
||
} | ||
Cartesian3.normalize(n, n); | ||
|
||
Cartesian3.add(p0, center, p0); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?