Page MenuHome

Spreadsheet: New spreadsheet editor (MVP).
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Mar 1 2021, 1:49 PM.

Details

Summary

This adds the MVP functionality for the new spreadsheet editor (T85879). All the boilerplate code for adding a new editor type is in D10645.

Getting the abstraction layers right for the MVP is a bit difficult, because it should be relatively simple, even though we know many things we want to add very soon.
For now I abstracted away the core drawing code in spreadsheet_draw.cc. It is responsible for drawing the table itself, and calls callbacks on a SpreadsheetDrawer instance for every visible cell. I expect this code to stay mostly the same for a while.
The code in spreadsheet_from_geometry.cc on the other hand will probably have to be reshuffled a couple more times in the near future to support the different features we planned (filtering, sorting, ...).

Supported features:

  • Show point attributes of evaluated meshes (no original data, no other geometry types, no other domains, yet).
  • Only show data for selected vertices when the mesh object is in edit mode.
  • All data is readonly.

Diff Detail

Repository
rB Blender
Branch
temp-spreadsheet-editor (branched from master)
Build Status
Buildable 13177
Build 13177: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Mar 1 2021, 1:49 PM
Jacques Lucke (JacquesLucke) created this revision.
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
  • add theme for spreadsheet editor

Will commit the the corresponding change for the addons repo once
this is in master.

Jacques Lucke (JacquesLucke) retitled this revision from Spreadsheet: New spreadsheet editor (WIP). to Spreadsheet: New spreadsheet editor..
Hans Goudey (HooglyBoogly) requested changes to this revision.Mar 2 2021, 1:17 AM

Overall this looks nice! The code is relatively simple and easy to understand, and looks nicely extensible.


There seems to be some instability in the column width calculation:


The header background color is much darker than other editors. I don't think this is purposeful?


There is a missing redraw when switching to and from edit mode.


It was a little hard to see the advantage of the ResourceCollector abstraction (as opposed to adding everything to a temporary struct) at first.
Looks like it's a way to make the data gathered in the attribute specific code available in its caller. I don't know, maybe that's worth a comment.


Some of the code in the from_geometry file seems like it shouldn't be in that file.
The set of functions starting at draw_float_in_cell seem like they could be better named draw_cell_value_as_label, or something like that. Since for any evaluated data spreadsheet mode you will need the same functionality.
You mention this above, so I don't know if you wanted to wait to address this.


I think the header row is slightly too tall. Something closer to 1.1 looks (to me) like a better factor there.


Some rambling thoughts:
One of the larger interesting implementation decision seems to be that this doesn't use RNA at all to draw the evaluated attributes. Usually I would be a bit trepidatious about that since it's nice to work to unifying the exposed interface to the UI code, but two things are different here:

  • All evaluated attributes aren't exposed to RNA.
  • That might be less efficient anyway.
source/blender/editors/space_spreadsheet/spreadsheet_draw.hh
37

Use call this the "header row" in other places. It would probably be best to be consistent with that naming.

source/blender/editors/space_spreadsheet/spreadsheet_from_geometry.cc
71

"It might" is not a great insurance here. It would be good to briefly state a reason to expect that it might not be the case sometimes.

186

I think we discussed removing this sorting? Without it they should show up in the order the attribute providers are defined, right? I think that would be the order we'd want here.

333

No big deal, but get_display_geometry_set seems like a simpler way of saying the same thing.

361

I guess the idea is that it's not even worth trying in other modes, since modifiers might lose the selection data? I guess it is in the MVP design task.

432

Why not just return it directly?

source/blender/editors/space_spreadsheet/spreadsheet_from_geometry.hh
29–30

I'm not sure I'm convinced that this needs a separate header file. Is there a strong reason not to put this in spreadsheet_intern.hh?

This revision now requires changes to proceed.Mar 2 2021, 1:17 AM

There seems to be some instability in the column width calculation

