Category filtering in DataFilterExtension #7915
Conversation
|
This looks really nice! What does this need to get over the line? |
| * Accessor to retrieve the category for each object that it will be filtered by. | ||
| * Returns either a number (if `filterSize: 1`) or an array of numbers. | ||
| */ | ||
| getFilterCategory?: Accessor<DataT, number | number[]>; |
There was a problem hiding this comment.
Would this work with a typed array passed into data.attributes.getFilterCategory as well?
There was a problem hiding this comment.
Not right now, as the attribute applies a transform: https://github.com/visgl/deck.gl/pull/7915/files#diff-83f57f71121ca1f8ed500ace34e39c597192576aea11e44b4efe51431a86c9c7R184-R187
There was a problem hiding this comment.
Great work! I just scanned the code so treat my comments as light nits and suggestions, I could easily have missed some points.
I think the interface may be a little inconvenient as filters are provided as an array of arrays rather than a Record<columnName, value[]>.
Not sure how much work it would be to support that, or if it makes sense.
| @@ -153,6 +154,61 @@ Format: | |||
| * If `filterSize` is `1`: `[softMin, softMax]` | |||
| * If `filterSize` is `2` to `4`: `[[softMin0, softMax0], [softMin1, softMax1], ...]` for each filtered property, respectively. | |||
|
|
|||
| ##### `getFilterCategory` ([Function](../../developer-guide/using-layers.md#accessors), optional) {#getfiltercategory} | |||
There was a problem hiding this comment.
Nit: This name didn't click immediately for me. I would probably have gone with getDataFilterCategory (especially as we now have multiple "filter" extensions) but I guess the naming convention is somewhat fixed by now.
There was a problem hiding this comment.
I don't love it either, I was trying to stay "consistent" with getFilterValue. I think we should stick with this or make both accessors consistent, e.g. use getDataFilterValue & getDataFilterCategory. This would be a breaking change, and I'm not sure it is worth it
| @@ -71,6 +71,7 @@ new DataFilterExtension({filterSize, fp64}); | |||
| ``` | |||
|
|
|||
| * `filterSize` (Number) - the size of the filter (number of columns to filter by). The data filter can show/hide data based on 1-4 numeric properties of each object. Default `1`. | |||
| * `categorySize` (Number) - the size of the category filter (number of columns to filter by). The category filter can show/hide data based on 1-4 properties of each object. Default `1`. | |||
There was a problem hiding this comment.
Can this be auto-detected?
There was a problem hiding this comment.
Perhaps in a followup PR. But in that case filterSize should also support it. Part of me thinks it is good to have it be explicit as then the user intention is clear. For auto detection, should we use filterCategories or the getFilterCategory accessor? What if they don't match?
| Format: | ||
|
|
||
| * If `categorySize` is `1`: `['category1', 'category2']` | ||
| * If `categorySize` is `2` to `4`: `[['category1', 'category2', ...], ['category3', ...], ...]` for each filtered property, respectively. |
There was a problem hiding this comment.
Could we avoid the overloading and always required nested array, and just use firm typescript typing?
There was a problem hiding this comment.
Could it be a map with column names?
There was a problem hiding this comment.
Again, I prioritized consistency with existing API here
For #7827
Background
Adds category filtering to
DataFilterExtensionScreen.Recording.2023-05-25.at.14.11.00.mov
Change List
getFilterCategoryandfilterCategoriesprop inDataFilterExtensioncategorySizeinDataFilterExtensionto allow 1-4D filtersDataFilterExtensionto support passing discrete valuesDataFilterExtensionshader module to perform GPU filtering