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

GPU Aggregation (7/8): ContourLayer #9099

Merged
merged 3 commits into from
Aug 23, 2024
Merged

GPU Aggregation (7/8): ContourLayer #9099

merged 3 commits into from
Aug 23, 2024

Conversation

Pessimistress
Copy link
Collaborator

For #7457

Some golden images are updated because the default grid origin is moved from screen bottom-left to common space 0,0.

Change List

  • Move ContourLayer to use new AggregationLayer and aggregator classes
  • Unit tests
  • Restore render tests

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Mostly a rubberstamp as diff is fairly big.


if (aggregator instanceof WebGLAggregator) {
const buffer = aggregator.getResult(channel)?.buffer;
if (buffer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Is there really no way of adding a getValueReader() type method to the aggregator interface so that we don't need this abstraction-breaking instanceof fiddling in the layers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm on the fence about this. This functionality is only really needed by this layer.

@coveralls
Copy link

Coverage Status

coverage: 89.293% (+0.4%) from 88.902%
when pulling 9829760 on x/aggregation-7
into 42bf8b5 on master.

@Pessimistress Pessimistress merged commit 9da9072 into master Aug 23, 2024
4 checks passed
@Pessimistress Pessimistress deleted the x/aggregation-7 branch August 23, 2024 23:27
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.

4 participants