Interesting, not sure what might be causing this. I could not reproduce exact behavior you have. Does the same issue exist when you simply resize an editor in Blender, or do you have to resize Blender itself?
I do get some text jumping around for unknown reasons well (note that this also affects the header, in which I don't do any custom drawing):

The header background color is much darker than other editors. I don't think this is purposeful?

I did not do it on purpose indeed. It seems to be darker, because I'm copying the theme from the file browser. Is the darker header intentional in the file browser?

It was a little hard to see the advantage of the ResourceCollector abstraction (as opposed to adding everything to a temporary struct) at first.

I understand, it's a concept that is not very common (maybe for good reason?). However, I found it very useful in the past to separate concerns. The main idea is that I can give a resource collector to a function and it will return me something that lives at least as long as the resource collector. This helped me when returning references to data that are sometimes statically allocated and sometimes dynamically allocated (as is the data referenced by the span returned by filter_visible_mesh_vertex_rows) (it helped me in other cases before, but those are not relevant here yet). An alternative would be to always return a vector or an array. Then the caller explicitly takes ownership of the data, but it also forces me to create a new vector, even if it is not necessary. To circumvent this, I could also return a Span and a bool that says if the data has to be freed. This works and is efficient, but it is also cumbersome to manage it makes it easier to leak memory accidentally. I just found it significantly more convenient to simply tell the function how long I want the resulting data to live at least using the resource collector. Hope that makes some sense.

The set of functions starting at draw_float_in_cell seem like they could be better named draw_cell_value_as_label, or something like that.

Sounds like a fine change, I just wanted to keep these names fairly specific right now, instead of overgeneralizing thing code. At first, I even had those functions in a separate file, but I figured it wasn't worth it yet, as this stuff has to change soonish anyway (also depending on at which level we want to implement filtering etc.).

One of the larger interesting implementation decision seems to be that this doesn't use RNA at all to draw the evaluated attributes.

The good thing is, the "core drawing code" does not care whether the cells use rna for drawing or not. So it should be relatively straight forward to use it when we need it. However, especially when we want to display some intermediate generated data, we might not be able to access it through rna ever, not sure.

source/blender/editors/space_spreadsheet/spreadsheet_from_geometry.cc
186

Hmm, I thought within the categories, we still want alphabetical sorting (only that attribute categories are not implemented yet).

361

Yeah, I had trouble making it work with other modes consistently. Just supporting edit mode seems fine to me.

432

Sometimes I like to store things in separate variables to ease debugging, but here it does not help too much indeed.

  • Merge branch 'master' into temp-spreadsheet-editor
  • fix ensure bmesh table
  • make title row a bit thinner
  • update editor on mode change
  • make naming more consistent
  • improve comment
  • cleanup

@Brecht Van Lommel (brecht), adding you as a reviewer, because you worked on a spreadsheet implementation before.

Does the same issue exist when you simply resize an editor in Blender, or do you have to resize Blender itself?

Testing this again now, and no, it's only when I resize the window. Some print statements tell me that it's actually the value that BLF_width returns that sometimes changes when the window is resized. That's quite weird...
I'm not even able to reproduce it consistently though, so I'm not sure if it's worth putting too much effort into fixing it right now.

  • Cleanup: clang-tidy
  • Merge branch 'master' into temp-spreadsheet-editor

Looks generally fine.

In terms of implementation I would have expected:

  • AttributeColumn to be a generic SpreadsheetColumn not specific to attributes.
  • Value drawing to use RNA properties so that number formatting and editing is taken care of by the UI code.

But these are the kind of things that can evolve as the implementation gets completed and more types of spreadsheets are added..

Right, most things in spreadsheet_from_geometry.cc are fairly like to change when the design matures. Right now I didn't want to make it too generic.
Maybe we could even just commit this without exposing the new space in the editor menu. Getting all the boilerplate code for a new editor out of the way, would make it much simpler to continue to work on it.

Just as a side note, I was also testing to define what is visible in the spreadsheet from python today (in the temp-spreadsheet-editor-python-prototyping branch). It was relatively straight forward to get that working based on the spreadsheet_draw.hh abstraction. This experiment just shows that we can easily depend on rna properties where possible and also have editable values:

The spreadsheet in the image is defined by this python file: https://developer.blender.org/diffusion/B/browse/temp-spreadsheet-editor-python-prototyping/release/scripts/modules/bpy_spreadsheet.py
Again, this is just an experiment, but it's interesting to see it working.

Spreadsheet: Boilerplate for adding the new editor type.

  • revert previous update which was supposed to go in a separate diff
  • rebase on boilerplate patch (D10645)
Jacques Lucke (JacquesLucke) retitled this revision from Spreadsheet: New spreadsheet editor. to Spreadsheet: New spreadsheet editor (MVP)..Mar 9 2021, 11:32 AM
  • update view2d before drawing

I think this is a great first step, and it is good enough to be merged to master.

I found two bugs by the way:

  1. General draw glitches:

  1. Add a cube and a torus, select the torus, go to the last vertex, now select the cube

The editor shows nothing. It go back into range only after mouse scrolling.

Works super well for my tests as a first MVP!

The only issues I found are only related to the limited scope.

  • the propagation of the selected state could be better but this will work a little differently when we have different spreadsheet contexts to choose from
  • a lot of information is still missing due to the fact that it is only limited to the vertex domain (that's by design, it's just not easy to understand for the user right now, as it's not indicated explicitly)

I'll leave specific code review to the geometry nodes team.

I'll just echo what Brecht said earlier, I agree with those two points. But I understand this can easily change in the future.

As an MVP this is fine. Beyond that, there are so many features we know we want that it's not really worth bringing anything up here.

The only problem I encountered is that the spreadsheet does not update when I play back, this should probably be fixed .

This revision is now accepted and ready to land.Mar 9 2021, 5:51 PM
  • redraw on frame change

Note that this does not redraw every frame when playing back the animation.
This is the same behavior as the properties editor for me.

This revision was automatically updated to reflect the committed changes.