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

Bump deck.gl and fix 3d-tile layer #309

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Bump deck.gl and fix 3d-tile layer #309

merged 1 commit into from
Aug 9, 2019

Conversation

xintongxia
Copy link

No description provided.

@xintongxia xintongxia requested a review from ibgreen August 8, 2019 22:46
@coveralls
Copy link

coveralls commented Aug 8, 2019

Coverage Status

Coverage decreased (-10.3%) to 51.671% when pulling 45b2cbe on xx/upgrade-deck into b1ac84e on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

This is great work, thanks!!!

Recommend landing this and deploying a new website, my comments are P2 and can be addressed in follow-up.

@@ -135,14 +172,14 @@ export default class Tile3DLayer extends CompositeLayer {
return changeFlags.somethingChanged;
}

updateState({props, oldProps}) {
async updateState({props, oldProps}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is OK, but I am pretty sure that deck.gl does not expect its life cycle methods to be async...

Basically, another iteration of this layer could be calling updateState before this function returns.

Worth adding a short comment stating that we are "taking some liberties" here?

@@ -74,13 +71,56 @@ function commonSpacePlanesToWGS84(viewport) {
}
}

function getFrameState(viewport, animationProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Idea: This looks pretty generic. As a second step, we could perhaps move this calculation to Tileset3D. In that case we would want to extact the params from the context first before calling it, as we don't want Tileset3D to know about viewport and animationProps.

@ibgreen ibgreen merged commit bb96efc into master Aug 9, 2019
@ibgreen ibgreen deleted the xx/upgrade-deck branch August 9, 2019 02:55
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.

None yet

3 participants