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

Only add necessary attributes in DataFilterExtension #8769

Merged
merged 4 commits into from Apr 9, 2024

Conversation

felixpalmer
Copy link
Collaborator

Closes #8694

Change List

  • Allow 0 as value for filterSize & categorySize
  • Only add attribute to shader for non-zero sizes
  • Default categorySize to 0 to fix breaking change in v9
  • Test & doc update

@coveralls
Copy link

coveralls commented Apr 8, 2024

Coverage Status

coverage: 89.996% (+0.008%) from 89.988%
when pulling 9a34aec on felix/data-filter-opts
into f50bec4 on master.

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.

Just thoughts around the theme that less conditional code could be a goal worth considering.

uniform DATAFILTER_TYPE filter_max64High;
#ifdef DATACATEGORY_TYPE
#ifdef NON_INSTANCED_MODEL
#define DATACATEGORY_ATTRIB filterCategoryValues
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this, I ask myself if we really want to require an attribute that can be applied in both instanced and non-instanced usage should be renamed. This simple renaming adds a lot of clutter to this shader.

@Pessimistress
Copy link
Collaborator

Can you add a few render tests (data filter, category filter & both)?

@felixpalmer felixpalmer merged commit bddd4cc into master Apr 9, 2024
2 checks passed
@Pessimistress Pessimistress deleted the felix/data-filter-opts branch April 10, 2024 18:48
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.

[Bug] deckgl 9.0.1 did not render PathLayer and others
4 participants