Avoid unnecessary inspector updates when loading or switching scenes

[YuriSizov]

Fixes #80126. Maybe some other errors too — related to plugins, inspector updates, and editor startup. For example, #73650.

So the issue seems like it has something to do with plugins, but it's not. The core of the problem lies in the fact that we like to excessively trigger update_tree on the inspector, which can result in multiple calls to edit the same object from different contexts in the same frame. Or result in requests for editing from scenes which are going to be replaced during the loading procedure. Overall I identified the following cases related to the issue:

1. When we load the editor, we try to restore some scenes. When we restore those scenes, we call load_scene. Each such call results in the loaded scene to be passed to the rest of the editor as the currently edited object... But in the very next second we remove the scene to load the next one. And set the next one as the currently edited object. And so on. When we're done with this, we finish the whole thing off with using the stored scene index to set it as the currently edited object AGAIN.

This sucks. Every attempt to edit anything builds the tree in the inspector, including subtrees if some properties are resources and are unfolded. This triggers a whole bunch of things. Only for the scene to be discarded immediately, so all is done for naught. At the same time if the last opened scene is the one that should be the currently edited scene, it is marked for editing twice. At best, this is very slow, unnecessarily so. But at worse, this leads to various bugs such as #80126.

You see, we call some methods with deferred calls. And when we open a scene with the root being Control node that happens to have a theme (quite a normal thing to have), we register one such deferred call. If this scene happens to be the last one and we mark it for editing again, then we make the second deferred call. So the next time the message queue is flushed we make two calls to add an editor for the same resource, resulting in the logged error.

To reiterate what happens, let's consider an example of two restored scenes where on has a control with a theme as its root. In our case this is the last scene on the list and it's also the scene that was last edited before shutting down the editor. The following happens:

- Load Scene 1
  - Put its nodes into the scene tree of the editor.
- Edit Scene 1
  - Build the inspector and update various docks.
- Remove Scene 1 from the scene tree
- Set the edited object to null.
- Load Scene 2
  - Put its nodes into the scene tree of the editor.
- Edit Scene 2
  - Build the inspector and update various docks.
  - Handle its theme and request to add a plugin.
- Remove Scene 2 from the scene tree
- Set the edited object to null.
- Set Scene 2 as currently edited
  - Build the inspector and update various docks.
  - Handle its theme and request to add a plugin.
- Add the theme editor plugin.
- Add the theme editor plugin.
  - This fails, we already have a plugin.

If Scene 2 is not the last edited scene, then there won't be an error, but curiously the theme editor for its embedded theme would still be open upon loading. Despite a different scene, Scene 1, being currently active.

I resolved this by guarding the push_item call in load_scene when we are in the process of restoring scenes. This should work fine. Maybe there is some weird case if we have scenes to restore, but don't have the last edited scene stored, but I don't think this is possible without user intervention and at this point the worst that should happen is no scene being edited. A couple of clicks should solve that.

2. Due to previous hacks, such as Fixed Inspector update when a node is renamed #35526, and fixes, such as Fix path typo for editor def capitalize_properties #19568, it was possible for the update_tree call to be triggered multiple times within the same setup process. This once again leads to conflicting logic and poor performance. I think one of the cases where I was able to reproduce the "plugin already exists" error unrelated to loading can be attributed to this.

The case was this: run any scene, switch to the remote view and select any node, then switch back to the edited scene with a control node that has a theme and select it. Boom, you have the same error logged.

#35526 is particularly suspicious, as I'm not entirely sure why rebuilding the tree would somehow restore the folded status that is supposedly broken when you rename a node. I was unable to reproduce this with the hack removed. The PR is old, so probably some 3.x weirdness, or maybe the solution wasn't appropriate even back then, can't really say for sure at this point.

#19568 does more than the title says. One thing that it adds is a call to set_use_folding to update that setting according to the editor configuration. Setting aside if this is the best way to do it, this call comes with an extra side effect: it also rebuilds the tree. The tree that we, for sure, just updated during the previous steps of EditorNode::_edit_current. Solution here is simple, add a flag to disable the tree update (so other use cases are unaffected) and make sure to move the call to before we build the tree.

---

While investigating I noticed that EditorNode::edit_item can be made a bit more flat and clear, so I updated it. The logic should be identical to before, except for one extra call to hide_unused_editors(p_editing_owner); that I think we were missing if the edited object is disabled by a feature profile.

