Skip to content

Optimize tree size computation and the scene tree dock filter #110759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
precup wants to merge 1 commit into
base: master
Choose a base branch
from
+27 −6
Open

Optimize tree size computation and the scene tree dock filter #110759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions editor/scene/scene_tree_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,17 @@ void SceneTreeEditor::_update_tree(bool p_scroll_to_selected) {
}

bool SceneTreeEditor::_update_filter(TreeItem *p_parent, bool p_scroll_to_selected) {
TreeItem *last_selected = nullptr;
bool result = _update_filter_helper(p_parent, p_scroll_to_selected, last_selected);
if (p_scroll_to_selected && last_selected) {
// Scrolling to the first selected in the _update_filter call above followed by the last
// selected here is enough to frame all selected items as well as possible.
callable_mp(tree, &Tree::scroll_to_item).call_deferred(last_selected, false);
}
return result;
}

bool SceneTreeEditor::_update_filter_helper(TreeItem *p_parent, bool p_scroll_to_selected, TreeItem *&r_last_selected) {
if (!p_parent) {
p_parent = tree->get_root();
filter_term_warning.clear();
Expand Down Expand Up @@ -1026,7 +1037,8 @@ bool SceneTreeEditor::_update_filter(TreeItem *p_parent, bool p_scroll_to_select
bool keep_for_children = false;
for (TreeItem *child = p_parent->get_first_child(); child; child = child->get_next()) {
// Always keep if at least one of the children are kept.
keep_for_children = _update_filter(child, p_scroll_to_selected) || keep_for_children;
// Only scroll if we haven't already found a child to scroll to.
keep_for_children = _update_filter_helper(child, p_scroll_to_selected && !keep_for_children, r_last_selected) || keep_for_children;
}

if (!is_root) {
Expand Down Expand Up @@ -1099,9 +1111,13 @@ bool SceneTreeEditor::_update_filter(TreeItem *p_parent, bool p_scroll_to_select
if (editor_selection) {
Node *n = get_node(p_parent->get_metadata(0));
if (selectable) {
if (p_scroll_to_selected && n && editor_selection->is_selected(n)) {
// Needs to be deferred to account for possible root visibility change.
callable_mp(tree, &Tree::scroll_to_item).call_deferred(p_parent, false);
if (n && editor_selection->is_selected(n)) {
if (p_scroll_to_selected) {
// Needs to be deferred to account for possible root visibility change.
callable_mp(tree, &Tree::scroll_to_item).call_deferred(p_parent, false);
Copy link
Member

@KoBeWi KoBeWi Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this (or the one above) is called for every selected item, so the Tree will needlessly scroll to each item, eventually settling on the last one. This can be queued with something like

void _queue_scroll_to_item(TreeItem *p_item) {
	if (!queued_item) {
		callable_mp(this, &SceneTreeEditor::_scroll_to_item).call_deferred();
	}
	queued_item = p_item;
}

void _scroll_to_item() {
	tree->scroll_to_item(queued_item );
	queued_item  = nullptr;
}

Copy link
Contributor Author

@precup precup Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to be called every time, but this PR actually prevents that via the condition update on line 1041. p_scroll_to_selected will only be true the first time a child is found, and will be false subsequently, so it actually only scrolls to the first element it finds. That's the main performance boost this PR provides, actually.

Calling it a single time on the last item only isn't ideal. Really, you want to call it on the first element once, and then the last element once, because doing so will frame as many of the elements as possible. If you do it only to the first or last element, it might not scroll to show as many of the selected elements. In order to preserve that behavior, I used this somewhat complicated boolean passing instead of a solution like the one you proposed.

I could accomplish the same result as what I'm doing now via a similar method, though, if you'd prefer.

} else {
r_last_selected = p_parent;
}
}
} else if (n) {
editor_selection->remove_node(n);
Expand Down
1 change: 1 addition & 0 deletions editor/scene/scene_tree_editor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ class SceneTreeEditor : public Control {

void _test_update_tree();
bool _update_filter(TreeItem *p_parent = nullptr, bool p_scroll_to_selected = false);
bool _update_filter_helper(TreeItem *p_parent, bool p_scroll_to_selected, TreeItem *&r_last_selected);
bool _node_matches_class_term(const Node *p_item_node, const String &p_term);
bool _item_matches_all_terms(TreeItem *p_item, const PackedStringArray &p_terms);
void _tree_changed();
Expand Down
7 changes: 5 additions & 2 deletions scene/gui/tree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2008,7 +2008,8 @@ int Tree::compute_item_height(TreeItem *p_item) const {
for (int i = 0; i < columns.size(); i++) {
height = MAX(height, p_item->get_minimum_size(i).y);
}
int item_min_height = MAX(theme_cache.font->get_height(theme_cache.font_size), p_item->get_custom_minimum_height());
int font_height = cache.font_height != -1 ? cache.font_height : theme_cache.font->get_height(theme_cache.font_size);
int item_min_height = MAX(font_height, p_item->get_custom_minimum_height());
if (height < item_min_height) {
height = item_min_height;
}
Expand Down Expand Up @@ -5024,7 +5025,8 @@ void Tree::_notification(int p_what) {
} break;

case NOTIFICATION_DRAW: {
v_scroll->set_custom_step(theme_cache.font->get_height(theme_cache.font_size));
int font_height = cache.font_height != -1 ? cache.font_height : theme_cache.font->get_height(theme_cache.font_size);
v_scroll->set_custom_step(font_height);

update_scrollbars();
RID ci = get_canvas_item();
Expand Down Expand Up @@ -5120,6 +5122,7 @@ void Tree::_notification(int p_what) {
}

void Tree::_update_all() {
cache.font_height = theme_cache.font->get_height(theme_cache.font_size);
for (int i = 0; i < columns.size(); i++) {
update_column(i);
}
Expand Down
1 change: 1 addition & 0 deletions scene/gui/tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ class Tree : public Control {
int hover_button_index_in_column = -1;

bool rtl = false;
int font_height = -1;
} cache;

int _get_title_button_height() const;
Expand Down