Continued Outliner Code Refactor #96713

Open
opened 2022-03-22 19:34:34 +01:00 by Julian Eisel · 7 comments
Member

The Outliner code is compiled in C++ now, and a big part of the tree building logic has be ported to a new object oriented design. See contents of source/blender/editors/space_outliner/tree/. A lot of existing code can be improved by making use of this new design.

(NOTE) Note for interested GSoC participants --
A GSoC participant wouldn't be expected to get all these tasks done, this is just a general collection of TODOs. It's fine to only pick a few of these tasks, depending on the proposed project duration.

Relevant Design Notes

These basics about the new design are important to understand:

  • Each Display Mode of the Outliner is represented by an AbstractTreeDisplay sub-class, e.g. TreeDisplayViewLayer, TreeDisplaySequencer, etc.
  • Elements in the tree are represented by a AbstractTreeElement sub-class, e.g. TreeElementGPencilLayer, TreeElementNLATrack. Not all tree element types are ported to this new design yet.
  • The legacy (C-style) representation of a tree element is TreeElement. There is one for each visible element in the tree. It should be entirely replaced by the AbstractTreeElement design.

Possible Tasks

  • Replace most switch-case like logic for tree element types by virtual function calls. E.g. replace the long tree_element_get_icon() with a virtual AbstractTreeElement::getIcon().
  • Port remaining element types to the new design.
  • Get rid of TreeElement.directdata. This is type unsafe and confusing (not clear what it contains in many places). Instead the AbstractTreeElement sub-classes should store the exact data needed, and if needed expose those publicly in some way.
  • Separate element creation from tree insertion, outliner_add_element() currently does both. This causes a few problems, most importantly, all data needed to construct an element has to be passed to the function as a (type unsafe) void *.
E.g.:
{P2843}
Instead the element can be constructed first with all the necessary arguments (possibly using C++ [[ https://devtut.github.io/cpp/perfect-forwarding.html | perfect forwarding ]]) and then added to the tree. E.g.:
{P2844}
These are some ideas, each approach has some pros and cons, but all of them should be better than what we have already.

  • Add function to change the parent of an element. This is done in a number of places manually, but should be put into a function that guarantees all invariants are preserved (e.g. the TreeElement.parent pointer is set correctly).
  • Entirely replace the legacy TreeElement with AbstractTreeElement.
  • Move the tree storage from SpaceOutliner.tree to SpaceOutliner_Runtime.tree (the tree is runtime only UI data). AbstractTreeDisplay.tree may also make sense.
  • Use a better container for tree storage than ListBase. blender::Vector?
  • Convert camel case (doSomething()) function names in new C++ code to snake case (do_something()). This is the conventional style for C++ code and how it probably should've been done in the beginning.
  • Attempt to let tree-displays (AbstractTreeDisplay sub-classes) do all tree building, removing the need for AbstractTreeElement.expand().
Making the tree-display manage all tree building avoids uncertainties of having tree building split across multiple classes, instead the tree-display has full & explicit control over what items are built for the tree. The resulting tree is more predictable, and tree-elements of a certain type can be reused in different display modes without having to share the same expanding code. Tree-elements become less confusing and focus solely on their own display and operations. Sub-tree building that should be shared for multiple items can be moved to helper functions. For example to add the mesh and the materials to an object element in multiple display modes. 
E.g.: {P2845}

  • Port tree traversals to new functional-style iterators. Add some further sensible iterators (e.g. lookup iterators that can early exit and return some object of custom type). See a4a7af4732, dc6fe73e70.
  • Document the new Outliner architecture.

Optional Experiments:

  • Experiment with using the new tree-view for the Outliner. Actually porting the Outliner is a quite complex project on its own.
  • Experiment with a more functional way of building the hierarchy.

Element Type Porting Status

Overview of element types and their porting status.

IS_REAL_ID() returns true

Element Type Ported? Class Name
TSE_SOME_ID TreeElementID and subclasses.
TSE_NLA_ACTION TreeElementNLAAction
TSE_DEFGROUP_BASE TreeElementDeformGroupBase
TSE_DEFGROUP TreeElementDeformGroup
TSE_BONE TreeElementBone
TSE_EBONE TreeElementEditBone
TSE_CONSTRAINT_BASE TreeElementConstraintBase
TSE_CONSTRAINT TreeElementConstraint
TSE_MODIFIER_BASE TreeElementModifierBase
TSE_MODIFIER TreeElementModifier
TSE_LINKED_OB TreeElementLinkedObject
TSE_POSE_BASE TreeElementPoseBase
TSE_POSE_CHANNEL TreeElementPoseChannel
TSE_ANIM_DATA TreeElementAnimData
TSE_R_LAYER_BASE TreeElementViewLayerBase
TSE_R_LAYER TreeElementViewLayer
TSE_POSEGRP_BASE TreeElementPoseGroupBase
TSE_POSEGRP TreeElementPoseGroup
TSE_LINKED_PSYS TreeElementParticleSystem
TSE_LAYER_COLLECTION TreeElementLayerCollection
TSE_SCENE_COLLECTION_BASE TreeElementCollectionBase
TSE_VIEW_COLLECTION_BASE TreeElementViewCollectionBase
TSE_SCENE_OBJECTS_BASE TreeElementSceneObjectsBase
TSE_GPENCIL_EFFECT_BASE TreeElementGPencilEffectBase
TSE_GPENCIL_EFFECT TreeElementGPencilEffect

Other Elements

Element Type Ported? Class Name
TSE_NLA TreeElementNLA
TSE_NLA_TRACK TreeElementNLATrack
TSE_DRIVER_BASE TreeElementDriverBase
TSE_SEQUENCE TreeElementSequence
TSE_SEQ_STRIP TreeElementSequenceStrip
TSE_SEQUENCE_DUP TreeElementSequenceStripDuplicate
TSE_RNA_STRUCT TreeElementRNAStruct
TSE_RNA_PROPERTY TreeElementRNAProperty
TSE_RNA_ARRAY_ELEM TreeElementRNAArrayElement
TSE_ID_BASE
TSE_GP_LAYER TreeElementGPencilLayer
TSE_LIBRARY_OVERRIDE_BASE TreeElementOverridesBase
TSE_LIBRARY_OVERRIDE TreeElementOverridesProperty
TSE_LIBRARY_OVERRIDE_OPERATION TreeElementOverridesPropertyOperation
TSE_GENERIC_LABEL TreeElementLabel
The Outliner code is compiled in C++ now, and a big part of the tree building logic has be ported to a new object oriented design. See contents of `source/blender/editors/space_outliner/tree/`. A lot of existing code can be improved by making use of this new design. (NOTE) **Note for interested GSoC participants** -- A GSoC participant wouldn't be expected to get all these tasks done, this is just a general collection of TODOs. It's fine to only pick a few of these tasks, depending on the proposed project duration. ## Relevant Design Notes These basics about the new design are important to understand: - Each [Display Mode ](https://docs.blender.org/manual/en/latest/editors/outliner/interface.html#display-mode) of the Outliner is represented by an `AbstractTreeDisplay` sub-class, e.g. `TreeDisplayViewLayer`, `TreeDisplaySequencer`, etc. - Elements in the tree are represented by a `AbstractTreeElement` sub-class, e.g. `TreeElementGPencilLayer`, `TreeElementNLATrack`. Not all tree element types are ported to this new design yet. - The legacy (C-style) representation of a tree element is `TreeElement`. There is one for each visible element in the tree. It should be entirely replaced by the `AbstractTreeElement` design. ## Possible Tasks - [ ] Replace most switch-case like logic for tree element types by virtual function calls. E.g. replace the long `tree_element_get_icon()` with a `virtual AbstractTreeElement::getIcon()`. - [ ] Port remaining element types to the new design. - [ ] Get rid of `TreeElement.directdata`. This is type unsafe and confusing (not clear what it contains in many places). Instead the `AbstractTreeElement` sub-classes should store the exact data needed, and if needed expose those publicly in some way. - [ ] Separate element creation from tree insertion, `outliner_add_element()` currently does both. This causes a few problems, most importantly, all data needed to construct an element has to be passed to the function as a (type unsafe) `void *`. ``` E.g.: {P2843} Instead the element can be constructed first with all the necessary arguments (possibly using C++ [[ https://devtut.github.io/cpp/perfect-forwarding.html | perfect forwarding ]]) and then added to the tree. E.g.: {P2844} These are some ideas, each approach has some pros and cons, but all of them should be better than what we have already. ``` - [ ] Add function to change the parent of an element. This is done in a number of places manually, but should be put into a function that guarantees all invariants are preserved (e.g. the `TreeElement.parent` pointer is set correctly). - [ ] Entirely replace the legacy `TreeElement` with `AbstractTreeElement`. - [ ] Move the tree storage from `SpaceOutliner.tree` to `SpaceOutliner_Runtime.tree` (the tree is runtime only UI data). `AbstractTreeDisplay.tree` may also make sense. - [ ] Use a better container for tree storage than `ListBase`. `blender::Vector`? - [ ] Convert camel case (`doSomething()`) function names in new C++ code to snake case (`do_something()`). This is the conventional style for C++ code and how it probably should've been done in the beginning. - [ ] Attempt to let tree-displays (`AbstractTreeDisplay` sub-classes) do all tree building, removing the need for `AbstractTreeElement.expand()`. ``` Making the tree-display manage all tree building avoids uncertainties of having tree building split across multiple classes, instead the tree-display has full & explicit control over what items are built for the tree. The resulting tree is more predictable, and tree-elements of a certain type can be reused in different display modes without having to share the same expanding code. Tree-elements become less confusing and focus solely on their own display and operations. Sub-tree building that should be shared for multiple items can be moved to helper functions. For example to add the mesh and the materials to an object element in multiple display modes. E.g.: {P2845} ``` - [ ] Port tree traversals to new functional-style iterators. Add some further sensible iterators (e.g. lookup iterators that can early exit and return some object of custom type). See a4a7af4732, dc6fe73e70. - [ ] Document the new Outliner architecture. ---- ## Optional Experiments: - [ ] Experiment with using the new [tree-view ](https://wiki.blender.org/wiki/Source/Interface/Views/Tree_Views) for the Outliner. Actually porting the Outliner is a quite complex project on its own. - [ ] Experiment with a more functional way of building the hierarchy. --- ## Element Type Porting Status Overview of element types and their porting status. ### `IS_REAL_ID()` returns `true` | Element Type | Ported? | Class Name | | --- | --- | --- | | `TSE_SOME_ID` | ✅ | `TreeElementID` and subclasses. | | `TSE_NLA_ACTION` | ✅ | `TreeElementNLAAction` | | `TSE_DEFGROUP_BASE` | ✅ | `TreeElementDeformGroupBase` | | `TSE_DEFGROUP` | ✅ | `TreeElementDeformGroup` | | `TSE_BONE` | ✅ | `TreeElementBone` | | `TSE_EBONE` | ✅ | `TreeElementEditBone` | | `TSE_CONSTRAINT_BASE` | ✅ | `TreeElementConstraintBase` | | `TSE_CONSTRAINT` | ✅ | `TreeElementConstraint` | | `TSE_MODIFIER_BASE` | ✅ | `TreeElementModifierBase` | | `TSE_MODIFIER` | ✅ | `TreeElementModifier` | | `TSE_LINKED_OB` | ✅ | `TreeElementLinkedObject` | | `TSE_POSE_BASE` | ✅ | `TreeElementPoseBase` | | `TSE_POSE_CHANNEL` | ✅ | `TreeElementPoseChannel` | | `TSE_ANIM_DATA` | ✅ | `TreeElementAnimData` | | `TSE_R_LAYER_BASE` | ✅ | `TreeElementViewLayerBase` | | `TSE_R_LAYER` | ✅ | `TreeElementViewLayer` | | `TSE_POSEGRP_BASE` | ✅ | `TreeElementPoseGroupBase` | | `TSE_POSEGRP` | ✅ | `TreeElementPoseGroup` | `TSE_LINKED_PSYS` | ✅ | `TreeElementParticleSystem` | | `TSE_LAYER_COLLECTION` | ✅ | `TreeElementLayerCollection` | | `TSE_SCENE_COLLECTION_BASE` | ✅ | `TreeElementCollectionBase` | | `TSE_VIEW_COLLECTION_BASE` | ✅ | `TreeElementViewCollectionBase` | | `TSE_SCENE_OBJECTS_BASE` | ✅ | `TreeElementSceneObjectsBase` | | `TSE_GPENCIL_EFFECT_BASE` | ✅ | `TreeElementGPencilEffectBase` | | `TSE_GPENCIL_EFFECT` | ✅ | `TreeElementGPencilEffect` | ### Other Elements | Element Type | Ported? | Class Name | | --- | --- | --- | | `TSE_NLA` | ✅ | `TreeElementNLA` | | `TSE_NLA_TRACK` | ✅ | `TreeElementNLATrack` | | `TSE_DRIVER_BASE` | ✅ | `TreeElementDriverBase` | | `TSE_SEQUENCE` | ✅ | `TreeElementSequence` | | `TSE_SEQ_STRIP` | ✅ | `TreeElementSequenceStrip` | | `TSE_SEQUENCE_DUP` | ✅ | `TreeElementSequenceStripDuplicate` | | `TSE_RNA_STRUCT` | ✅ | `TreeElementRNAStruct` | | `TSE_RNA_PROPERTY` | ✅ | `TreeElementRNAProperty` | | `TSE_RNA_ARRAY_ELEM` | ✅ | `TreeElementRNAArrayElement` | | `TSE_ID_BASE` | ❌ | | | `TSE_GP_LAYER` | ✅ | `TreeElementGPencilLayer` | | `TSE_LIBRARY_OVERRIDE_BASE` | ✅ | `TreeElementOverridesBase` | | `TSE_LIBRARY_OVERRIDE` | ✅ | `TreeElementOverridesProperty` | | `TSE_LIBRARY_OVERRIDE_OPERATION` | ✅ |`TreeElementOverridesPropertyOperation` | `TSE_GENERIC_LABEL` | ✅ | `TreeElementLabel` |
Author
Member

Changed status from 'Needs Triage' to: 'Confirmed'

Changed status from 'Needs Triage' to: 'Confirmed'
Author
Member

Added subscriber: @JulianEisel

Added subscriber: @JulianEisel
Julian Eisel changed title from Outliner Code Refactor to Continued Outliner Code Refactor 2022-03-22 20:14:09 +01:00

Added subscriber: @Chirag-Mathur

Added subscriber: @Chirag-Mathur

Added subscriber: @Jian-Wang

Added subscriber: @Jian-Wang

Added subscriber: @priyanshx19

Added subscriber: @priyanshx19
Philipp Oeser removed the
Interest
User Interface
label 2023-02-10 09:22:10 +01:00

While working on the task to convert snakeCase function names to camel_case function names, I have noticed on several occurrences that there are function names that have double underscore.

I noticed some of these in outliner_draw.cc:

line 313: static void outliner__object_set_flag_recurse_fn(...) {}

line 323: static void outliner__base_set_flag_recursive_fn(...) {}

line 638: static void view_layer__layer_collection_set_flag_recursive_fn(...) {}

Those are just a few examples. Are these user errors while writing the functions, or are these intentional? There isn't any mention in the docs about this so just looking for some clarification before making any changes.

While working on the task to convert snakeCase function names to camel_case function names, I have noticed on several occurrences that there are function names that have double underscore. I noticed some of these in `outliner_draw.cc`: ``` line 313: static void outliner__object_set_flag_recurse_fn(...) {} line 323: static void outliner__base_set_flag_recursive_fn(...) {} line 638: static void view_layer__layer_collection_set_flag_recursive_fn(...) {} ``` Those are just a few examples. Are these user errors while writing the functions, or are these intentional? There isn't any mention in the [docs](https://wiki.blender.org/wiki/Style_Guide/C_Cpp) about this so just looking for some clarification before making any changes.
Author
Member

I think this intentional to indicate that these are meant to be callbacks, not regular functions. Not a usual convention and there is the _fn suffix now, so we can probably clean that up. I'm personally not bothered by it, wouldn't make an issue out of it.

I think this intentional to indicate that these are meant to be callbacks, not regular functions. Not a usual convention and there is the `_fn` suffix now, so we can probably clean that up. I'm personally not bothered by it, wouldn't make an issue out of it.
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#96713
No description provided.