Improve Scene Tree editor performance #99700

hpvb · 2024-11-26T01:39:49Z

We now cache the Node*<>TreeItem* mapping in the SceneTreeEditor. This allows us to make targeted updates to the Tree used to display the scene tree in the editor.

Previously on almost all changes to the scene tree the editor would rebuild the entire widget, causing a large number of deallocations an allocations. We now carefully manipulate the Tree widget in-situ saving a large number of these allocations.

In order to know what Nodes need to be updated we add a editor_state_changed signal to Node, this is a TOOLS_ENABLED, editor-only signal fired when changes to Node happen that are relevant to editor state.

We also now make sure that when nodes are moved/renamed we don't check expensive properties that cannot contain NodePaths. This saves a lot of time when SceneTreeDock renames a node in a scene with a lot of MeshInstances. This makes renaming nodes go from ~27 seconds to ~2 seconds on large scenes.

SceneTreeEditor instances will now also not do all of the potentially expensive update work if they are invisible. This behavior is turned off by default so it won't affect existing users. This change allows the editor to only update SceneTreeEditors that actually in view. In practice this means that for most changes instead of updating 6
SceneTreeEditors we only update 1 instantly, and the others only when they become visible.

There is definitely more that could be done, but this is already a massive improvement. In complex scenes we see an improvement of 10x, things that used to take ~30 seconds now only take 2.

This fixes #83460

I want to thank @KoBeWi, @TokisanGames, @a-johnston, @daniel080400 for their tireless testing. And @AeioMuch for their testing and providing a fix for the hover issue.

hpvb · 2024-11-26T01:41:08Z

Before: (this scene has about 15,000 nodes)

Screencast.From.2024-11-26.02-11-22.mp4

After:

Screencast.From.2024-11-26.02-10-35.mp4

arkology

Stealing @AThousandShips 's job 🤣

editor/gui/scene_tree_editor.cpp

editor/gui/scene_tree_editor.h

scene/gui/tree.cpp

editor/gui/scene_tree_editor.cpp

arkology

I confirm that birdies are in the nest (dots at the end of the comments)😃

editor/gui/scene_tree_editor.cpp

KoBeWi · 2024-11-26T13:25:12Z

Changing valid types does not update the nodes (they stay disabled/enabled).

Can be tested with this simple EditorScript:

extends EditorScript
func _run() -> void:
	EditorInterface.popup_node_selector(func(n): print(n), ["Node2D"])

Run it, then remove "Node2D" and run again. Nodes that didn't match originally will stay disabled.

editor/gui/scene_tree_editor.cpp

doc/classes/TreeItem.xml

aaronfranke · 2024-12-12T00:14:16Z

In order to know what Nodes need to be updated we add a editor_state_changed signal to Node, this is a TOOLS_ENABLED, editor-only signal fired when changes to Node happen that are relevant to editor state.

I'm not a fan of even more editor-specific code in non-editor code. Why not add the signal onto the object using Object::add_user_signal? https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-add-user-signal

AeioMuch

For what it's worth, I've hammered the TPS Demo project's scenes trees with random changes, undid them, redid them, etc and haven't been able to break it.

hpvb · 2024-12-12T10:32:05Z

I'm not a fan of even more editor-specific code in non-editor code. Why not add the signal onto the object using Object::add_user_signal? https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-add-user-signal

After this pr we can remove several editor only signals as well, it if sadly not possible to add the signals at runtime as they get emitted from parts of node that just don't have signals. Like when a node gets added to a persistent group.

However the ultimate net effect is going to be less overall editor specific code in node.

WrobotGames · 2024-12-12T12:11:35Z

I tried this on my Jungle Demo scene and (compared to 4.4dev6) the time it takes for re-parenting a node is reduced from ~49 seconds to ~1 second. Amazing work! I, however, feels like the scene takes a bit longer to load in editor, (~10% longer).

hpvb · 2024-12-12T12:53:21Z

