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

[PART-4] CPUAggregation Refactor : Add CPU Aggregation support to GridAggregationLayer #3954

Merged
merged 22 commits into from
Dec 7, 2019

Conversation

1chandu
Copy link
Contributor

@1chandu 1chandu commented Dec 3, 2019

For #3583

NOTE: Splitting #3912 to multiple stacked PRs, will close #3912 (keeping it open for existing discussions to be resolved).

Background

Change List

  • GridAggregationLayer:
    • Add support for CPU Aggregation (using same utility methods used for CPUGridLayer).
    • Add required state management to handle various aggregation scenarios (CPU vs GPU, WorldSpace vs ScreenSpace, single vs multiple weight aggregation)
  • Update all grid aggregation layers, ContourLayer, ScreenGridLayer, and GPUGridLayer to extend form GridAggregationLayer. [NOTE: CPUGridLayer doesn't use GridAggregationLayer, which requires more sophisticated dimension management, same as `HexagonLayer]
  • Add docs and unit tests.

docs/layers/contour-layer.md Outdated Show resolved Hide resolved
modules/aggregation-layers/src/aggregation-layer.md Outdated Show resolved Hide resolved
@@ -47,8 +51,17 @@ void main(void) {

vec2 pos = project_to_pixel(windowPos);

vec2 pixelXY64[2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you are not handling positions64Low for ScreenGridLayer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pixelXY64 defined here only to perform fp64 arithmetic for division operation, applies to both world space aggregation and screen space, but I am not actually using low fp64 positions attributes, doesn't seem to be needed, we can add if there is a need.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The projection to clipspace is inaccurate if you don't use the low parts. This applies to both geospatial and non-geospatial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm only making the comment here because GitHub doesn't allow me to comment on a line you didn't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated attributes to be 64bit and added attribute to the shader.

contourSegments.length > 0 &&
new LinesSubLayerClass(
this.getSubLayerProps({
id: 'contour-line-layer'
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Just use sub layer ids "lines" and "bands". They will be concatenated with the parent layer's id so you don't have to call out that they are children of a contour layer.
  • The sub layer ids should be documented in contour-layer.md

}
});
}
// Private (Sublayers)

_onGetSublayerColor(element) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is extremely expensive (creating inline functions inside an iteration).

You should be able to do something like contours[object.contourIndex].color.

Same thing for _onGetSublayerStrokeWidth

@1chandu 1chandu merged commit 1d03510 into CPUAggregationRefactor3 Dec 7, 2019
@1chandu 1chandu deleted the CPUAggregationRefactor4 branch December 7, 2019 00:54
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

2 participants