-
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
Add cartographic to cartesian3 method #6163
Add cartographic to cartesian3 method #6163
Conversation
@hanbollar, thanks for the pull request! Maintainers, we have a signed CLA from @hanbollar, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Thanks @hanbollar! Please update |
Please don't merge this until we take a closer look at the implications. While it appears like a small change, it actually creates a circular dependency and the built version of cesium will most likely have issues. |
For reference, this was the PR that added Maybe the same ideas apply here? |
@mramato Thanks for the feedback! I swapped the location of this conversion method from the Cartesian3 file to the Cartographic file to bypass the circular include issue. |
Source/Core/Cartographic.js
Outdated
*/ | ||
Cartographic.toCartesian3 = function(cartographic, ellipsoid, result) { | ||
//>>includeStart('debug', pragmas.debug); | ||
if (!defined(cartographic)) { |
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.
This block can be replaced with Check.defined('cartographic', cartographic)
, which is out new way of doing parameter checking.
Source/Core/Cartographic.js
Outdated
/** | ||
* Returns a Cartesian3 position from a {@link Cartographic} input. | ||
* | ||
* @param {Cartographic} Cartographic input to be converted into a Cartesian3 output. |
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.
Lowercase c.
@mramato Updated following your comments. |
Specs/Core/CartographicSpec.js
Outdated
@@ -27,6 +27,16 @@ defineSuite([ | |||
expect(c.height).toEqual(3); | |||
}); | |||
|
|||
it('toCartesian3', function(){ |
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.
explain what your test is supposed to do: toCartesian3 converts a Cartographic to a Cartesian3
Source/Core/Cartographic.js
Outdated
@@ -146,6 +146,22 @@ define([ | |||
return result; | |||
}; | |||
|
|||
/** | |||
* Returns a Cartesian3 position from a {@link Cartographic} input. |
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.
Creates a new {@link Cartesian3} instance from a Cartographic.
No need to link to Cartographic, we're already in that class.
@ggetz Updated with your recommended changes. |
Source/Core/Cartographic.js
Outdated
* @param {Cartesian3} [result] The object onto which to store the result. | ||
* @returns {Cartesian3} The position | ||
*/ | ||
Cartographic.toCartesian3 = function(cartographic, ellipsoid, result) { |
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.
One more thing, let's be consistent with fromCartesian
and call this toCartesian
. Also keep the wording consistent with the other functions, see https://github.com/AnalyticalGraphicsInc/cesium/pull/6163/files#diff-f2664d42919c57ae2aa0b2575097bb56R54 and https://github.com/AnalyticalGraphicsInc/cesium/pull/6163/files#diff-f2664d42919c57ae2aa0b2575097bb56R82
@ggetz Thanks for the feedback - added commits to match previous conventions better |
Thanks @hanbollar, I'll merge when the build passes. |
oneliner method to shorten conversion from cartographic to cartesian3.
inside the method just calls fromRadians - instead of needing to input the cartographic values into the fromRadians specifically