[Bug] Fix hexagon layer projection #4173

heshan0131 · 2020-01-20T09:36:26Z

use center of data to calculate radius in common space. for projection
save radiusCommon in aggregation result
recalculate meter radius based on saved radiusCommon when viewport updated

Pessimistress · 2020-01-20T18:41:11Z

modules/aggregation-layers/src/hexagon-layer/hexagon-aggregator.js

+    [Infinity, -Infinity] // pt[1]
+  ];
+
+  for (let i = 0; i < size * data.length; i += size) {


This will recalculate every iteration. Assign to a variable.

Pessimistress · 2020-01-20T18:41:57Z

modules/aggregation-layers/src/hexagon-layer/hexagon-aggregator.js

+  ];
+
+  for (let i = 0; i < size * data.length; i += size) {
+    const arrayIsFinite = Number.isFinite(positions[i]) && Number.isFinite(positions[i + 1]);


Create variables x & y

Pessimistress · 2020-01-20T18:43:29Z

modules/aggregation-layers/src/hexagon-layer/hexagon-aggregator.js

+  const positions = attributes.positions.value;
+  const {size} = attributes.positions.getAccessor();
+
+  const bounds = [


Perf: I’d just use four variables minX, minY, etc.

Pessimistress · 2020-01-20T18:50:50Z

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js

    let angle = 90;
+    let radius;
+    const {viewport} = this.context;
+    const {unitsPerMeter} = viewport.getDistanceScales();

    if (Array.isArray(vertices)) {


What is the branch used for? Kepler’s h3 aggregation?

You shouldn’t have to reproject the vertices every time the viewport changes. Radius in common units is a constant.

it WAS used when kepler.gl has its own aggregator and aggregate points to h3 hexagons. However, we don't need to use this anymore, since we are not aggregate points into h3. For future development, when we want to create a h3 aggregation layer, we can revisit this

A H3 aggregation layer should use the H3HexagonLayer. If this is no longer used by kepler we should mark it as deprecated.

Pessimistress · 2020-01-20T20:42:52Z

You need to rebase master and update the render test golden images.

coveralls · 2020-01-21T19:11:28Z

Coverage decreased (-2.6%) to 80.877% when pulling 90199dc on fix-hexagon-layer-projection into ff55172 on master.

Pessimistress

Why would the colors change in the golden images? I don't think color values are affected by this change.

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js

heshan0131 · 2020-01-22T08:38:05Z

Why would the colors change in the golden images? I don't think color values are affected by this change.

Projection shifted, changed number of points in bin, which changes color scale

Pessimistress · 2020-01-23T02:19:27Z

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js

-      const radius = dxy / 2 / unitsPerMeter[0];
+      // offset all points by centroid to meter offset
+      const vertices = hexagonVertices.map(vt => [
+        (vt[0] - centroid[0]) * unitsPerMeter,


unitsPerMeter is an array

LngLat coordinates must first be projected to the common space.

Pessimistress · 2020-01-23T23:36:57Z

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js

-      this.updateRadiusAngle(hexagonVertices);
+    // if user provided custom aggregator and returns hexagonVertices,
+    // Need to recalculate radius and angle based on vertices
+    const {hexagonVertices} = cpuAggregator.state.layerData || {};


Suggested change

const {hexagonVertices} = cpuAggregator.state.layerData || {};

const {hexagonVertices, radiusCommon} = cpuAggregator.state.layerData || {};

Pessimistress · 2020-01-23T23:37:33Z

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js

+      }
+    } else {
+      // update radius angle by viewport
+      this.updateRadiusAngle();


Suggested change

this.updateRadiusAngle();

this.updateRadiusAngle(radiusCommon);

Pessimistress · 2020-01-23T23:42:13Z

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js

  }

  updateState(opts) {
    super.updateState(opts);
-    const {cpuAggregator} = this.state;
-    const oldLayerData = cpuAggregator.state.layerData;
+    const {cpuAggregator, hexagonVertices: oldVertices} = this.state;
    this.setState({


Perf:

if (opts.changeFlags.propsOrDataChanged) { // update aggregation }

* use center of data to calculate radius in common space. for projection * save radiusCommon in aggregation result * recalculate meter radius based on saved radiusCommon when viewport updated * if hexagonVertices is provided, convert them to meter offset

* master: (82 commits) fix typo in performance.md pydeck: ArcLayer, BitmapLayer, ColumnLayer examples (#4189) [React] fix missing key error (#4193) [Bug] Fix hexagon layer projection (#4173) Remove HtmlWebpackPlugin from examples/playground (#4178) @deck.gl/json: Fix bug dropping props with falsy values (#4185) Fix buffer size check in Attribute.updateBuffer (#4190) Bump luma dependency (#4191) data-filter: support double precision (#4163) Use int type for enum uniforms (#4171) [TileLayer] fix tile indices generation in edge cases (#4170) v8.1.0-alpha.1 Voodoo fix for Mac+NVIDIA bug (#4166) Remove unnecessary code from project glsl (#4162) Fix H3HexagonLayer update when viewport jumps (#4158) Refactor render tests; use stricter pass criteria (#4157) [Extension] Add source_target to brushing mode (#4150) Add offset feature to PathStyleExtension (#4126) Project module: support pre-projected positions (#4140) Repeat maps at low zoom levels (#4105) ...

* use center of data to calculate radius in common space. for projection * save radiusCommon in aggregation result * recalculate meter radius based on saved radiusCommon when viewport updated * if hexagonVertices is provided, convert them to meter offset

heshan0131 requested a review from Pessimistress January 20, 2020 09:36

Pessimistress reviewed Jan 20, 2020

View reviewed changes

heshan0131 added 5 commits January 20, 2020 13:13

use viewport at data center to calculate radius

7243693

fix hexagon layer projection

c771ee0

lint

a660e4f

disable debugger

5bcd585

address feedback

4a15726

heshan0131 force-pushed the fix-hexagon-layer-projection branch from 3666fe6 to 4a15726 Compare January 20, 2020 22:25

update golden image

0c81a54

Pessimistress reviewed Jan 21, 2020

View reviewed changes

modules/aggregation-layers/src/hexagon-layer/hexagon-layer.js Show resolved Hide resolved

convert hexagonVertices to meter offset

0fb480e

Pessimistress reviewed Jan 23, 2020

View reviewed changes

heshan0131 added 4 commits January 23, 2020 14:56

correctly calculate hexagon vertices to meter offset

2db4f9b

better error log

630b4bc

prettier

0ae5c77

check vertice length

0fc8edc

Pessimistress approved these changes Jan 23, 2020

View reviewed changes

Pessimistress reviewed Jan 23, 2020

View reviewed changes

perf improve

90199dc

heshan0131 merged commit b513df6 into master Jan 24, 2020

Pessimistress deleted the fix-hexagon-layer-projection branch February 13, 2020 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Fix hexagon layer projection #4173

[Bug] Fix hexagon layer projection #4173

heshan0131 commented Jan 20, 2020 •

edited

Loading

Pessimistress Jan 20, 2020

Pessimistress Jan 20, 2020

Pessimistress Jan 20, 2020

Pessimistress Jan 20, 2020

heshan0131 Jan 21, 2020

Pessimistress Jan 21, 2020

Pessimistress commented Jan 20, 2020

coveralls commented Jan 21, 2020 •

edited

Loading

Pessimistress left a comment

heshan0131 commented Jan 22, 2020

Pessimistress Jan 23, 2020

Pessimistress Jan 23, 2020

Pessimistress Jan 23, 2020

Pessimistress Jan 23, 2020

Pessimistress Jan 23, 2020

	const {hexagonVertices} = cpuAggregator.state.layerData \|\| {};
	const {hexagonVertices, radiusCommon} = cpuAggregator.state.layerData \|\| {};

	this.updateRadiusAngle();
	this.updateRadiusAngle(radiusCommon);

[Bug] Fix hexagon layer projection #4173

[Bug] Fix hexagon layer projection #4173

Conversation

heshan0131 commented Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pessimistress commented Jan 20, 2020

coveralls commented Jan 21, 2020 • edited Loading

Pessimistress left a comment

Choose a reason for hiding this comment

heshan0131 commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heshan0131 commented Jan 20, 2020 •

edited

Loading

coveralls commented Jan 21, 2020 •

edited

Loading