godotengine
[Tree] Improve Tree Performance by replacing computed height with TreeItem's cached minimum size
Originally, TreeItem's content height was computed in every draw while traversing the tree, even if the item content never changes. And there's already minimum_size cached in each item, which is computed in a similar way.
Tested on a massive plain-tree with 15K and 150K items(project file attached below), and got significant performance improvement (About 5~10x). Here're results on my desktop when scrolling to near the bottom of tree.
| Amount | This PR | Master |
|---|---|---|
| 15K Items | 1543FPS | 152FPS |
| 150K Items | 36FPS | 3FPS, and with a lot much longer start time |
I also checked the behaviors of more complex Trees in the editor, and didn't notice any obvious artifacts, but I'm not very sure if there were any side-effect after changing these.
Might partially fix these issues?
- Bugsquad edit, closes: https://github.com/godotengine/godot/issues/70869
- Bugsquad edit, closes: https://github.com/godotengine/godot/issues/83460
EDIT:
After some "extreme" test with huge amount of auto-wrapping text TreeItems and scrolling fast will cause some item's height cache being outdated.
~~I'm still looking for some other way to make this work. So convert this to draft now and will try reopen when I figured out.~~
I believe that's not related to this PR's modification, as I can reproduce the above issue in both 4.3 and master branch. I'll open another issue about that.
Oops, I think I messed up something in the commit, will commit again.
@ALiwoto Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.
After testing, it works great in my personal project, now I can directly use Tree to build my tools 👍
master:
this pr:
I see some discrepancy between the code you deleted in compute_item_height() and TreeItem's get_minimum_size(), e.g. the latter does not account for theme_cache.checked height.
I see some discrepancy between the code you deleted in
compute_item_height()and TreeItem'sget_minimum_size(), e.g. the latter does not account fortheme_cache.checkedheight.
I'm wondering the reason that get_minimum_size() didn't take theme_cache.checked's height into account. If there's nothing affected I'll re-add that into the method.
Also I noticed that update_item_cell() seems to be called twice every time (another in get_minimum_size()).
Maybe we can remove the following one?
https://github.com/godotengine/godot/blob/f5f813e9674fa433a9702ea614bcd56e59dd80db/scene/gui/tree.cpp#L1824-L1827
https://github.com/godotengine/godot/blob/f5f813e9674fa433a9702ea614bcd56e59dd80db/scene/gui/tree.cpp#L1523-L1525
This PR is working fine in my project, what's causing it not to be merged yet?
Also I noticed that update_item_cell() seems to be called twice every time (another in get_minimum_size()).
Only once call will be effective, unless the cell becomes dirty again for some reason.
The scene tree is noticeably bigger vertically. The left is with this PR, the right is without:
The scene tree is noticeably bigger vertically. The left is with this PR, the right is without:
Thanks for pointing out!
I've found that's because of the cached minimum size computes buttons height using the combination of button texture's size and button_pressed's minimum_size, compared with the old calculation method.
https://github.com/godotengine/godot/blob/f5f813e9674fa433a9702ea614bcd56e59dd80db/scene/gui/tree.cpp#L1542-L1552
As cached minimum size was used only for width calculation before this PR, I'm keeping the width combination here.
To match the original Tree::compute_item_height's implementation, I just discard the height of button button_pressed's minimum size. Or should it use the maximum height instead?
Or should it use the maximum height instead?
There is some weird behavior when button_pressed is bigger than other styles, but it's the same as the one on master, so I think it's fine.
