Page MenuHome
Differential D10959

Spreadsheet Editor: Row filters
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Apr 12 2021, 10:12 PM.

Details

Reviewers
Jacques Lucke (JacquesLucke)
Dalai Felinto (dfelinto)
Simon Thommes (simonthommes)
Maniphest Tasks
T86133: Rules based row filtering for attribute spreadsheet
Commits
rBf9aea19d9890: Spreadsheet Editor: Row Filters
Summary

This patch adds support for filtering out rows based on rules and values.
Filters will work for any attribute data source, they are a property of
the spreadsheet rather than of the attribute system. The properties
displayed in the row filter can depend on data type of the currently
visible column with that name. If the name is no longer visible, the row
filter will remember its data type until a column with that name is
visible again.

Implementation Questions

  • The comments in screen.c combined with tagging the sidebar for redraw after the main region point to a lack of understanding or some technical debt, not sure.

Future Improvements

  • This patch doesn't contain a search of visible columns when adding a new filter. That's separate enough that I think it makes sense to push that with a separate patch later.

This patch is available in the temp-spreadsheet-row-filter branch.

Diff Detail

Repository
rB Blender
Branch
temp-spreadsheet-row-filter
Build Status
Buildable 14019
Build 14019: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Simon Thommes (simonthommes) added a comment.Apr 13 2021, 12:43 PM

Very nice!
I think the design of the filters themselves works super well.

Couple of remarks:

  • I do like the idea of the popover, as I'm also not a huge fan of adding a sidebar for this. But I can definitely see Jacques' problem with it.
  • Changing the domain results in attributes not being found and data-types reverting to float in cases where it doesn't make sense (you also mentioned this in your further questions). I think this brings up a design question that we haven't considered in general. I can see two different approaches working:
    • We have separate filters per domain. They can't share names throughout domains anyways.
    • A filter can search through attributes of all domains and the row mask propagates based on topology the same way as it does when changing select modes in the edit mode. (E.g. filter that returns a single face propagates to its corner vertices)
  • We still have the same issue with attributes that are missing when the context changes. Would it be possible to remember the type of a known attribute and not revert to default, when it's not found anymore?
  • In line with that, it would also be great if, in the future, we could add a viewport overlay to highlight the data of the shown rows. Same thing for making rows selectable and highlighting the selected ones. (Way out of scope, I know. It's just something this made me think about and I'm not sure we talked about something like this before.)
Hadrien Brissaud (hadrien) added a subscriber: Hadrien Brissaud (hadrien).Apr 13 2021, 9:35 PM

Just a thought about UI : I think it makes sense to have filter options both inside the popover and in a sidebar, akin to tool/brush options in the 3D view that are both accessible through the header as popovers and in the sidebar (so they're always visible).

Hans Goudey (HooglyBoogly) updated this revision to Diff 36153.Apr 14 2021, 4:33 AM
Hans Goudey (HooglyBoogly) marked 6 inline comments as done.
  • Address small review comments from Jacques
  • New implementation of the UI in a sidebar
Harbormaster completed remote builds in B14051: Diff 36153.Apr 14 2021, 4:33 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 14 2021, 4:35 AM
Hans Goudey (HooglyBoogly) added inline comments.Apr 14 2021, 4:41 AM
source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc
179

Oops, I didn't mean to do that, thanks.

source/blender/editors/space_spreadsheet/spreadsheet_ops.cc
37

Sure, works for me.

40

I usually try to do that too, I thought each section here was doing slightly different types of operations though. I'll see if I can remove some new lines, since I mostly agree.

Hans Goudey (HooglyBoogly) updated this revision to Diff 36154.Apr 14 2021, 4:41 AM
  • Cleanup: Whitespace and unused includes
Harbormaster completed remote builds in B14052: Diff 36154.Apr 14 2021, 4:41 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 14 2021, 7:51 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 36172.Apr 14 2021, 7:51 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Use nullptr instead of NULL
  • Compare display names instead of the column ID
Harbormaster completed remote builds in B14061: Diff 36172.Apr 14 2021, 7:51 PM
Hans Goudey (HooglyBoogly) added a task: T86133: Rules based row filtering for attribute spreadsheet.Apr 14 2021, 9:29 PM
Dalai Felinto (dfelinto) added a comment.Apr 15 2021, 11:17 AM

@Hans Goudey (HooglyBoogly) can you rebase this with latest master? I'm running into some bugs when trying "only selected" + nodes which I'm not sure are from the patch or not.

David (Galagax) added a subscriber: David (Galagax).Apr 16 2021, 3:56 AM

My proposal on the UI: mantain it as a popover but add the pin option to stick it on place to not hide it if you click outside

Hans Goudey (HooglyBoogly) updated this revision to Diff 36230.Apr 16 2021, 7:07 PM
  • Merge branch 'master' into temp-spreadsheet-row-filter
Harbormaster completed remote builds in B14080: Diff 36230.Apr 16 2021, 7:07 PM
Hans Goudey (HooglyBoogly) added a comment.Apr 16 2021, 7:09 PM
In D10959#280238, @Dalai Felinto (dfelinto) wrote:

@Hans Goudey (HooglyBoogly) can you rebase this with latest master? I'm running into some bugs when trying "only selected" + nodes which I'm not sure are from the patch or not.

Done. Some issues with "Only Selected" are expected for generative modifiers, especially ones where we can't maintain the original indices, that's not related to this patch though.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 16 2021, 7:10 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 36232.EditedApr 16 2021, 7:49 PM
  • Remember the last data type when a column is no longer visible
  • Completely gray out row filters with no corresponding visible column
Harbormaster completed remote builds in B14082: Diff 36232.Apr 16 2021, 7:50 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 16 2021, 7:54 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Simon Thommes (simonthommes) added a comment.Apr 16 2021, 8:42 PM

I'm honestly not a big fan of combining the Selected Only option with the filters under one toggle.
To me the selection works on a separate level as it's strongly and globally connected to the viewport and should be a separate toggle from the filters that are local per editor.

Hans Goudey (HooglyBoogly) added a comment.Apr 16 2021, 9:14 PM

Would you keep it in the header then? I think that toggle in the header is still a bit weird too.

Tomas (Rusculleda) added a subscriber: Tomas (Rusculleda).Apr 16 2021, 10:11 PM

I don't know if it's been mentioned already, but all the animation editors (except timeline) have a selected only option. In those cases it's in the header, and it's an icon


Maybe this could use the same?

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 19 2021, 12:34 PM

I wonder if the filters should be turned on automatically when adding a new filter. I forget this a couple of times and wondered why it's not working.

release/scripts/presets/keyconfig/keymap_data/blender_default.py
3007 ↗(On Diff #36232)

Why are you adding the toolbar key here already (same in the other keymap)?

source/blender/editors/space_spreadsheet/space_spreadsheet.cc
234

Move this into the loop in spreadsheet_main_region_draw, otherwise this data might not be updated correctly.

283

remove comment?

source/blender/editors/space_spreadsheet/spreadsheet_column.cc
65 ↗(On Diff #36232)

Use StringRefNull and display_name.c_str.

source/blender/editors/space_spreadsheet/spreadsheet_intern.hh
27 ↗(On Diff #36232)

commented code

source/blender/editors/space_spreadsheet/spreadsheet_ops.cc
82

I'd probably make these names a bit more specific, like SPREADSHEET_OT_remove_row_filter_rule. I don't really see a downside to being more specific here.

source/blender/editors/space_spreadsheet/spreadsheet_row_filter_ui.cc
87 ↗(On Diff #36232)

Not sure why you marked this as unreachable, it's perfectly reachable currently when using Name as filter when viewing instances.

204 ↗(On Diff #36232)

Feels like the proper solution would be to not create the data source during drawing but to have some new callback in SpaceType that is run before any of the regions are drawn. That does not have to be part of this patch though.

source/blender/makesrna/RNA_access.h
618 ↗(On Diff #36232)

Sort alphabetically.

This revision now requires changes to proceed.Apr 19 2021, 12:34 PM
Simon Thommes (simonthommes) added a comment.Apr 19 2021, 3:50 PM
In D10959#280653, @Tomas (Rusculleda) wrote:

I don't know if it's been mentioned already, but all the animation editors (except timeline) have a selected only option. In those cases it's in the header, and it's an icon


Maybe this could use the same?

We have to see how well this would work in combination with expanding the use of the spreadsheet for other data than geometry, but generally I'd be in favor of separating it like this.

Hans Goudey (HooglyBoogly) updated this revision to Diff 36303.Apr 19 2021, 11:52 PM
Hans Goudey (HooglyBoogly) marked 9 inline comments as done.
  • Don't gray out filter panels when their string is empty
  • Remove "T" keymap item
  • Move assigning column runtime data to the main draw function
  • Use StringRefNull
  • Give operators more specific names
  • Don't mark unreachable code incorrectly
  • Sort RNA definition automatically
  • Remove commented code / always display "Only Selected" option
Harbormaster completed remote builds in B14112: Diff 36303.Apr 19 2021, 11:52 PM
Hans Goudey (HooglyBoogly) added inline comments.Apr 19 2021, 11:52 PM
release/scripts/presets/keyconfig/keymap_data/blender_default.py
3007 ↗(On Diff #36232)

Mostly because this was copy-pasty code and we'll be getting the data source region soon, which would open with T I guess. But you're right, it shouldn't be in this patch.

source/blender/editors/space_spreadsheet/spreadsheet_ops.cc
82

Yeah, good point.

source/blender/editors/space_spreadsheet/spreadsheet_row_filter_ui.cc
204 ↗(On Diff #36232)

Okay, I'll leave that out of this patch then, I'll just always display the option.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 19 2021, 11:53 PM
Jacques Lucke (JacquesLucke) accepted this revision.EditedApr 22 2021, 1:32 PM

The code is looking good to me now. Not sure if there is still a design discussion going on.

Might be a good idea to wait with the merge until vector/color columns are combined in 2.93. Otherwise the merge will be nasty.

source/blender/editors/interface/interface_panel.c
677 ↗(On Diff #36303)

Why this change?

source/blender/editors/space_spreadsheet/space_spreadsheet.cc
264

Outdated comment.

source/blender/makesrna/intern/rna_space.c
7341

Replace all SpreadSheet with Spreadsheet.

This revision is now accepted and ready to land.Apr 22 2021, 1:32 PM
Jacques Lucke (JacquesLucke) added inline comments.Apr 22 2021, 1:42 PM
source/blender/editors/space_spreadsheet/spreadsheet_column.cc
65 ↗(On Diff #36303)

Need to free the old display name here.

Hans Goudey (HooglyBoogly) updated this revision to Diff 36416.Apr 22 2021, 4:12 PM
Hans Goudey (HooglyBoogly) marked 3 inline comments as done.
  • Merge branch 'master' into temp-spreadsheet-row-filter
  • Fix memory leak
  • Remove unecessary comment
  • Use "Spreadsheet" instead of "SpreadSheet"
Harbormaster completed remote builds in B14173: Diff 36416.Apr 22 2021, 4:12 PM
Hans Goudey (HooglyBoogly) added inline comments.Apr 22 2021, 4:22 PM
source/blender/editors/interface/interface_panel.c
677 ↗(On Diff #36303)

I'll split that out when committing, it's so the reordering works in the side panel when there are no categories visible, just a case this code didn't get tested with yet.

Jacques Lucke (JacquesLucke) mentioned this in D11056: Spreadsheet: Combine vector/color spreadsheet columns..Apr 23 2021, 10:28 AM
Hans Goudey (HooglyBoogly) updated this revision to Diff 37214.May 18 2021, 7:15 PM
  • Add support for float2, float3, color, and instance columns

One thing I'm not happy with is the headers for the row filter panels
when with vector types, the text always ends up getting cut off.

Harbormaster completed remote builds in B14625: Diff 37214.May 18 2021, 7:15 PM
Jacques Lucke (JacquesLucke) added a comment.May 19 2021, 12:59 PM

The code looks fine still. Filtering vectors and colors is quite inconvenient now unfortunately. However, I'm fine if this gets improved in a separate patch. Not sure what the other think.

Lopo Isaac (lopoIsaac) awarded a token.May 19 2021, 3:48 PM
Hans Goudey (HooglyBoogly) mentioned this in D11046: Spreadsheet: Dataset region for spreadsheet editor.May 27 2021, 2:00 AM
Hans Goudey (HooglyBoogly) updated this revision to Diff 37866.Jun 4 2021, 3:05 PM
  • Merge branch 'master' into temp-spreadsheet-row-filter
  • Fix build error after merging master
Harbormaster completed remote builds in B14966: Diff 37866.Jun 4 2021, 3:05 PM
Hans Goudey (HooglyBoogly) planned changes to this revision.EditedJun 11 2021, 6:36 PM

The results of our latest discussion about this patch:

  • Separate "Selection Only" from other filters.
  • Turning off filters should not deactivate the selection filter.
  • Grey out filters when all filters are turned off.
  • Move "Selection Only" to an icon like the graph editor
  • Don't show settings for filters that don't correspond to a visible column (including empty names)
Hans Goudey (HooglyBoogly) updated this revision to Diff 38376.Jun 16 2021, 5:14 AM
  • Split selection filter and row filters (filter flag no longer applies to selection filter)
  • Gray out row filters if the flag is turned off
  • Move "Selection Only" to the editor header
  • Remove "Last Data Type" hack
This revision is now accepted and ready to land.Jun 16 2021, 5:14 AM
Harbormaster completed remote builds in B15236: Diff 38376.Jun 16 2021, 5:14 AM
Dalai Felinto (dfelinto) added a comment.Jun 17 2021, 5:55 PM
  • Filtering should be on by default (for old files as well - doversion).
  • Suggest using "=" instead of "==" for the labels showing equality.
  • I think I'm running into an issue with 2D Vector filters:

In this case (default cube) I would expect to see some of the values. It seems that it doesn't matter what I do with the Smaller or Greater Than, I only get to hide the elements 0 and 1 of the cube.

Hans Goudey (HooglyBoogly) updated this revision to Diff 38466.Jun 17 2021, 6:32 PM
  • Use "=" for equality instead of "=="
  • Add versioning code to enable fitlering (will subversion bump when committing)
  • Fix using 3D vector values for 2D row filter
Harbormaster completed remote builds in B15287: Diff 38466.Jun 17 2021, 6:32 PM
Dalai Felinto (dfelinto) accepted this revision.Jun 18 2021, 6:14 PM

Patch is good to go (I didn't test the latest iteration, but if the remarks were addressed +1).

Non-blocking extra thoughts:
I would still be curious to see the range idea working - as the default mode with the range filled once you pick the column - that would means things don't disappear right away, but you get to gradually get rid of lines while adjusting the range). But nothing blocking anyways (the same behaviour could be applied for the greater and smaller than too, where data don't just disappear once you pick the column. It does only when you start to slide the values away from the initial smartly filled values

Hans Goudey (HooglyBoogly) added a comment.Jun 18 2021, 11:04 PM

Followup tasks:

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jun 18 2021, 11:20 PM
Closed by commit rBf9aea19d9890: Spreadsheet Editor: Row Filters (authored by Hans Goudey (HooglyBoogly)). · Explain WhyJun 18 2021, 11:33 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rBf9aea19d9890: Spreadsheet Editor: Row Filters.

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
113 lines
source/
blender/
blenkernel/
intern/
13 lines
editors/
include/
1 line
screen/
5 lines
space_spreadsheet/
2 lines
35 lines
23 lines
2 lines
122 lines
78 lines
spreadsheet_row_filter.hh
spreadsheet_ops.cc
13 lines
218 lines
makesdna/
48 lines
makesrna/
intern/
125 lines

Diff 36104

View Options

release/scripts/startup/bl_ui/space_spreadsheet.py

Loading...
View Options

source/blender/blenkernel/intern/screen.c

Loading...
View Options

source/blender/editors/include/ED_screen.h

Loading...
View Options

source/blender/editors/screen/screen_ops.c

Loading...
View Options

source/blender/editors/space_spreadsheet/CMakeLists.txt

Loading...
View Options

source/blender/editors/space_spreadsheet/space_spreadsheet.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_column_values.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_ops.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_row_filter.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_row_filter.cc

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_space.c

Loading...