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

Add H3 layer #198

Merged
merged 7 commits into from Aug 27, 2018
Merged

Add H3 layer #198

merged 7 commits into from Aug 27, 2018

Conversation

ericsoco
Copy link
Collaborator

@ericsoco ericsoco commented Aug 14, 2018

Still planning to add a coverage slider to match existing Hexbin Layer.
Also, I think @nrabinowitz has some additional edits.

This addresses #154, but does not add support for H3 bin aggregation (that can come in a followup diff, extending our use of h3-js.

screen shot 2018-08-14 at 11 20 27 am

@ericsoco ericsoco changed the title Add H3 layer (WIP) Add H3 layer Aug 14, 2018
@ericsoco
Copy link
Collaborator Author

ericsoco commented Aug 14, 2018

Still TODO:

  • Coverage slider
  • @nrabinowitz additional h3-utils refinements
  • Correctly populate + render "Layer Type" UI in H3 layer panel
  • Figure out why CI is failing (tests pass locally...)
  • H3 bin aggregation: probably want to implement as switch and conditional slider in Hexbin: "use H3 hexes" and, when that's flipped on, "H3 resolution" (instead of "Hexagon Radius (km)"). Most likely will save this for a future PR.
  • Correctly serialize H3 settings, for config export
  • Don't auto-add point layer when H3 can be auto-detected

@heshan0131
Copy link
Contributor

heshan0131 commented Aug 15, 2018

This is awesome. Thank you for doing this. Did you move the files from @uber/kepler.gl?

@@ -3506,6 +3506,10 @@ grid-index@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/grid-index/-/grid-index-1.0.0.tgz#ad2c5d54ce5b35437faff1d70a9aeb3d1d261110"

h3-js@^3.1.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update your ~/.npmrc to use registry = https://registry.yarnpkg.com/ before running yarn. This is why the build failed

@ericsoco
Copy link
Collaborator Author

@heshan0131 yep, copied from internal kepler wrapper. Will try to find time this week to wrap it up, TBD.

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.

Nice work! Is this generic enough that we should we consider moving it to deck.gl?

@ericsoco
Copy link
Collaborator Author

@ibgreen perhaps? Thoughts on where in deck.gl this would be added, experimental-layers to start I assume? (Been a while since I contributed.)

Would probably want to standardize enhanced-hexagon-cell-layer across this PR and the existing one in Kepler, and may need some other cleanup.

I'm guessing the best path here is to land this into Kepler first and then work w/ @heshan0131 and @nrabinowitz to tidy things up enough to hoist them into deck.gl. Thoughts?

@ibgreen
Copy link
Collaborator

ibgreen commented Aug 20, 2018

@ericsoco - All sounds reasonable.

Also see the ideas in the deck.gl layers roadmap where we start discussing having multiple layer catalogs.

Putting things in experimental layers is a nice first start, since more users can access the layers.

The big job is typically getting layers out of experimental layers and into the official catalog(s).

Copy link
Collaborator

@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

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

LGTM, just a few perf and cleanup notes.

hexagonResolution: this.dataToFeature.resolution
};

// return {layerData, layer: this};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kill comment please

getElevation,
getColor,
getHexId,
hexagonVertices: this.dataToFeature.vertices,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still needed? We calculate in render now, right?


allData.forEach((d, index) => {
const id = getHexId(d);
if (typeof id !== 'string' || !id.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider h3IsValid here, which checks whether it's a real H3 address. This would be slower but more reliable.

}

renderLayer({
data,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be a Kepler.gl style question, but I find the way that data refers to different objects in different methods a bit confusing.

// get vertices should return [lon, lat]
export function getVertices({id}) {
// always reverse it
return h3ToGeoBoundary(id).map(d => d.reverse());
Copy link
Collaborator

Choose a reason for hiding this comment

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

h3ToGeoBoundary(id, true) will give you GeoJSON [lon, lat] coords in a more performant way than .reverse(). It will also close the loop (repeat the first point), which is required for valid GeoJSON - not sure if that matters here.


return {
geometry: {
coordinates: [...vertices, vertices[0]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for vertices[0] here if you use the h3ToGeoBoundary(id, true) suggestion above.

const vertices = getVertices(object);

return {
geometry: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

May not matter here, but this isn't a valid GeoJSON object w/o "type": "Feature" at the top level.

}

// get centroid should return [lon, lat]
export function getCentroid({id}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably wouldn't matter if it wasn't in a hot loop, but you might be doing this for 100k hexagons, so I'd suggest just passing the hex id w/o needless destructing.

@heshan0131
Copy link
Contributor

heshan0131 commented Aug 27, 2018

@ericsoco I can take over this PR. Please grant me access to your fork. You can allow edits from maintainers

@heshan0131 heshan0131 merged commit a8ea76f into keplergl:master Aug 27, 2018
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

4 participants