-
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
BoundingSphere and Cartesian functions #548
Conversation
result = new BoundingSphere(); | ||
} | ||
|
||
var center = Cartesian3.add(corner, oppositeCorner); |
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.
You could use result.center
as the result
to add
to avoid creating one or two new Cartesian3
s.
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.
Yeap. Updated. Please retest; I used the GitHub editor. Questionable, I know.
Just the one comment. Otherwise, looks good and the tests pass. |
Tests pass. Merging. |
BoundingSphere and Cartesian functions
}); | ||
|
||
it('fromCornerPoints with a result parameter', function() { | ||
var sphere = new BoundingSphere; |
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.
The first time the new ant jsHint
command caught something for me 😄 I'll fix this since you sounded like you were busy.
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.
Cool! Thanks.
Part of the models branch:
BoundingSphere.fromCornerPoints
.fromArray
anddistance
functions toCartesian2
,Cartesian3
, andCartesian4
.Longer-term we want to masquerade arrays as Cartesians, but these functions are orthogonal to that effort.