Skip to content
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.

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

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

Open
viksl opened this issue Oct 16, 2023 · 19 comments · May be fixed by #99700
Open

Comments

@viksl
Copy link
Contributor

viksl commented Oct 16, 2023

Godot version

4.2 beta 1

System information

Windows 11 - Vulkan - Nvidia RTX 4070 - intel i5 13600KF

Issue description

The more nodes there are in the tree the slower working with the tree gets. I don't even have that many and moving one node in my tree in my game inside editor takes seconds (editor freezes for several seconds until it moves the node in the tree (not even moving it in space).

Link video: https://youtu.be/SAilvoE_ACc
(check how I release the mouse and move it away and how long it takes to make nodes move)

Use the MRP, there are two scenes, main and test_no_mesh_instances, feel free to use either just understand the main has an extreme amount of mesh instances in one places, the latter uses only Node3D hiearchy so it should be a lot less intensive on HW.
I've also purposely added a lot of nodes just to make the issue clear, in my game I don't have even nearly as many nodes but the freeze times are even longer than in MRP.

Please note my HW specs, it's not the highest end but certainly no slug either.

Steps to reproduce

Steps:

  1. Open the MRP
  2. Open one of the scenes
  3. Add a Node3D, Camera3D, WorldENvironment or any node into the sceen tree
  4. Move the tree from last position to the very top of the tree for example, then move it elsewhere if you want
  5. Observer the editor freezing as it's moving your node in the sceen tree.
    (6. if you have Frame Time open it will show yo ua spike for GPUT TIme and maybe even CPU Time in red after it moves the nodei n the tree)

Minimal reproduction project

ManyNodesSlowTreeMRP.zip

EDIT: Not just moving but also simply deleting or renaming a node takes forever too

EDIT2: This has been discussed in the dev chat (rocket chat) and the issue is due to how the referencing works. When you do any of the operations mentioned above the Editor will loop through all nodes recursively (meaning all their children and children of children and so on) and loops through all their properties which has some expensive operations (casting StringName or something like that and such) which eventually starts to take a lot of time. In a more advanced test the freeze took up to 17 seconds on the above mentioned CPU which has frequency up to 5.1 GHz wtih RAM 6000 MHz so not a particularly slow machine to begin with), currently couple suggestions were mentioned but were brought down due to potentiallyy causing issues/slowdowns in other areas so it seems no on right now knows how to solve this if I understood it correctly?

Since it's recursive it doesn't matter if you split your scene into multiple scenes and save the mseparately and then put together, it will still cause the exactly same issue.

This issue was also c++ profiled to navigate to problematic areas and check the timings as mentioned above).

@bitsawer
Copy link
Member

There might be some performance issues either in the Tree control itself or in the editor scene tree implementation when using a lot of nodes / tree items. Some was improved in #79325, but as the discussion there shows there might still be some performance issues left.

Testing with earlier Godot 4.0, 4.1 and maybe some 4.2 dev releases might give a better idea if this is a recent regression or an existing issue.

@Calinou

This comment was marked as outdated.

@YuriSizov
Copy link
Contributor

I think this is an independent issue, or at least worth an investigation. #84910 was opened afterwards, which I believe is very much relevant here.

Let's give this a test and try to pin the cause. I believe someone on the chat identified that similarly to #84910 this is about updating various references in nodes and resources. So the bigger the tree, the more we need to iterate through.

Some sort of cache for relevant nodes/resources would be a good idea in that case.

@viksl
Copy link
Contributor Author

viksl commented Nov 28, 2023

The discussion started here: https://chat.godotengine.org/channel/editor?msg=hH42LhRaxLm6cLFK2 (do these links work? It's kind of 50/50 for me :D).
It was also confirmed by another user in dev chat new-contributors when we were profiling what's happening.

@ShadyShlowpoke
Copy link

This is also an issue for me on 4.3-beta1. Seems like #85502 did not fix it.

In my own project I have many instanced scenes, but in testing I experienced increasing slowdown manipulating the scene tree with simply creating thousands of Node2Ds with a tool script.

I also went back and did the same thing on 3.5 and it was an issue there as well.

@viksl
Copy link
Contributor Author

viksl commented Nov 10, 2024

@AThousandShips Hi, can we please reopen this report. The PR does not solve this issue.
This is not related to drawing the tree but iterating over all nodes and their properties (recursively) and loads of type casting which slows the processing down a lot.
I've tested the PR and it makes no difference to this issue.

During testing I noticed a new issue or perhaps this issue again (I'm not sure, should I open a new bug report for this: https://youtu.be/zv4fFdvRSf0 - when selecting and unselecting a lot of nodes Godot pretty much freezes and takes very long time as you can see in the video until it selects the nodes, it might be relevant to this issue but I'm not sure)?

@AThousandShips
Copy link
Member

I didn't link this but I'll reopen it

@AThousandShips AThousandShips removed this from the 4.4 milestone Nov 10, 2024
@TokisanGames
Copy link
Contributor

TokisanGames commented Nov 21, 2024

I also find the scene tree to be extremely slow in the editor when there are thousands of nodes. In one large scene there are only a thousand or so nodes, but including subscenes, 62,000 nodes. I just spent 5 hours separating a large scene into several smaller ones and half of that time was wasted waiting for Godot to add, rename, move, or delete nodes. I had to spend this time to separate out these nodes because my environment artist couldn't work due to it being so slow. The scene tree is basically unusable past a certain point. I've heard the same complaint from people on my discord, so it's not just our project.

The issue for me is neither CPU nor GPU bound, and independent of Terrain3D. I just renamed a node in a large scene w/o Terrain3D and Godot hung. In Taskmanager it reported 1-3% CPU not even maxing out one core, and GPU was nominal. Godot was non-responsive until it finally renamed the node. I disabled all plugins, but it didn't help.

4.3-stable, 4.4dev4.

Edit: Just tried 4.4dev5, which includes #96841 and it's no better. I think it's worse.

@viksl
Copy link
Contributor Author

viksl commented Nov 21, 2024

@TokisanGames the issue isn't really that it's performance intensive so you won't see much on any metrics in this regard, you can use a cpp profiler to find where it hangs, we did it with people in dev chat. The issue is that there're tons of checks and type casting which incredibly slows everything down since it takes a while but the operations themselves are not performance hogs so you just wait. Basically every property you have it checked, every animation is checked and it goes recursively to make sure there are no links left or something along those lines which means with 62k nodes and like 50 properties on a node on average you have a lot ot checks and type casts (StringName was the biggest hog from type casts if I remember correctly).
You can better discuss it with people in the Editor section in the dev chat https://chat.godotengine.org/channel/editor maybe.

@TokisanGames
Copy link
Contributor

TokisanGames commented Nov 21, 2024

I was asked to run a profiler on dev chat, and got this file from very sleepy: capture.zip. This is from renaming a node in a scene.

@atirut-w
Copy link
Contributor

Basically every property you have it checked, every animation is checked and it goes recursively to make sure there are no links left or something along those lines which means with 62k nodes and like 50 properties on a node on average you have a lot ot checks and type casts (StringName was the biggest hog from type casts if I remember correctly).

This sounds like a fundamental problem with how the engine does stuff.

@hpvb
Copy link
Member

hpvb commented Nov 22, 2024

I did some targeted profiling on just the steps suggested in the MRP and I found that 98% of the time is spend in free() during the pause.

I'll be looking into it further. But I wanted to share my finding so far.

@viksl
Copy link
Contributor Author

viksl commented Nov 22, 2024

@hpvb
Free as if freeing nodes? Is that also relevant to Renaming, moving around, ... operations, please?

@hpvb
Copy link
Member

hpvb commented Nov 22, 2024

There are a couple of things kinda slow, I'm working on a solution. I'm implementing a skiplist in the Tree data structure in the editor to make create_child() O(log(n)) instead of O(n), that will help with moving and creating nodes.

For the renaming nodes I will have to have another look. But the different operations are slow for different reasons. The 97% time spent in free() was for a renaming operation.

@viksl
Copy link
Contributor Author

viksl commented Nov 24, 2024

@hpvb Thank you for looking into this! If you don't mind, I don't know if you noticed but in the second half of my reply here I noticed another thing which might be something else but I just wanted point it out to keep it together (and if it's a different issue eventually I'd report it separately): #83460 (comment)

One more thing you don't mind, are these operations we talk about in the tree handled on a single thread or parallel, I could imagine when it comes to checks against the entire tree that parallelization could also improve the performance perhaps, though I haven't dove so deep into the sceen tree so I don't really know how this processing is handled?

hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
@hpvb hpvb linked a pull request Nov 26, 2024 that will close this issue
hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 26, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
@hpvb
Copy link
Member

hpvb commented Nov 26, 2024

@viksl The selection problem isn't fixed yet in #99700, that will have to be a separate commit as that needs to touch different code. What is currently improved is:

  • Creating nodes
  • Moving/re-parenting nodes
  • Deleting nodes
  • Renaming nodes

Stuff that is still slow

  • Selecting a large number of nodes
  • Filtering nodes by name or type

Those require a different approach

@hpvb
Copy link
Member

hpvb commented Nov 26, 2024

Basically every property you have it checked, every animation is checked and it goes recursively to make sure there are no links left or something along those lines which means with 62k nodes and like 50 properties on a node on average you have a lot ot checks and type casts (StringName was the biggest hog from type casts if I remember correctly).

This sounds like a fundamental problem with how the engine does stuff.

This is/was not the problem, and this is not how it works. What happened was that on each change to the scene tree the SceneTreeEditor would re-create the GUI elements that represent the tree. There is no such checking of links and whatnot happening. This was purely an editor inefficiency, not some hidden design disaster as suggested here.

@viksl
Copy link
Contributor Author

viksl commented Nov 26, 2024

Basically every property you have it checked, every animation is checked and it goes recursively to make sure there are no links left or something along those lines which means with 62k nodes and like 50 properties on a node on average you have a lot ot checks and type casts (StringName was the biggest hog from type casts if I remember correctly).

This sounds like a fundamental problem with how the engine does stuff.

This is/was not the problem, and this is not how it works. What happened was that on each change to the scene tree the SceneTreeEditor would re-create the GUI elements that represent the tree. There is no such checking of links and whatnot happening. This was purely an editor inefficiency, not some hidden design disaster as suggested here.

I see, cool, this was what I was told in the editor dev channel, I don't really know the details but I'm glad you checked this issue out and worked on it, thank you very much!

@TokisanGames
Copy link
Contributor

What is currently improved is:
Creating nodes
Moving/re-parenting nodes
Deleting nodes
Renaming nodes

Thanks for working on this. These basics are the urgent, critical needs. I would love to see this merged in and cherry picked for 4.3.1.

Stuff that is still slow
Selecting a large number of nodes
Filtering nodes by name or type

These can come later as far as I'm concerned.

I will test your PR in my project soon, and comment there.

hpvb added a commit to hpvb/godot that referenced this issue Nov 27, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 27, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 28, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 28, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 28, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 29, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 29, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
hpvb added a commit to hpvb/godot that referenced this issue Nov 29, 2024
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.

There is definitely more that could be done, but this is already a
massive improvement.

This fixes godotengine#83460
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants