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

Consolidate cell layers #2796

Merged
merged 5 commits into from Mar 16, 2019

Conversation

Projects
None yet
4 participants
@Pessimistress
Copy link
Contributor

Pessimistress commented Mar 14, 2019

For #2736

Change List

  • HexagonCellLayer is removed; add generic ColumnLayer (see layer API doc)
  • GridCellLayer now extends ColumnLayer
  • The old cell layers were not projecting meter elevations correctly. This is a breaking change for GridCellLayer, GridLayer and HexagonLayer (see updated golden images)
  • Documentation
  • Update layer browser example and unit tests
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 14, 2019

Coverage Status

Coverage decreased (-0.2%) to 58.248% when pulling f725443 on cell-layers into fc2beea on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 14, 2019

Coverage Status

Coverage decreased (-0.2%) to 58.295% when pulling 14226bd on cell-layers into cde9e78 on master.

@heshan0131

This comment has been minimized.

Copy link
Member

heshan0131 commented Mar 14, 2019

This change will break h3 hexagon layer in kepler.gl and the fix wouldn't be easy. H3 hexagon is not a perfect hexagon, the vertices are slightly distorted based on which part of the world it is in. Previously when we can pass hexagonVertices to hexagon cell, we can actually pass in h3 hex vertices to achieve good rendering result.

https://twitter.com/heshan_cheri/status/1033929329976037376

@heshan0131

This comment has been minimized.

Copy link
Member

heshan0131 commented Mar 14, 2019

That being said, I like the idea of consolidating hexagon-cell and grid-cell layer into a column layer. Instead of rescrit the geometry to be a disk with x number of sides can we also allow pass in diskVertices? And if diskVertices is passed, we don't need to calculate radius / angle

@Pessimistress

This comment has been minimized.

Copy link
Contributor Author

Pessimistress commented Mar 14, 2019

@heshan0131 you can always calculate radius and angle in your own H3 Hexagon layer before passing them to the sub layer.

With that said, we do want to implement a proper H3Layer as part of the deck.gl layer catalog.

@heshan0131

This comment has been minimized.

Copy link
Member

heshan0131 commented Mar 15, 2019

@Pessimistress Since H3 is not actually a perfect hexagon shape, we need to pass in all 6 vertices to render it. Angle and radius won't do

@Pessimistress

This comment has been minimized.

Copy link
Contributor Author

Pessimistress commented Mar 15, 2019

@heshan0131 That is not how I read the current HexagonCellLayer. hexagonVertices is only used to calculate angle and radius. I think kepler is using a custom version?

@Pessimistress

This comment has been minimized.

Copy link
Contributor Author

Pessimistress commented Mar 15, 2019

@heshan0131 we can add a prop to support custom vertices, but they need to be in meter offsets instead of lng/lat like the old API did. Core layers must support all coordinate systems and views, and the old treatment was specialized to only geospatial data in LNGLAT mode.

@heshan0131

This comment has been minimized.

Copy link
Member

heshan0131 commented Mar 15, 2019

Sounds good, we can work with that

@Pessimistress Pessimistress force-pushed the cell-layers branch from e2e029d to f725443 Mar 16, 2019

@Pessimistress Pessimistress merged commit 963c117 into master Mar 16, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@Pessimistress Pessimistress deleted the cell-layers branch Mar 16, 2019

ajduberstein added a commit to ajduberstein/deck.gl that referenced this pull request Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.