Page MenuHome

UI Code Quality: Start refactoring Outliner tree building & port to C++
ClosedPublic

Authored by Julian Eisel (Severin) on Nov 7 2020, 2:33 PM.
Tokens
"100" token, awarded by Kickflipkid687."Love" token, awarded by Tetone."Like" token, awarded by JacquesLucke."Love" token, awarded by HooglyBoogly."Burninate" token, awarded by natecraddock.

Details

Summary
NOTE: I do not ask for an in-depth review. This is just to make sure other devs agree on this rather significant shift in design.
Motivation

Problems with current design:

  • The Outliner tree building code is messy and hard to follow.
  • Hard-coded display mode checks are scattered over many places.
  • Data is passed around in rather unsafe ways (e.g. lots of void *).
  • There are no individually testable units.
  • Data-structure use is inefficient.

The current Outliner code needs quite some untangling, the tree building seems like a good place to start.

This introduces a new abstraction "tree-display" to help constructing and managing the tree for the different display types (View Layer, Scene, Blender file, etc.).

Design Idea

Idea is to have an abstract base class (blender::ed::outliner::AbstractTreeDisplay), and then sub-classes with the implementation for each display type (e.g.
TreeDisplayViewLayer, TreeDisplayDataAPI, etc). The tree-display is kept alive until tree-rebuild as runtime data of the space, so that further queries based
on the display type can be executed (e.g. "does the display support selection syncing?", "does it support restriction toggle columns?", etc.).
New files are in a new space_outliner/tree sub-directory.

Note that this patch only converts the View Layer and Blender File mode, eventually it should look like this:

The following sections are commit messages for further refactors I've done here.


UI Code Quality: General refactor of Outliner View Layer display mode building
  • Turn functions into member functions (makes API for a type more obvious & local, allows implicitly sharing data through member variables, enables order independend definition of functions, allows more natural language for function names because of the obvious context).
  • Move important variables to classes rather than passing around all the time (shorter, more task-focused code, localizes important data names).
  • Add helper class for adding object children sub-trees (smaller, more focused units are easier to reason about, have higher coherence, better testability, can manage own resources easily with RAII).
  • Use C++ iterators over C-macros (arguably more readable, less macros is generally preferred)
  • Add doxygen groups (visually emphasizes the coherence of code sections, provide place for higher level comments on sections).
  • Prefer references over pointers for passing by reference (makes clear that NULL is not a valid value and that the current scope is not the owner).

UI Code Quality: Use C++ data-structures for Outliner object hierarchy building
  • Use blender::Map over GHash
  • Use blender::Vector over allocated ListBase *

Benefits:

  • Significantly reduces the amount of heap allocations in large trees (e.g. from O(n) to O(log(n)), where n is number of objects).
  • Higher type safety (no void *, virtually no casts).
  • More optimized (e.g. small buffer optimization).
  • More practicable, const-correct APIs with well-defined exception behavior.

Code generally becomes more readable (less lines of code, less boilerplate,
more logic-focused APIs because of greater language flexibility).

Diff Detail

Repository
rB Blender
Branch
outliner-cpp-refactor
Build Status
Buildable 11188
Build 11188: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Nov 7 2020, 2:33 PM
Julian Eisel (Severin) created this revision.
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 7 2020, 2:40 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)

TreeViewViewLayer reads and looks a bit weird, I think it's better to name the classes TreeView_ViewLayer, TreeView_DataAPI, TreeView_Libraries, ... Even if that isn't a common convention.

source/blender/editors/space_outliner/tree/tree_view_view_layer.cc
186 ↗(On Diff #30862)

Typo.

I think it's fine to have this kind of abstraction.

The most important abstraction to introduce in the outliner is for tree elements I think. So that there is a generic mechanism to generate children, draw, drag & drop, select, activate, delete, reorder, show menus, etc. That's far beyond the intent of this patch, but just throwing it out there.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 7 2020, 2:52 PM

Yes that is my idea as well. This just seemed like the logical first part to untangle (lower-hanging fruit) before changing less isolated bits. Tree elements are accessed by all Outliner files after all.

This is a great improvement. Like Brecht I have also wanted to use class relationships for all of the tree element types, and this is a good start for that.

TreeViewViewLayer reads and looks a bit weird, I think it's better to name the classes TreeView_ViewLayer, TreeView_DataAPI, TreeView_Libraries, ... Even if that isn't a common convention.

If this class is only for building the tree it could also be called TreeBuilder or TreeCreator, though I'm not against the underscore separator.

The class is not supposed to be a mere builder. After the basics are done, I'd like to add queries to the viewer. IE rather than:

if (ELEM(space_outliner->outlinevis, SO_FOO, SO_BAR)) {
  do_something();
}

... it would use virtual function call queries:

if (view->needsSomething()) {
  do_something();
}

And if there is further display-mode specific logic to be executed, it would be moved to the viewer as well.
That's why this patch keeps the view-object alive, even after building is done, so further queries are possible.

Guess you could also consider it a display mode adapter. Although that doesn't seem like a great name either, but a nice brief description.

Then perhaps AbstractTreeDisplay would be a good name, since we already use the word "display" in the interface for the mode. I like the sound of TreeDisplayViewLayer

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 7 2020, 4:31 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 7 2020, 4:54 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 7 2020, 4:56 PM
  • Convert Outliner Blender File mode to new tree buiding design
  • Cleanup: General cleanup of Outliner Blender File display mode building
  • Cleanup: Follow C++ code style for new Outliner building code
  • Cleanup: Rename Outliner "tree-view" types to "tree-display" & update comments
  • Cleanup: Split header for Outliner tree building into C and C++ headers

If there are no objections against this new design I will merge this into master soon and continue work there (converting remaining display modes).

This revision is now accepted and ready to land.Nov 9 2020, 5:37 PM

Note: The description is a bit out-dated, I switched to the term "tree-display" rather than "tree-view", e.g. AbstractTreeDisplay, TreeDisplayViewLayer, TreeDisplayLibraries, etc. Will update text and picture soon.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 11 2020, 6:49 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)

Alright, pushed this all into master now. Thanks for the feedback!