-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add H3 layers #2808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great and will help us a ton, thanks @Pessimistress!
fcda927
to
7bb036f
Compare
renderLayers() { | ||
const {data, getHexagon, updateTriggers} = this.props; | ||
|
||
const SubLayerClass = this.getSubLayerClass('hexagon-cell', ColumnLayer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ColumnLayer
handle calculation of normals based on vertices in the primitive geometry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ColumnLayer
is currently using CylinderGeometry
from luma. I feel this option needs to be added at the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could make sense. Opened luma issue visgl/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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to add another getCoverage
accessor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes? What is the use case, do we need different coverage for each hexagon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As shown here https://twitter.com/carnby/status/1059530716269154306
It's useful for odt analysis
kepler.gl currently edited shader to enable instanceCoverage
https://github.com/uber/kepler.gl/blob/master/src/deckgl-layers/h3-hexagon-cell-layer/h3-hexagon-cell-layer.js#L26-L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do in a follow up PR.
d565af9
to
79e57f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
docs/layers/h3-cluster-layer.md
Outdated
|
||
# H3ClusterLayer | ||
|
||
The H3ClusterLayer Layer renders regions represented by hexagon sets from the [h3](https://uber.github.io/h3/) geospatial indexing system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: backticks around class names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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...
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mentioned in the description of getHexagon
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. One example pentagon at resolution 9 is 891c0000003ffff
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playing with alignment... S2CellLayer
?
renderLayers() { | ||
const {data, getHexagon, updateTriggers} = this.props; | ||
|
||
const SubLayerClass = this.getSubLayerClass('hexagon-cell', ColumnLayer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
<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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add an H3 version badge, similar to what we have on the binding readme: http://github.com/uber/h3-js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it's the same major version it should be fine. cc @nrabinowitz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this is a pentagon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
374c8f5
to
d8993c1
Compare
For #2443
H3HexagonLayer (instanced):
H3ClusterLayer (renders geojson polygons):
Change List