Integrate all aggregation layers with AttributeManager by 1chandu · Pull Request #3777 · visgl/deck.gl

1chandu · 2019-10-12T00:40:47Z

For #3583

Background

Integrate all aggregation layers (except HexagonLayer and CPUGridLayer) with AttributeManager so that :

Custom data parsing is removed and attributes are consumed from AttributeManager
Enables GPU data-filter extension.

At a high level these changes include

When extensions change (could potentially change shader modules, injections, etc ..) re create aggregation models.
Update attributes during updateState, so all aggregation layers can re-run aggregation with latest attributes during update cycle. (regular Composite layers don't do this)
Generate module settings for aggregation model, which usually happens only for non Composite layers only.
Re aggregate when data-filter props change.
Update all affected unit tests.

Remaining items:

Integrate AttributeManager for CPU aggregation, which includes HexagonLayer, CPUGridLayer and CPU Aggregation path of ScreenGridLayer.
Enable data-filter extension for above CPU aggregation paths.
Enable Fp64 extension for both CPU and GPU Aggregation paths.

Change List

Aggregation-layers: GPUAggregation: Integrate with AttiributeManager

coveralls · 2019-10-18T00:15:12Z

Coverage decreased (-2.6%) to 80.337% when pulling 35163e3 on AggregationLayer into 7f49cc9 on master.

1chandu · 2019-10-31T00:23:35Z

+  'filterTransformColor'
+];
+
+export default class AggregationLayer extends CompositeLayer {


Not using Layer as base class, doing so I have to copy _renderLayers method.

1chandu · 2019-11-01T17:34:39Z

-      dataChanged = true;
-      // Clear countsData cache
+    const dataChanged = this._isAggregationDirty(opts);
+    const cellSizeChanged = opts.oldProps.cellSize !== opts.props.cellSize;


Aggregator needs dataChanged and cellSIzeChanged flags seperately, so in CPU aggregation path, projection is not performed when only cell size is changed, cellSize can be added to aggregationProps, once CPU Aggregation is moved out of aggregator.

Pessimistress · 2019-11-04T18:34:39Z

+const {compareProps} = experimental;
+
+// props when changed results in new uniforms that requires re-aggregation
+const UNIFORM_PROPS = [


Layers should not be aware of the implementation detail of extensions. This is not going to scale when we support other extensions.

Yeah, thought about it, ideal solution for this would be the extension itself expose these props and our existing prop diffing, differentiate these prop changes with existing changeFlags.propsChanged, so that aggregation is only run when these props are changed (not when any prop changed).
We don't need this for non composite layers, because we render the layers any time a prop is changed, but that is un-necessary and expensive for aggregation layers. I can create a task for this.

Maybe you can add a LayerExtension.needsRedraw() method for this purpose?

Will do this in separate PR #3844

Pessimistress · 2019-11-04T18:41:51Z


    // viewport is needed only when performing screen space aggregation (projectPoints is true)
-    log.assert(!(changeFlags.viewportChanged && projectPoints) || opts.viewport);
+    log.assert(!(changeFlags.viewportChanged && projectPoints) || opts.moduleSettings.viewport);


Do not use assert unless checking for user error. GPUGridAggregator is considered an internal component.

validateProps props method is validating various parameter combinations, once CPU aggregation is moved out and other refactors done, I can remove it altogether, till then this is handy to catch errors. For now commenting out.

Pessimistress · 2019-11-04T18:43:31Z

+
+    const shaders = mergeShaders(
+      {
+        vs: fp64 ? AGGREGATE_TO_GRID_VS_FP64 : AGGREGATE_TO_GRID_VS,


Drop legacy fp64 support

I had issues enabling fp64 extension, I have todo item in the issue.
Aggregation only needs fp64 module not the project64, will cleanup this code in a seperate PR.

Pessimistress · 2019-11-04T18:47:08Z

    }
  }

+  setupAggregationModel(fp64 = false) {


Why create a (public) method for a function that is only 3 lines? Same for getAggregationModel and getAllAggregationModel - these negatively impact the bundle size. Either merge them or make them utility functions outside of this class.

Took pass on class methods, moved several into helper methods, will make more deeper look in future aggregator specific PRs.

Pessimistress · 2019-11-04T18:48:42Z

  }

-  _setModelAttributes(model, changedAttributes) {
+  _getShaderAttributes(attributes, excludeAttributes = {}) {


Move to AttributeManager

* Aggregation-layers: GPUAggregation: Integrate with AttributeManager * Clear HeatmapLayer debounce timer

1chandu requested a review from Pessimistress October 12, 2019 00:41

1chandu force-pushed the AggregationLayer branch from 8575a6b to 9c4390c Compare October 30, 2019 20:58

1chandu changed the title ~~[WIP] Integrate all aggregation layers with AttributeManager~~ Integrate all aggregation layers with AttributeManager Oct 30, 2019

1chandu force-pushed the AggregationLayer branch 2 times, most recently from e8878cd to ecb418d Compare October 30, 2019 23:09