-
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
make entity model matrix computation public #5584
Conversation
3b37cfe
to
7728f21
Compare
@ggetz do you want to do a first pass review? |
@rahwang Looks good to me, just update CHANGES.md |
7728f21
to
a05c5dc
Compare
Thanks @ggetz ! Done. |
@mramato did you want to look at this before merging? |
@rahwang please merge in master. |
Source/DataSources/Entity.js
Outdated
*/ | ||
Entity.prototype._getModelMatrix = function(time, result) { | ||
Entity.prototype.computeModelMatrix = function(time, result) { | ||
var position = Property.getValueOrUndefined(this._position, time, positionScratch); |
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.
Now that this is public, we should add a Check
call for time
(and unit test to go along with it) since it's required.
Source/DataSources/Entity.js
Outdated
@@ -581,9 +581,14 @@ define([ | |||
var orientationScratch = new Quaternion(); | |||
|
|||
/** | |||
* @private | |||
* Computes the model matrix for the entity's transform at specified time. | |||
* |
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.
Expand on this documentation to indicate what happens if the entity has no position or no orientation.
Source/DataSources/Entity.js
Outdated
* @param {JulianDate} time The time to retrieve model matrix for. | ||
* @param {Matrix4} [result] The object onto which to store the result. | ||
* | ||
* @returns {Matrix4} The modified result parameter or a new Matrix4 instance if one was not provided. |
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.
Here too, since undefined
can be returned.
CHANGES.md
Outdated
* Fixed documentation for `OrthographicFrustum` [#5586](https://github.com/AnalyticalGraphicsInc/cesium/issues/5586) | ||
|
||
|
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.
Whiespace
I was tempted to say let's make this a Just those couple of comments. Thanks @rahwang! |
CHANGES.md
Outdated
@@ -5,8 +5,10 @@ Change Log | |||
|
|||
* Fixed a bug where a Model's compressed textures were not being displayed. [#5596](https://github.com/AnalyticalGraphicsInc/cesium/pull/5596) | |||
* Fixed a bug where jsep was undefined when using webpack [#5593](https://github.com/AnalyticalGraphicsInc/cesium/issues/5593) | |||
* Make Entity mode matrix public via computeModelMatrix(). [#5584](https://github.com/AnalyticalGraphicsInc/cesium/pull/5584) |
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.
We usually say something like Added Entity.computeModelMatrix which ...
. End users have no idea what was private/public so you need to explain what the function is.
95535ab
to
4735c4d
Compare
Thanks @mramato . Done! |
Sorry for the delay here @rahwang! I merged in master and fixed CHANGES.md but other than that this looks great. |
Could you review, @mramato ? Thanks!