I also renamed some related methods of EditorData, as their existing names were making it a bit confusing to read the code. These are internal, so it shouldn't lead to any issues. The names may not be ideal, but they should be clearer than before at least.

Finally, I noticed a bunch of calls like EditorNode::get_singleton()->get_editor_data() in related code, so I did a pass that simplifies it to EditorNode::get_editor_data().

[YuriSizov]

I just tested it with hyperfine to see if there are measurable startup improvements when restoring scenes, and sure as day, there are:

Command that was used

hyperfine -r 10 -i ".\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0" ".\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0"

Test 1. 2 open scenes

Benchmark 1: .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):      9.927 s ±  0.376 s    [User: 5.944 s, System: 0.239 s]
  Range (min … max):    9.634 s … 10.947 s    10 runs

Benchmark 2: .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     10.408 s ±  0.109 s    [User: 6.453 s, System: 0.247 s]
  Range (min … max):   10.284 s … 10.692 s    10 runs

Summary
  .\new-patch\... ran
    1.05 ± 0.04 times faster than .\old-patch\... 

Test 2. 2 open scenes

Benchmark 1: .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     10.273 s ±  0.467 s    [User: 6.011 s, System: 0.290 s]
  Range (min … max):    9.760 s … 11.044 s    10 runs

Benchmark 2: .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     10.796 s ±  0.214 s    [User: 6.686 s, System: 0.292 s]
  Range (min … max):   10.609 s … 11.175 s    10 runs

Summary
  .\new-patch\... ran
    1.05 ± 0.05 times faster than .\old-patch\...

Test 3. 9 open scenes

Benchmark 1: .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     10.201 s ±  0.342 s    [User: 6.033 s, System: 0.297 s]
  Range (min … max):    9.956 s … 11.100 s    10 runs

Benchmark 2: .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     12.500 s ±  0.200 s    [User: 7.514 s, System: 0.283 s]
  Range (min … max):   12.166 s … 12.979 s    10 runs

Summary
  .\new-patch\... ran
    1.23 ± 0.05 times faster than .\old-patch\...

Test 4. 9 open scenes

Benchmark 1: .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     10.432 s ±  0.452 s    [User: 6.225 s, System: 0.272 s]
  Range (min … max):    9.932 s … 11.347 s    10 runs

Benchmark 2: .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     12.704 s ±  0.416 s    [User: 7.781 s, System: 0.301 s]
  Range (min … max):   12.204 s … 13.602 s    10 runs

Summary
  .\new-patch\... ran
    1.22 ± 0.07 times faster than .\old-patch\...

Tests 1&2 and 3&4 used the same setup for each pair and are used to double-check the results. All runs were performed on unoptimized dev builds (so they are generally slower than what you'd expected from official builds).

From these you can see that load times are now consistent no matter how many scenes you have opened by default.

[KoBeWi]

The changes look good. I found a couple more unnecessary updates (in the main inspector):

There's a bunch of methods here calling tree update:

godot/editor/inspector_dock.cpp

Lines 762 to 773 in 4714e95

	inspector = memnew(EditorInspector);
	add_child(inspector);
	inspector->set_autoclear(true);
	inspector->set_show_categories(true);
	inspector->set_v_size_flags(Control::SIZE_EXPAND_FILL);
	inspector->set_use_doc_hints(true);
	inspector->set_hide_script(false);
	inspector->set_hide_metadata(false);
	inspector->set_use_settings_name_style(false);
	inspector->set_property_name_style(property_name_style);
	inspector->set_use_folding(!bool(EDITOR_GET("interface/inspector/disable_folding")));
	inspector->register_text_enter(search);

This seems to assign default profile, triggering inspector update:

godot/editor/editor_node.cpp

Line 677 in 4714e95

feature_profile_manager->notify_changed();

Maybe we could add a flag to the inspector, like surpress_updates, which can be set externally when the inspector is not supposed to update, e.g. when initializing its properties.

[KoBeWi]

This is alright in the current form (except that one style comment). It also fixes some issues related to weird TileMap editor behavior on launch.

Further improvements can be done later.

[YuriSizov]

Maybe we could add a flag to the inspector, like surpress_updates, which can be set externally when the inspector is not supposed to update, e.g. when initializing its properties.

Yeah, I agree. If there are many setters that can trigger it, it would be better to have a global suppression flag rather than pass one to each setter. If implemented, the new argument added to set_use_folding can be removed.

[adamscott]

Great job, @YuriSizov. Your PR is very thorough, detailed and well explained.

[akien-mga]

Thanks!