Skip to content
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

Removes frameState parameter from evaluate and evaluateColor #6890

Merged
merged 6 commits into from
Aug 14, 2018

Conversation

janeyx99
Copy link
Contributor

@janeyx99 janeyx99 commented Aug 7, 2018

This addresses issue #6874 and removes mentions of frameState in evaluate and evaluateColor. This helps keep frameState private.

However, frameState still appears in the documentation for ImageryLayer.js. In the constructor, some of the parameters give the user the option of taking in a function that has the signature function(frameState, layer, x, y, level). I'm not sure if frameState is needed here, and I can't find where the source code evaluates these functions.

@cesium-concierge
Copy link

Thanks for the pull request @janeyx99!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

lilleyse commented Aug 7, 2018

Thanks @janeyx99, I'll take a look later!

@lilleyse
Copy link
Contributor

lilleyse commented Aug 9, 2018

This looks really good overall. Thanks for catching some of those extra typos.

A couple things:

Throughout applyStyle no longer needs to take a frameState. Some examples:
https://github.com/janeyx99/cesium/blob/fix-6874/Source/Scene/Vector3DTilePoints.js#L316
https://github.com/janeyx99/cesium/blob/fix-6874/Source/Scene/Cesium3DTileBatchTable.js#L547

Just two areas I noticed where frameState still needed to be removed:
https://github.com/janeyx99/cesium/blob/fix-6874/Source/Scene/Vector3DTilePoints.js#L375
https://github.com/janeyx99/cesium/blob/fix-6874/Apps/Sandcastle/gallery/3D%20Tiles%20Interactivity.html#L148

Add a note to CHANGES under Breaking Changes that mentions this change. I think it's okay that this change doesn't go through a deprecation period. This isn't a widely used feature and it will be a hassle to add all the deprecation warnings.

@janeyx99
Copy link
Contributor Author

@lilleyse Hey Sean, I ended up realizing applyStyle never needed frameState so I removed all instances of that as well.

Just two areas I noticed where frameState still needed to be removed: ...

Thanks for catching the other evaluate(..frameStates...)!

Is what I have for CHANGES.md okay? I'm not sure if I have to mention the deprecation period part.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

The code changes look good. Just a couple comments on the CHANGES.md entry.

CHANGES.md Outdated
@@ -6,6 +6,7 @@ Change Log
##### Breaking Changes :mega:
* Removed `ClippingPlaneCollection.clone` [#6872](https://github.com/AnalyticalGraphicsInc/cesium/pull/6872)
* Changed `Globe.pick` to return a position in ECEF coordinates regardless of the current scene mode. This will only effect you if you were working around a bug to make `Globe.pick` work in 2D and Columbus View. Use `Globe.pickWorldCoordinates` to get the position in world coordinates that correlate to the current scene mode. [#6859](https://github.com/AnalyticalGraphicsInc/cesium/pull/6859)
* Removed the unused `frameState` parameter in `evaluate`, `evaluateColor` and `applyStyle` functions [#6890](https://github.com/AnalyticalGraphicsInc/cesium/pull/6890).
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to mention applyStyle since it is a private function.

CHANGES.md Outdated
@@ -6,6 +6,7 @@ Change Log
##### Breaking Changes :mega:
* Removed `ClippingPlaneCollection.clone` [#6872](https://github.com/AnalyticalGraphicsInc/cesium/pull/6872)
* Changed `Globe.pick` to return a position in ECEF coordinates regardless of the current scene mode. This will only effect you if you were working around a bug to make `Globe.pick` work in 2D and Columbus View. Use `Globe.pickWorldCoordinates` to get the position in world coordinates that correlate to the current scene mode. [#6859](https://github.com/AnalyticalGraphicsInc/cesium/pull/6859)
* Removed the unused `frameState` parameter in `evaluate`, `evaluateColor` and `applyStyle` functions [#6890](https://github.com/AnalyticalGraphicsInc/cesium/pull/6890).
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention the classes that the functions are removed from.

@janeyx99
Copy link
Contributor Author

janeyx99 commented Aug 14, 2018

How's CHANGES.md now? @lilleyse

@lilleyse
Copy link
Contributor

I tweaked it slightly, but it looks good.

@lilleyse lilleyse merged commit 7e459c8 into CesiumGS:master Aug 14, 2018
@janeyx99 janeyx99 deleted the fix-6874 branch August 14, 2018 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants