Fix scene reload causing segfaults when non-existent node is currently selected

[nikitalita]

Errors in the way we were handling deselecting nodes were causing segfaults when the currently edited scene was reloaded.

When reloading a scene, the currently selected node may not exist. This is supposed to be handled by EditorSelection::update(), which emits a signal that calls SceneTreeDock::_selection_changed(), which calls SceneTreeDock::_push_item, which calls EditorNode::get_singleton()->push_node_item.

However, the problem is thus:

SceneTreeDock::_push_item always calls push_node_item for nullptr objects, but this ends up being useless, because EditorNode::push_node_item was checking to see if the currently edited object in the inspector was a Node; if the inspector object is null, then it won't be a node, so nothing happens, and the NodeDock and associated docks don't get their selection updated.

As a result, interacting with any of the NodeDocks besides the SceneTreeDock after a scene reload with a removed node selected can cause a crash. The easiest way to cause this is by interacting with the Node menu and double clicking on one of the signals.

Checking if the currently edited node in the inspector is null and calling push_item if so fixes the issue.

[KoBeWi]

How can you select a removed node?

[nikitalita]

If you have the node selected, you reload the scene from disk, and the scene no longer contains that node.

[KoBeWi]

And what do you mean by "reload from disk"?
Both using Reload Saved Scene option and reloading after external change properly updates the selection.

[nikitalita]

In our particular use case, which is a version control plugin, we have to manually call reload_scene when checking out a new branch or updating from remote to get the currently edited scene to update in the editor, since the editor will likely still be in focus when the files are updated and it won't reload the scene (unless the editor is re-focused and prompts the user, which is a rather bad user experience). It's in this particular context where we're seeing this.

In either case, do you have an issue with the logic here? if the node currently being inspected in the Inspector dock is null, and the item being pushed with push_node_item is also null, we should push that item so that the selection gets cleared; is that in question?

[KoBeWi]

When reviewing bugfixes, I always try to reproduce the bug first and then verify if it's really fixed. It helps to better understand the problem. I didn't even look at the code yet.

I tried calling reload_scene() manually (using EditorScript) and still can't reproduce the crash.

[nikitalita]

Understood; I think we may be hitting some sort of a race condition that doesn't really appear in normal use (perhaps, in normal use, the inspector dock gets updated after the editor selection gets updated for the node docks?).

You could try this: Select a node, and then select a resource so that the inspector tab gets pointed to a non-node item, then delete that node outside of the editor, reload the scene

[Repiteo]

Thanks!