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 layers #2808

Merged
merged 6 commits into from Mar 20, 2019

Conversation

Projects
None yet
8 participants
@Pessimistress
Copy link
Contributor

Pessimistress commented Mar 16, 2019

For #2443

H3HexagonLayer (instanced):

h3-hexagon

H3ClusterLayer (renders geojson polygons):

h3-cluster

Change List

  • Add H3HexagonLayer and H3ClusterLayer
  • API documentation
  • Tests
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 16, 2019

Coverage Status

Coverage increased (+0.2%) to 58.982% when pulling d8993c1 on h3-layers into 5aa5714 on master.

@akre54

akre54 approved these changes Mar 18, 2019

Copy link
Member

akre54 left a comment

This looks great and will help us a ton, thanks @Pessimistress!

@Pessimistress Pessimistress force-pushed the h3-layers branch from fcda927 to 7bb036f Mar 18, 2019

renderLayers() {
const {data, getHexagon, updateTriggers} = this.props;

const SubLayerClass = this.getSubLayerClass('hexagon-cell', ColumnLayer);

This comment has been minimized.

Copy link
@heshan0131

heshan0131 Mar 19, 2019

Member

Does ColumnLayer handle calculation of normals based on vertices in the primitive geometry?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

Not yet, it currently assumes that normals are close enough to the regular polygon. We do not render normals correctly anyways (normals are interpolated instead of flat for each side).

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor

normals are interpolated instead of flat for each side

Unless we have some other good ideas, the standard solution is to duplicate vertices for the edges. Since this leads to a bigger geometry, we may want to have that as an option/prop on the ColumnLayer: shading: 'flat' or shading; 'smooth'.

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

The ColumnLayer is currently using CylinderGeometry from luma. I feel this option needs to be added at the source.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 20, 2019

Contributor

That could make sense. Opened luma issue uber/luma.gl#994, so that deck can reference this in a follow-up tracker ticket for the H3 layers.

data: h3.kRing('882830829bfffff', 4),
getHexagon: d => d,
getColor: (d, {index}) => [255, index * 5, 0],
getElevation: (d, {index}) => index * 100

This comment has been minimized.

Copy link
@heshan0131

heshan0131 Mar 19, 2019

Member

is it possible to add another getCoverage accessor?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

Yes? What is the use case, do we need different coverage for each hexagon?

This comment has been minimized.

Copy link
@heshan0131

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

Will do in a follow up PR.

@Pessimistress Pessimistress force-pushed the h3-layers branch from d565af9 to 79e57f4 Mar 19, 2019

@ibgreen
Copy link
Contributor

ibgreen left a comment

Looks quite thorough, nice work!

  • The best validation of the updates would be to update internal apps to use the open source versions.
  • Do we want to give ourself some freedom until then by marking these as experiments (could just be marked after title, no underscore or similar)?

# H3ClusterLayer

The H3ClusterLayer Layer renders regions represented by hexagon sets from the [h3](https://uber.github.io/h3/) geospatial indexing system.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor

Nit: backticks around class names

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor

Perhaps worth auditing the naming with H3 folks. At the time I was working with the library, a cluster wasn't a thing, maybe there is a term that fits better?

@nrabinowitz @isaacbrodsky

An idea is that it would ultimately be nice to offer some level of "parity" between H3 and S2 layers, so a similar cluster layer could be on the roadmap for S2 layers...

Show resolved Hide resolved docs/layers/h3-hexagon-layer.md Outdated

## Remarks

* Each hexagon in the H3 indexing system is [slightly different in shape](https://uber.github.io/h3/#/documentation/core-library/coordinate-systems). To draw a large number of hexagons efficiently, the H3HexagonLayer assumes that all hexagons within the current viewport have the same shape as the one at the center of the current viewport. This strategy is usually sufficient. However, the discrepancy may become visually significant at rare geolocations. In that case, the [H3ClusterLayer] can be used as an alternative by trading performance for accuracy.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor
  • Backticks.
  • In spite of being a composite layer, this only handles one H3 aperture, maybe mention that?
  • Top aperture has mix of hexagons and pentagons, comment that this is not covered?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

It is mentioned in the description of getHexagon.

This comment has been minimized.

Copy link
@dfellis

dfellis Mar 19, 2019

If I can interject, every aperture has a mix of hexagons and pentagons. The pentagons have simply been placed in water so common use-cases do not run into them.

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

This will require some changes in the ColumnLayer, will do in a follow up PR. Do you have a hex id I can test with?

This comment has been minimized.

Copy link
@dfellis

dfellis Mar 20, 2019

Sure. One example pentagon at resolution 9 is 891c0000003ffff.

This comment has been minimized.

Copy link
@dfellis

dfellis Mar 20, 2019

(To be fair, when I said "a mix of hexagons and pentagons" there are always 12 pentagons at each resolution, so the higher the resolution it becomes far, far less likely to run into one.)

You may also want to run a kRing of 10 or 20 around that pentagon, since pentagons are at the epicenter of the hexagon distortions and you're going to run into essentially 5 different hexagon shapes in that area (one for each of the icosahedron faces the hexagons are on, and technically some really weird ones that are split across the icosahedron boundaries).

@@ -4,11 +4,14 @@ import {
MeshLayer,
ScenegraphLayer,
GreatCircleLayer,
S2Layer
S2Layer,

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor

Playing with alignment... S2CellLayer?

renderLayers() {
const {data, getHexagon, updateTriggers} = this.props;

const SubLayerClass = this.getSubLayerClass('hexagon-cell', ColumnLayer);

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor

normals are interpolated instead of flat for each side

Unless we have some other good ideas, the standard solution is to duplicate vertices for the edges. Since this leads to a bigger geometry, we may want to have that as an option/prop on the ColumnLayer: shading: 'flat' or shading; 'smooth'.

@@ -407,6 +407,14 @@ export const docPages = generatePath([
name: 'GridCellLayer',
content: getDocUrl('layers/grid-cell-layer.md')
},
{
name: 'H3HexagonLayer',
content: getDocUrl('layers/h3-hexagon-layer.md')

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 19, 2019

Contributor

Nit: I think we should have a separate index rather than duplicate layers in two places just to get an alphabetic list. An index could be easily auto-generated by ocular once we port the deck website after v7 release. The docs should just contain folders corresponding to the modules. If keeping this.

Show resolved Hide resolved modules/geo-layers/src/h3-layers/h3-hexagon-layer.js Outdated
Show resolved Hide resolved docs/layers/h3-cluster-layer.md Outdated
Show resolved Hide resolved docs/layers/h3-hexagon-layer.md Outdated
Show resolved Hide resolved docs/layers/h3-hexagon-layer.md Outdated
Show resolved Hide resolved docs/layers/h3-hexagon-layer.md Outdated

<p class="badges">
<img src="https://img.shields.io/badge/64--bit-support-blue.svg?style=flat-square" alt="64-bit" />
<img src="https://img.shields.io/badge/extruded-yes-blue.svg?style=flat-square" alt="extruded" />

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Mar 19, 2019

Do we want to add an H3 version badge, similar to what we have on the binding readme: http://github.com/uber/h3-js?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 19, 2019

Author Contributor

I don't expect the JavaScript API to be important here, but the version of the indexing system definitely matters. How stable is the h3-js API? Could we relax the dependency to something like ">=2" so users may pick their version?

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Mar 20, 2019

As long as it's the same major version it should be fine. cc @nrabinowitz

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Mar 20, 2019

Honestly even the major version of the library shouldn't matter, as long as it's the same index version.

if (resolution < 0) {
return;
}
const hex = geoToH3(viewport.latitude, viewport.longitude, resolution);

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Mar 19, 2019

What happens if this is a pentagon?

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Mar 20, 2019

Discussed offline with @Pessimistress - the responsibility is on the user to use this fast version only if they're willing to deal with occasional issues arising from pentagons and crossing icosahedron edges.

@Pessimistress Pessimistress force-pushed the h3-layers branch from 374c8f5 to d8993c1 Mar 19, 2019

@Pessimistress Pessimistress merged commit 7c693e7 into master Mar 20, 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 h3-layers branch Mar 20, 2019

@ibgreen ibgreen referenced this pull request Mar 20, 2019

Closed

Support Flat Shading in Geometries, esp. CylinderGeometry #994

0 of 6 tasks complete

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.