Page MenuHome

Refactor: Simplify spreadsheet handling of cell values
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Dec 5 2021, 8:11 PM.

Details

Summary

Previously we used a CellValue class to hold the data for a cell,
and called a function to fill it whenever necessary. This is an
unnecessary complication when we have virtual generic arrays
and most data is already easily accessible that way anyway.
This patch removes CellValue and uses fn::GVArray to provide
access to data instead.

In the future, if rows have different types within a single column,
we can use a GVArray of blender::Any to interface with the drawing.

Along with that, the use of virtual arrays made it easy to do a
few other cleanups:

  • Use selection domain interpolations from rB5841f8656d95 for the mesh selection filter.
  • Change the row filter to only calculate for necessary indices (this makes the patch a bit bigger than I would like, but it's a bit annoying to untangle).

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Dec 5 2021, 8:11 PM
Hans Goudey (HooglyBoogly) created this revision.
  • Merge branch 'master' into arcpatch-D13478
  • Fix: Check for existing column in row filter
  • Fix selection filter
  • Remove unnecessary changes
  • Cleanup: Move comment
  • Remove unnecessary code
  • Fix: Add missing break
  • Fix build error
Hans Goudey (HooglyBoogly) retitled this revision from Refactor: Simplify spreadhsset handling of cell values (WIP) to Refactor: Simplify spreadhsset handling of cell values.Dec 15 2021, 1:32 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) retitled this revision from Refactor: Simplify spreadhsset handling of cell values to Refactor: Simplify spreadhseet handling of cell values.Dec 15 2021, 5:18 AM
Jacques Lucke (JacquesLucke) requested changes to this revision.Dec 15 2021, 11:37 AM

That's a great improvement :)

  • Add a note to the commit message that it is still possible to have a dynamically typed column by putting e.g. an Any into a virtual array.
source/blender/editors/space_spreadsheet/spreadsheet_column_values.hh
42

Add assert to check that the varray is not null.

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc
54

This shouldn't be done here, but in blenkernel somewhere.

215

Be careful with referencing the context when building a lambda that might outlive the surrounding scope. Better be explicit about what has to be captured. Same below.

source/blender/editors/space_spreadsheet/spreadsheet_layout.cc
188

Using a const reference here is not wrong, but it could be misleading, because get does not (and can not return a reference.

source/blender/editors/space_spreadsheet/spreadsheet_row_filter.cc
67

Here it would be ok to use [&] because the lambda is only evaluated in the called function.

212

Could just use .id instead of reinterpret_cast.

source/blender/functions/FN_generic_virtual_array.hh
178

The comment makes it sound like you pass in the destination that the value is copied to.

source/blender/makesdna/DNA_space_types.h
2018

Think we should use -1 instead of 8. That might be more future proof.

This revision now requires changes to proceed.Dec 15 2021, 11:37 AM
Hans Goudey (HooglyBoogly) marked 7 inline comments as done.
  • Merge branch 'master' into cleanup-spreadsheet-simplify
  • Use -1 for unkown spreadsheet value type
  • Improve GVArray comment
  • Add assert for empty spreadsheet column
  • Move CPPType declaration to blenkernel
  • Use explicit capture when function outlives scope
  • Don't use reference on GVArray::get result
  • Use generic capture for local lambdas
  • Small fix
  • Small cleanup
source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc
54

Good point. I moved it to geometry_component_instances.cc

source/blender/editors/space_spreadsheet/spreadsheet_row_filter.cc
212

Unless there's a reason not to, I think I prefer doing it this way though, since I can avoid including DNA_object_types.h and DNA_collection_types.h

source/blender/editors/space_spreadsheet/spreadsheet_row_filter.cc
212

I don't see much value in not including these headers here tbh.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Use .id to get name
source/blender/editors/space_spreadsheet/spreadsheet_row_filter.cc
212

Okay, this is using .id now

This revision is now accepted and ready to land.Dec 15 2021, 4:30 PM
Hans Goudey (HooglyBoogly) retitled this revision from Refactor: Simplify spreadhseet handling of cell values to Refactor: Simplify spreadsheet handling of cell values.Dec 15 2021, 4:30 PM