Cleanup the Tree API and usage

[KoBeWi]

Describe the project you are working on

Godot etc

Describe the problem or limitation you are having in your project

Tree is this huge Control class that displays elements in a hierarchical structure. It's used in many places in the editor and has some limited use in project (mostly for custom tools). Over years it has accumulated lots of bloat functionality. Most of it came from the fact that Tree was previously used for the editor inspector. Then the inspector was replaced with a dedicated class, but the functionality has remained. Many of it isn't used very much or sometimes not at all. Another thing is that TreeItem was written under CPU/memory constrains of old game consoles, so the API doesn't make much sense nowadays.

The tree.cpp is just too big, full of hacks and full of bugs that appear regularly, which just add more hacks to get fixed. The class is in dire need of some refactoring. It should be done before 4.0, so that we can do any required breaking changes.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

At first I was going to list some API changes that just break compatibility, but I eventually came up with an idea that requires much more than that, so I'll just split this section in 2 parts. This applies to both Tree and TreeItem, as they exist together.

Simple renaming changes that could be done (I assume godotengine/godot#47665 is going to be merged):

• CELL_MODE_ICON just makes a TreeItem with centered icon. Any TreeItem might have an icon, so having a separate mode for that is useless -> should be removed
• CELL_MODE_RANGE this is a niche feature. Maybe could be removed and replaced with something
• CELL_MODE_CUSTOM is basically a TreeItem button displayed differently and with dedicated signals. Also kinda redundant
• methods like set_editable() in TreeItem should have arguments swapped. First argument is cell id, but the first column is used most often, so it could default to that
• item_double_clicked and item_activated signals do basically the same, but one is for label and the other is for icon. Could be merged into 1 signal and pass context as argument
• nothing_selected is redundant (we have empty_clicked, it's the same)
• item_edited signal could pass the edited item
• signals item_selected, item_cell_selected and item_mouse_selected do almost the same. The former 2 could be merged and pass cell as an argument
• custom_item_clicked and custom_popup_edited signals are almost the same (not sure what the latter does exactly tbh)
• column_titles_visible - is this needed? we could just auto-display column titles when any isn't empty
• set_suffix should be set_range_suffix I assume
• uncollapse_tree() should be renamed to uncollapse_subtree() (it's TreeItem method)
• get_custom_popup_rect() -> what's this even for...

Other problems beyond simply renaming things:

• all TreeItems can get an icon and text, but not all cell modes support them
• there are methods specifically for certain cell modes, like set_range_config()
• most of the TreeItem methods just set or get something on a specific cell

While the renaming part would be simple to do, the Tree could get more changes that would fix all of the problems. We could split TreeItem into 2 classes: TreeItem and TreeItem cell. So the Tree would be collection of TreeItems and each TreeItem would contain TreeItemCells. The advantage of that is that we could remove most of the methods in TreeItem and make them properties in TreeItemCell. Then add TreeItemCell sub-classes, so that different modes aren't hacked into one class with lots of unrelated properties, but are properly separated. It should make the code much cleaner and less spaghettied; just easier to maintain.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

A TreeItemCell class would be added. idk if cells have some common properties (even text isn't supported in all cell modes), might be worth making it an abstract class. With that, the class would have these subclasses:

• TreeItemCellText - for cells with text
  • TreeItemCellCheck - for checkable cells
  • TreeItemCellButton - from CELL_MODE_CUSTOM, I think that name would describe its functionality the best. Perhaps
• TreeItemCellRange - for range editing (with a subclass, having it could be justified I guess)

TreeItemCells would be only managed by their TreeItem. There wouldn't be any methods that add or remove cells, as each TreeItem would just have columns number of cells. Setting a cell type would remove the old one and instance a new class, clearing data from the old one. All set_* methods in TreeItem would be converted to properties in TreeItemCell. TreeItem would just get a method get_cell(idx) to obtain a cell.

Most of the code for operating on TreeItems and cells could be moved to their classes. The signals should still be emitted from Tree. Maybe items and cells could just have a method called emit_tree_signal(signal, args) to make it simpler (TreeItem and TreeItemCell don't exist without Tree, so coupling like that isn't a problem). Tree would be mostly focused on managing TreeItems and TreeItem on managing cells. Right now the code is intertwined a lot; there should be some way to split it.

This probably doesn't cover everything, so if there is something I forgot, feel free to comment so I can update. This is lots of changes and it should be discussed.

If this enhancement will not be used often, can it be worked around with a few lines of script?

If you divide the number of lines by 1000 then yes.

Is there a reason why this should be core and not an add-on in the asset library?

I know abandoning Tree class in favor of clean custom implementation is tempting, but the core one should be fixed.

[fire-forge]

Another idea I had is to make TreeItem a Node. Right now TreeItems can only be created and edited through code, but if they were nodes then their properties could be edited in the inspector and the hierarchy could be edited in the scene tree. Drawing would still be handled by the Tree node itself.

Then several of the TreeItem methods can be removed in favor of the similar methods that exist in Node:

• create_child()
• get_child()
• get_child_count()
• get_children()
• get_first_child()
• get_parent()
• remove_child()

[KoBeWi]

TreeItem can't be a Node as Nodes are just too expensive. Imagine you have 1000 nodes on your scene and then there is 1000 more in the scene tree dock to represent that. They have lots of useless overhead functionality like pause behavior, processing, groups. TreeItem just doesn't need it.

We could add a custom editor to edit Tree contents in the inspector, but Trees are most often created dynamically, so there is no real use-case for that.

[fire-forge]

@KoBeWi Ok, that's understandable. It's definitely better to keep the more efficient solution we have now then.

We could add a custom editor to edit Tree contents in the inspector, but Trees are most often created dynamically, so there is no real use-case for that.

I agree. It wouldn't be worth it to create a custom editor for TreeItems because of how little it'd be used, but if someone wanted this it could be done with a plugin using the current API.

[fire-forge]

Two more changes I'd make to TreeItem:

• Change set_custom_draw() to use a Callable. Most other places that accept an object and method name were changed to use Callable for 4.0
  set_custom_draw(column: int, object: Object, callback: StringName) -> set_custom_draw(column: int, callable: Callable)
• Change set_custom_bg_color() to set_custom_bg_stylebox(). Using a rectangular StyleBoxFlat gives the same appearance as a custom color, but it also means you can use rounded corners, drop shadow, texture, etc. without using set_custom_draw()
  • This could be used in the editor for giving rounded corners to tree items that use set_custom_bg_color(), like the node types in the signals dock:
    

[YuriSizov]

Approved in the proposal review meeting! If you want to work on this, we'd love to have it before 4.0, though this does look like a lot of work, so pace yourself :)

[KoBeWi]

I actually started some work: https://github.com/KoBeWi/godot/tree/treework
But after seeing the 500-line-long TreeItem drawing method, I have some doubts if it ever gets finished 🙃
So far I added the new classes and the new Tree will draw some broken text when you add items.

[me2beats]

Another idea I had is to make TreeItem a Node. Right now TreeItems can only be created and edited through code, but if they were nodes then their properties could be edited in the inspector and the hierarchy could be edited in the scene tree. Drawing would still be handled by the Tree node itself.

I think this could be solved by implementing a Tree editor like we have for ItemList for example. Would be great especially for prototyping

[KoBeWi]

I just discovered that CELL_MODE_RANGE has double functionality. It's a slider value, but if you give it text, it magically becomes a dropdown 🤦‍♂️In my (now dead) refactor PR I made TreeItemCellRange, but now I see that there needs to be another class, likely TreeItemCellDropdown.

btw this proposal has 5.0 milestone, but it can be implemented in Godot 4 as a separate class (like AnimationTree and AnimationTreePlayer in Godot 3).

[Shadowblitz16]

TreeItem can't be a Node as Nodes are just too expensive. Imagine you have 1000 nodes on your scene and then there is 1000 more in the scene tree dock to represent that. They have lots of useless overhead functionality like pause behavior, processing, groups. TreeItem just doesn't need it.

We could add a custom editor to edit Tree contents in the inspector, but Trees are most often created dynamically, so there is no real use-case for that.

I don't understand any modern markup gui has separate objects per ui element.
This seems like a implementation issue rather then node's just strait up being not suitable.

If node's are too slow then optimize them.

I mean there is wpf and avalonuia ui that treat tree node's as separate tree objects and that's written in C# and not c++,
And avalonia ui is known for being really fast.

Also I think they would be more useful if they had the features node's have.
Half the time godot is too bare bones for the things I want to do with it.