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

QuadkeyLayer implementation & tests #6678

Merged
merged 18 commits into from
Feb 23, 2022
Merged

QuadkeyLayer implementation & tests #6678

merged 18 commits into from
Feb 23, 2022

Conversation

felixpalmer
Copy link
Collaborator

@felixpalmer felixpalmer commented Feb 22, 2022

For #6677

Change List

  • Introduction of private abstract GeoCellLayer to share common code between H3ClusterLayer & S2Layer
  • Addition of QuadkeyLayer (also based on GeoCellLayer)
  • Unit tests
  • Documentation including live example
  • QuadkeyLayer added to layer-browser

const [topLeft, bottomRight] = quadkeyToWorldBounds(quadkey);
const [w, n] = worldToLngLat(topLeft);
const [e, s] = worldToLngLat(bottomRight);
return new Float64Array([e, n, e, s, w, s, w, n, e, n]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not necessary to convert the coordinates to a typed array. It only adds performance overheads because of extra GC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Is there a reason why a Float64Array is created in s2-utils.js?

https://github.com/visgl/deck.gl/blob/master/modules/geo-layers/src/s2-layer/s2-utils.js#L42

@coveralls
Copy link

coveralls commented Feb 22, 2022

Coverage Status

Coverage increased (+0.006%) to 82.64% when pulling e809d89 on felix/quadkey-layer into 6c3b089 on master.

@felixpalmer
Copy link
Collaborator Author

I've added the following example to the layer-browser and the documentation

Screenshot 2022-02-23 at 12 24 00

The test data is synthetic and needs to be uploaded to deck.gl-data: https://github.com/visgl/deck.gl/pull/6678/files#diff-202bd1a464188d5c7bfadc858bca0e579de2527f08217648e28aba7c31bef569

Copy link
Collaborator

@Pessimistress Pessimistress left a comment

Choose a reason for hiding this comment

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

The PR is already pretty large and I think it's ok to merge as is.

Follow up, the GeoCellLayer needs to handle update trigger changes of the index accessor, and ideally avoid passing the expensive index to geo bounds conversion to getPolygon - it will be called twice for each object in the tessellator. This is an existing perf issue in some of the current layers and not introduced by this PR.

docs/api-reference/geo-layers/quadkey-layer.md Outdated Show resolved Hide resolved
modules/geo-layers/src/geo-cell-layer/GeoCellLayer.js Outdated Show resolved Hide resolved
modules/geo-layers/src/geo-cell-layer/GeoCellLayer.js Outdated Show resolved Hide resolved
modules/geo-layers/src/h3-layers/h3-cluster-layer.js Outdated Show resolved Hide resolved
modules/geo-layers/src/quadkey-layer/quadkey-layer.js Outdated Show resolved Hide resolved
test/modules/geo-layers/quadkey-layer.spec.js Outdated Show resolved Hide resolved
@felixpalmer felixpalmer merged commit 1f9ecaf into master Feb 23, 2022
@felixpalmer felixpalmer deleted the felix/quadkey-layer branch February 23, 2022 20:32
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