I tried this on my Jungle Demo scene and (compared to 4.4dev6) the time it takes for re-parenting a node is reduced from ~49 seconds to ~1 second. Amazing work! I, however, feels like the scene takes a bit longer to load in editor, (~10% longer).

If you would be willing to share your project with me in some fashion I can have a look at that also. But this would likely be addressed in a follow-up pr. You can find me on the rocket.chat with username hp if you'd like to discuss in private.

akien-mga · 2024-12-12T13:02:11Z

I think the two commits could be squashed together, there isn't much reason to implement the functionality in one commit and expose it in the next, we usually implement and expose at the same time.

akien-mga · 2024-12-12T13:05:43Z

editor/scene_tree_dock.cpp

+			if (Object::cast_to<Material>(resource)) {
+				// For performance reasons, assume that Materials don't have NodePaths in them.
+				// TODO This check could be removed when String performance has improved.
+				break;
+			}


Can't this use the PROPERTY_HINT_NO_NODEPATH hint you added instead?

No, the problem here is that getting the property list itself is super slow. By the time I get the hint it is already too late.

https://github.com/godotengine/godot/blob/master/scene/resources/material.cpp#L2404

This is the function that I need to avoid calling. There's a way to make it a bit faster too, but even then it is still pretty slow.

Now I realized this might break ViewportTextures in materials. They won't update paths anymore.

They still work, I forgot to put that as a test in my list of things I tested. But that was specifically tested.

hpvb · 2024-12-12T13:10:05Z

I think the two commits could be squashed together, there isn't much reason to implement the functionality in one commit and expose it in the next, we usually implement and expose at the same time.

I put it in two separate commits in case any backport to 4.3 wouldn't want to expose it. I can squash them though if that is not actually desired.

akien-mga · 2024-12-12T21:42:55Z

I think the two commits could be squashed together, there isn't much reason to implement the functionality in one commit and expose it in the next, we usually implement and expose at the same time.

I put it in two separate commits in case any backport to 4.3 wouldn't want to expose it. I can squash them though if that is not actually desired.

I'd say it's fine to squash. If we do a 4.3 backport, I think exposing a minor new functionality wouldn't be a big deal compared to the very significant refactoring of both editor code and core logic, which might be riskier than just a new function.

From a quick test it wouldn't be a simple git cherry-pick either but a proper backport, so if we do one we can also decide at that time to not expose the method (but really I don't think exposing new methods is a big deal - if it's there, we can as well offer it).

We now cache the Node*<>TreeItem* mapping in the SceneTreeEditor. This allows us to make targeted updates to the Tree used to display the scene tree in the editor. Previously on almost all changes to the scene tree the editor would rebuild the entire widget, causing a large number of deallocations an allocations. We now carefully manipulate the Tree widget in-situ saving a large number of these allocations. In order to know what Nodes need to be updated we add a editor_state_changed signal to Node, this is a TOOLS_ENABLED, editor-only signal fired when changes to Node happen that are relevant to editor state. We also now make sure that when nodes are moved/renamed we don't check expensive properties that cannot contain NodePaths. This saves a lot of time when SceneTreeDock renames a node in a scene with a lot of MeshInstances. This makes renaming nodes go from ~27 seconds to ~2 seconds on large scenes. SceneTreeEditor instances will now also not do all of the potentially expensive update work if they are invisible. This behavior is turned off by default so it won't affect existing users. This change allows the editor to only update SceneTreeEditors that actually in view. In practice this means that for most changes instead of updating 6 SceneTreeEditors we only update 1 instantly, and the others only when they become visible. There is definitely more that could be done, but this is already a massive improvement. In complex scenes we see an improvement of 10x, things that used to take ~30 seconds now only take 2. This fixes godotengine#83460 I want to thank KoBeWi, TokisanGames, a-johnston, aniel080400 for their tireless testing. And AeioMuch for their testing and providing a fix for the hover issue.

hpvb · 2024-12-12T21:47:53Z

From a quick test it wouldn't be a simple git cherry-pick either but a proper backport, so if we do one we can also decide at that time to not expose the method (but really I don't think exposing new methods is a big deal - if it's there, we can as well offer it).

Alright! Squashed.

hpvb · 2024-12-16T15:26:29Z

Can we has merge this so it can be tested in the next dev build prz? 👉👈

akien-mga

YOLO

akien-mga · 2024-12-16T16:16:03Z

Thanks!

hilloftheking · 2024-12-20T06:31:01Z

editor/gui/scene_tree_editor.cpp

+void SceneTreeEditor::set_marked(const HashSet<Node *> &p_marked, bool p_selectable, bool p_children_selectable) {
+	_update_if_clean();
+
+	_update_marking_list(marked);


I am getting a crash within this function occasionally when opening the signal connection dialog due to garbage data in marked. Is calling _update_marking_list() twice intended? Commenting out the first call seems to fix the issue, but I can't tell if that breaks anything.

Yes, calling it twice is intentional. Do you have a back trace of the crash?

There should never be "garbage data" in marked that is a pre-existing bug then, but I can't figure out how garbage data could get into it.

@hilloftheking can you try adding marked.clear() to https://github.com/godotengine/godot/blob/master/editor/gui/scene_tree_editor.cpp#L909 and see if this solves the issue?

Sorry, I'm away for a few days. I should have opened an issue for this but I couldn't make a decent reproduction. I think it has to do with the connection dialog keeping data from a closed scene if anyone else can make a reproduction project. If not, I will make one when I get back.

It does keep data from a closed scene until it reopens, this is by design because updating the invisible SceneTreeEditors is very costly. It makes everything take 6x as long! :)

I will see if I can come up with a fix for what I hypothesize is the problem.

Macksaur · 2024-12-21T06:34:45Z

editor/gui/scene_tree_editor.cpp

 	}

-	if (p_node->has_signal(SceneStringName(visibility_changed))) {
-		if (p_node->is_connected(SceneStringName(visibility_changed), callable_mp(this, &SceneTreeEditor::_node_visibility_changed))) {
-			p_node->disconnect(SceneStringName(visibility_changed), callable_mp(this, &SceneTreeEditor::_node_visibility_changed));


This signal is no longer disconnected. Is that okay? Is it part of the performance savings? Will it cause any issues in long running editor sessions?

This is by design, the cost of connecting and disconnecting signals here is pretty high, and leaving them connected doesn't really do anything. If it turns out to be a problem it can be changed back, but I have not observed any issues with this.

These signals do not fire often, and a signal that's not firing is just a bit of memory, and not a lot.

hpvb added topic:editor performance cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 26, 2024

hpvb requested review from a team as code owners November 26, 2024 01:39

arkology reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

AThousandShips added this to the 4.x milestone Nov 26, 2024

bruvzg reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.h Outdated Show resolved Hide resolved

scene/gui/tree.cpp Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch 2 times, most recently from 6aefc0b to 9a25be0 Compare November 26, 2024 12:03

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb requested a review from a team as a code owner November 26, 2024 12:20

hpvb force-pushed the scene_tree_editor_performance branch from 431094f to 05519d4 Compare November 26, 2024 12:29

hpvb requested review from arkology, bruvzg and KoBeWi November 26, 2024 12:32

arkology approved these changes Nov 26, 2024

View reviewed changes

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch from 05519d4 to d4d55d0 Compare November 26, 2024 13:20

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch 2 times, most recently from c5fca34 to e076f60 Compare November 26, 2024 16:15

This comment was marked as outdated.

Sign in to view

hpvb mentioned this pull request Nov 26, 2024

Manipulating the scene tree gets progressively slower with more nodes added to the tree in the editor. #83460

Closed

Mickeon reviewed Nov 26, 2024

View reviewed changes

doc/classes/TreeItem.xml Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch from e076f60 to 8851ae1 Compare November 26, 2024 17:18

hpvb force-pushed the scene_tree_editor_performance branch from 47465d3 to 987fdd4 Compare December 11, 2024 19:30

AeioMuch approved these changes Dec 12, 2024

View reviewed changes

akien-mga added enhancement bug and removed enhancement labels Dec 12, 2024

akien-mga reviewed Dec 12, 2024

View reviewed changes

hpvb force-pushed the scene_tree_editor_performance branch from 987fdd4 to 6f7525c Compare December 12, 2024 21:47

KoBeWi mentioned this pull request Dec 15, 2024

Improve Scene Tree editor performance #100424

Closed

akien-mga approved these changes Dec 16, 2024

View reviewed changes

akien-mga merged commit 08508d2 into godotengine:master Dec 16, 2024
20 checks passed

hilloftheking reviewed Dec 20, 2024

View reviewed changes

akien-mga mentioned this pull request Dec 20, 2024

[4.4 dev 7] Crash to Desktop on choosing skeleton node #100666

Closed

Macksaur reviewed Dec 21, 2024

View reviewed changes

KoBeWi mentioned this pull request Dec 23, 2024

Fix clear_custom_color on invalid tree_item_inspected pointer #99122

Closed

timoschwarzer mentioned this pull request Dec 26, 2024

Scene Tree Editor cache becomes invalid and crashes #100812

Closed

This was referenced Dec 27, 2024

Attempting to populate MultiMeshInstance3D surface in version 4.4.dev7 crashes the engine #100840

Closed

[v4.4dev7] Godot crash when selecting Skeleton property on MeshInstance3D #100924

Closed

matheusmdx mentioned this pull request Jan 3, 2025

Adding a MultiMeshInstance3D node to a Node3D node, clicking the in-editor MultiMesh button, and selecting Target Surface button, crashes Godot 4.4.dev7. #101067

Closed

yahkr mentioned this pull request Jan 5, 2025

Add the ability to expose nodes for direct access in instantiated scenes #84018

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Scene Tree editor performance #99700

Improve Scene Tree editor performance #99700

hpvb commented Nov 26, 2024 •

edited

Loading

hpvb commented Nov 26, 2024 •

edited

Loading

arkology left a comment

arkology left a comment •

edited

Loading

KoBeWi commented Nov 26, 2024

This comment was marked as outdated.

aaronfranke commented Dec 12, 2024

AeioMuch left a comment

hpvb commented Dec 12, 2024

WrobotGames commented Dec 12, 2024

hpvb commented Dec 12, 2024

akien-mga commented Dec 12, 2024

akien-mga Dec 12, 2024

hpvb Dec 12, 2024

hpvb Dec 12, 2024

KoBeWi Dec 12, 2024

hpvb Dec 12, 2024

hpvb commented Dec 12, 2024

akien-mga commented Dec 12, 2024

hpvb commented Dec 12, 2024

hpvb commented Dec 16, 2024

akien-mga left a comment

akien-mga commented Dec 16, 2024

hilloftheking Dec 20, 2024

hpvb Dec 20, 2024

hpvb Dec 20, 2024

hpvb Dec 20, 2024

hilloftheking Dec 21, 2024

hpvb Dec 21, 2024

Macksaur Dec 21, 2024

hpvb Dec 21, 2024

hpvb Dec 21, 2024

Improve Scene Tree editor performance #99700

Improve Scene Tree editor performance #99700

Conversation

hpvb commented Nov 26, 2024 • edited Loading

hpvb commented Nov 26, 2024 • edited Loading

arkology left a comment

Choose a reason for hiding this comment

arkology left a comment • edited Loading

Choose a reason for hiding this comment

KoBeWi commented Nov 26, 2024

This comment was marked as outdated.

aaronfranke commented Dec 12, 2024

AeioMuch left a comment

Choose a reason for hiding this comment

hpvb commented Dec 12, 2024

WrobotGames commented Dec 12, 2024

hpvb commented Dec 12, 2024

akien-mga commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpvb commented Dec 12, 2024

akien-mga commented Dec 12, 2024

hpvb commented Dec 12, 2024

hpvb commented Dec 16, 2024

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpvb commented Nov 26, 2024 •

edited

Loading

hpvb commented Nov 26, 2024 •

edited

Loading

arkology left a comment •

edited

Loading