Overhaul the SurfaceUpgradeTool

[clayjohn]

Tentative fix for #83991

I am not sure if this fixes the issue or not as I can't reproduce the issue on any of the provided MRPs.

Background

My theory about what was going wrong is that the upgrade tool was running on one thread while scenes were being loaded in the background on other threads. So resources were being reimported while the inheriting scene was being loaded leading to paths getting broken.

Proposed solution

This PR defers the actual update to a new run of the engine and it disconnects the re-import of resources from the re-saving of scenes and resources.

To do this it deletes the imported meshes/scenes from the .import folder, then immediately restarts the editor. Following the restart, it waits for the re-import to finish then it closes all open scenes and then opens and saves all scenes + non-importable resources (e.g. meshes saves as .mesh or .res).

This should essentially do the same things as the old version, except avoid running at the same time as regular loading.

Open Questions

I am not sure how loading works and I am not that familiar with the editor. So I am assuming that by closing all scenes, it is guaranteed that all loading operations will be stopped.

I am not confident that checking get_file_type() for Mesh, ArrayMesh, and PackedScene on all files is sufficient to catch all files that are meshes or scenes. I think other meshes are slipping through the cracks, but I am unfamiliar with editor stuff, so I am not sure if there is something better to check.

[Lippanon]

Windows 10.

I tried this PR with a project last ran on v4.2.dev.custom_build [f7bc653].
The upgrade popup showed up at some point during the importing of assets (I deleted the .godot folder before opening the project), I selected the option to upgrade and save. There were many errors in console followed by this:

I selected 'OK'. The load bar continued and when it reached the end Godot became unresponsive forcing me to terminate it. Reopening the project crashes on startup:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.beta.custom_build (d036f8aa9a5bb0aae1f9e908819e91f8ff46c3a4)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] <couldn't map PC to fn name>
[1] <couldn't map PC to fn name>
[2] Node::_propagate_enter_tree (X\scene\main\node.cpp:261)
[3] Node::_propagate_enter_tree (X\scene\main\node.cpp:274)
[4] Node::_propagate_enter_tree (X\scene\main\node.cpp:274)
[5] Node::_set_tree (X\scene\main\node.cpp:2937)
[6] OS_Windows::run (X\platform\windows\os_windows.cpp:1470)
[7] widechar_main (X\platform\windows\godot_windows.cpp:182)
[8] _main (X\platform\windows\godot_windows.cpp:206)
[9] main (X\platform\windows\godot_windows.cpp:218)
[10] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[11] <couldn't map PC to fn name>
-- END OF BACKTRACE --
================================================================

I understand it's likely not much help, but I can help test subsequent changes. Thanks.

[YuriSizov]

I tried to work on a patch to improve the logic a bit and prevent conflicts while loading, importing, and upgrading. I don't have many projects to test this on, so I used one of GDQuest demos: https://github.com/gdquest-demos/godot-4-3d-third-person-controller.

I cloned the project, opened it in Godot 4.1.2 to make sure it fully imports, made a copy of .godot and then proceeded to test.

The master branch version right now doesn't load the project at all for me. It just locks on a mutex, seemingly related to GDScriptCache, but you can notice that in another thread the surface upgrade tool's dialog is blocking execution. The issue is that it happens way before the editor is ready.

With this PR this issue is gone already, so that's good. I managed to load the project, and it predictably complains about outdated meshes. Clicking on the "Resave" button in the dialog exposes one issue: if in that moment one of the scenes is being loaded, we get a file lock.

This is because the tool is trying to read the .import file and to delete the imported mesh, while another part of the editor is trying to read those files to load the scene. Ignoring this issue makes the editor to proceed with the restart, but the locked file will not be converted at the next step.

After the reload the conversion works as expected, and the project becomes usable (aside from the importer issues addressed by #84034). However, as I outlined the process currently depends on the resources_reimported signal. Which is fine for this particular project, but will make the tool useless in a project that has only internal meshes and no imported meshes. As unlikely as it is, this is worth handling.

There is also the problem reported above, where reimporting can cause conflicts with the tool. It is similar to my first issue outlined above, but not exactly the same. I tried to address all of them though.

The changes are as follows:

• Avoid deleting imported files during the pre-step, before the reload. Instead, track them and handle the deletion after the reload. We have to track them in the pre-step because the EditorFileSystem is not going to be available early enough for us to rely on it after the reload. This should prevent any conflicts with both loading scenes and reimporting the scenes, though I can't reliably test the second case.
  • Some of these changes required making the tool into a proper singleton and making properties and methods instance-bound.
• Move the upgrade step from the resources_imported handler to the moment before we do the first scan by EditorFileSystem. The scan is what both populates the Editor FS (which is why we track files from the pre-step) and triggers the re-imports. We want to do our deed before that happens. To ensure this I'm also adding a signal and only trigger the scan after the upgrade tool has finished.
  • During the upgrade step we now also delete the imported files. This means that we will always delete all know files before the editor can check them, which should successfully trigger the re-import.
• Due to the EditorProgress implementation issues which forbid it from running during the flushing of the message queue or during deferred calls I had to move the whole first scane/upgrade tool step into the notification handler. This doesn't appear to cause any noticeable issues (and I don't expect it should).

Here's the code that I arrived to. This diff should apply cleanly if you rebase this PR against the master (93cdacb).

diff --git a/editor/editor_node.cpp b/editor/editor_node.cpp
index e3301f5272..a0da494b5a 100644
--- a/editor/editor_node.cpp
+++ b/editor/editor_node.cpp
@@ -623,6 +623,20 @@ void EditorNode::_notification(int p_what) {
 			ResourceImporterTexture::get_singleton()->update_imports();
 
 			bottom_panel_updating = false;
+
+			if (requested_first_scan) {
+				requested_first_scan = false;
+
+				OS::get_singleton()->benchmark_begin_measure("editor_scan_and_import");
+
+				if (run_surface_upgrade_tool) {
+					run_surface_upgrade_tool = false;
+					SurfaceUpgradeTool::get_singleton()->connect("upgrade_finished", callable_mp(EditorFileSystem::get_singleton(), &EditorFileSystem::scan), CONNECT_ONE_SHOT);
+					SurfaceUpgradeTool::get_singleton()->finish_upgrade();
+				} else {
+					EditorFileSystem::get_singleton()->scan();
+				}
+			}
 		} break;
 
 		case NOTIFICATION_ENTER_TREE: {
@@ -1015,11 +1029,6 @@ void EditorNode::_resources_reimported(const Vector<String> &p_resources) {
 	}
 
 	scene_tabs->set_current_tab(current_tab);
-
-	if (run_surface_upgrade_tool) {
-		SurfaceUpgradeTool::finish_upgrade();
-		run_surface_upgrade_tool = false;
-	}
 }
 
 void EditorNode::_sources_changed(bool p_exist) {
@@ -4622,8 +4631,10 @@ void EditorNode::_editor_file_dialog_unregister(EditorFileDialog *p_dialog) {
 Vector<EditorNodeInitCallback> EditorNode::_init_callbacks;
 
 void EditorNode::_begin_first_scan() {
-	OS::get_singleton()->benchmark_begin_measure("editor_scan_and_import");
-	EditorFileSystem::get_singleton()->scan();
+	if (!waiting_for_first_scan) {
+		return;
+	}
+	requested_first_scan = true;
 }
 
 Error EditorNode::export_preset(const String &p_preset, const String &p_path, bool p_debug, bool p_pack_only) {
@@ -8062,7 +8073,7 @@ EditorNode::EditorNode() {
 	surface_upgrade_tool = memnew(SurfaceUpgradeTool);
 	run_surface_upgrade_tool = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "run_on_restart", false);
 	if (run_surface_upgrade_tool) {
-		SurfaceUpgradeTool::begin_upgrade();
+		SurfaceUpgradeTool::get_singleton()->begin_upgrade();
 	}
 }
 
diff --git a/editor/editor_node.h b/editor/editor_node.h
index 35b4dad78f..fe99cf1f01 100644
--- a/editor/editor_node.h
+++ b/editor/editor_node.h
@@ -460,6 +460,8 @@ private:
 	bool opening_prev = false;
 	bool restoring_scenes = false;
 	bool unsaved_cache = true;
+
+	bool requested_first_scan = false;
 	bool waiting_for_first_scan = true;
 
 	int current_menu_option = 0;
diff --git a/editor/surface_upgrade_tool.cpp b/editor/surface_upgrade_tool.cpp
index f8c74a87d0..58b19ef9a5 100644
--- a/editor/surface_upgrade_tool.cpp
+++ b/editor/surface_upgrade_tool.cpp
@@ -36,7 +36,6 @@
 #include "servers/rendering_server.h"
 
 SurfaceUpgradeTool *SurfaceUpgradeTool::singleton = nullptr;
-bool SurfaceUpgradeTool::popped_up = false;
 
 void SurfaceUpgradeTool::_add_files(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths) {
 	for (int i = 0; i < p_dir->get_subdir_count(); i++) {
@@ -54,7 +53,7 @@ void SurfaceUpgradeTool::_add_files(EditorFileSystemDirectory *p_dir, HashSet<St
 	}
 }
 
-void SurfaceUpgradeTool::_get_import_paths(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths) {
+void SurfaceUpgradeTool::_get_import_paths(EditorFileSystemDirectory *p_dir, Vector<String> &r_paths) {
 	for (int i = 0; i < p_dir->get_subdir_count(); i++) {
 		_get_import_paths(p_dir->get_subdir(i), r_paths);
 	}
@@ -64,25 +63,27 @@ void SurfaceUpgradeTool::_get_import_paths(EditorFileSystemDirectory *p_dir, Has
 				p_dir->get_file_type(i) == "ArrayMesh" ||
 				p_dir->get_file_type(i) == "PackedScene") {
 			if (FileAccess::exists(p_dir->get_file_path(i) + ".import")) {
-				Ref<ConfigFile> config;
-				config.instantiate();
-				Error err = config->load(p_dir->get_file_path(i) + ".import");
-				if (err != OK) {
-					ERR_PRINT("Could not open " + p_dir->get_file_path(i) + "for upgrade.");
-					continue;
-				}
-
-				String path = config->get_value("remap", "path", "");
-				if (!path.is_empty()) {
-					r_paths.insert(path);
-				}
+				r_paths.append(p_dir->get_file_path(i) + ".import");
 			}
 		}
 	}
 }
 
+void SurfaceUpgradeTool::_try_show_popup() {
+	if (singleton->show_requested) {
+		return;
+	}
+	singleton->show_requested = true;
+
+	if (EditorFileSystem::get_singleton()->is_importing()) {
+		EditorFileSystem::get_singleton()->connect("resources_reimported", callable_mp(singleton, &SurfaceUpgradeTool::_show_popup), CONNECT_ONE_SHOT);
+	} else {
+		singleton->_show_popup();
+	}
+}
+
 void SurfaceUpgradeTool::_show_popup() {
-	MutexLock lock(singleton->mutex);
+	MutexLock lock(mutex);
 
 	if (popped_up) {
 		return;
@@ -95,17 +96,9 @@ void SurfaceUpgradeTool::_show_popup() {
 	if (accepted) {
 		EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "run_on_restart", true);
 
-		// Remove the imported scenes/meshes from .import so they will be reimported automatically on the next open.
-		HashSet<String> paths;
+		Vector<String> paths;
 		_get_import_paths(EditorFileSystem::get_singleton()->get_filesystem(), paths);
-		for (const String &file_path : paths) {
-			String path = OS::get_singleton()->get_resource_dir() + file_path.replace_first("res://", "/");
-			print_verbose("Moving to trash: " + path);
-			Error err = OS::get_singleton()->move_to_trash(path);
-			if (err != OK) {
-				EditorNode::get_singleton()->add_io_error(TTR("Cannot remove:") + "\n" + file_path + "\n");
-			}
-		}
+		EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "import_paths", paths);
 
 		EditorNode::get_singleton()->restart_editor();
 	} else {
@@ -138,11 +131,42 @@ void SurfaceUpgradeTool::finish_upgrade() {
 			ResourceSaver::save(res);
 		}
 	}
+
+	// Remove the imported scenes/meshes from .import so they will be reimported automatically after this.
+	Vector<String> import_paths = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "import_paths", Vector<String>());
+	for (const String &file_path : import_paths) {
+		Ref<ConfigFile> config;
+		config.instantiate();
+		Error err = config->load(file_path);
+		if (err != OK) {
+			ERR_PRINT("Could not open " + file_path + " for upgrade.");
+			continue;
+		}
+
+		String remap_path = config->get_value("remap", "path", "");
+		if (remap_path.is_empty()) {
+			continue;
+		}
+
+		String path = OS::get_singleton()->get_resource_dir() + remap_path.replace_first("res://", "/");
+		print_verbose("Moving to trash: " + path);
+		err = OS::get_singleton()->move_to_trash(path);
+		if (err != OK) {
+			EditorNode::get_singleton()->add_io_error(TTR("Cannot remove:") + "\n" + remap_path + "\n");
+		}
+	}
+	EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "import_paths", Vector<String>());
+
+	emit_signal(SNAME("upgrade_finished"));
+}
+
+void SurfaceUpgradeTool::_bind_methods() {
+	ADD_SIGNAL(MethodInfo("upgrade_finished"));
 }
 
 SurfaceUpgradeTool::SurfaceUpgradeTool() {
 	singleton = this;
-	RS::get_singleton()->set_surface_upgrade_callback(_show_popup);
+	RS::get_singleton()->set_surface_upgrade_callback(_try_show_popup);
 }
 
 SurfaceUpgradeTool::~SurfaceUpgradeTool() {}
diff --git a/editor/surface_upgrade_tool.h b/editor/surface_upgrade_tool.h
index 444a107b9b..7067f5b0b5 100644
--- a/editor/surface_upgrade_tool.h
+++ b/editor/surface_upgrade_tool.h
@@ -35,19 +35,30 @@
 
 class EditorFileSystemDirectory;
 
-class SurfaceUpgradeTool {
+class SurfaceUpgradeTool : public Object {
+	GDCLASS(SurfaceUpgradeTool, Object);
+
 	static SurfaceUpgradeTool *singleton;
-	static bool popped_up;
+
+	bool show_requested = false;
+	bool popped_up = false;
 	Mutex mutex;
 
-	static void _get_import_paths(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths);
+	void _get_import_paths(EditorFileSystemDirectory *p_dir, Vector<String> &r_paths);
+
+	static void _try_show_popup();
+	void _show_popup();
+	void _add_files(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths);
 
-	static void _show_popup();
-	static void _add_files(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths);
+protected:
+	static void _bind_methods();
 
 public:
-	static void begin_upgrade();
-	static void finish_upgrade();
+	static SurfaceUpgradeTool *get_singleton() { return singleton; };
+
+	void begin_upgrade();
+	void finish_upgrade();
+
 	SurfaceUpgradeTool();
 	~SurfaceUpgradeTool();
 };

[YuriSizov]

Okay, so further testing showed a couple of errors. First of all, I kind of messed up the approach to upgrade internal meshes. Second, testing the space game repo (https://github.com/chrisl8/space-game by @chrisl8) highlighted an issue with autoloads. This project uses them, and they in turn load scenes. Because of the implementation of the editor, these are loaded way too early and we fail to catch them correctly.

The new diff builds on top of the old one, fixing both these issues. The general approach is simple: track everything before the restart, and then if the editor node is not ready for the tool, let it continue the upgrade at its earliest convenience.

So we collect all resources that we need to reimport and all resources that we need to resave, and then initiate an editor restart. Upon restart the editor may load some things before we can do anything, but that's fine. We wait for the editor to become ready and responsive and let it check if the tool wants to do something. If it does, we let it run the rest of the process.

It works correctly regardless if you delete the .godot folder or not. If we get caught by the import process, we wait for it to finish and only then ask the user about the upgrade (demonstratable with GDQuest's demo I linked before).

Follow-up diff, apply on top of the previous one

diff --git a/editor/editor_node.cpp b/editor/editor_node.cpp
index afe73db1d8..d17a1ccecc 100644
--- a/editor/editor_node.cpp
+++ b/editor/editor_node.cpp
@@ -1056,6 +1056,10 @@ void EditorNode::_sources_changed(bool p_exist) {
 
 			OS::get_singleton()->benchmark_dump();
 		}
+
+		if (SurfaceUpgradeTool::get_singleton()->is_show_requested()) {
+			SurfaceUpgradeTool::get_singleton()->show_popup();
+		}
 	}
 }
 
@@ -6825,6 +6829,13 @@ EditorNode::EditorNode() {
 
 	FileAccess::set_backup_save(EDITOR_GET("filesystem/on_save/safe_save_on_backup_then_rename"));
 
+	// Warm up the surface upgrade tool as early as possible.
+	surface_upgrade_tool = memnew(SurfaceUpgradeTool);
+	run_surface_upgrade_tool = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "run_on_restart", false);
+	if (run_surface_upgrade_tool) {
+		SurfaceUpgradeTool::get_singleton()->begin_upgrade();
+	}
+
 	{
 		int display_scale = EDITOR_GET("interface/editor/display_scale");
 
@@ -8067,12 +8078,6 @@ EditorNode::EditorNode() {
 	String exec = OS::get_singleton()->get_executable_path();
 	// Save editor executable path for third-party tools.
 	EditorSettings::get_singleton()->set_project_metadata("editor_metadata", "executable_path", exec);
-
-	surface_upgrade_tool = memnew(SurfaceUpgradeTool);
-	run_surface_upgrade_tool = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "run_on_restart", false);
-	if (run_surface_upgrade_tool) {
-		SurfaceUpgradeTool::get_singleton()->begin_upgrade();
-	}
 }
 
 EditorNode::~EditorNode() {
diff --git a/editor/surface_upgrade_tool.cpp b/editor/surface_upgrade_tool.cpp
index 58b19ef9a5..31834ed0c4 100644
--- a/editor/surface_upgrade_tool.cpp
+++ b/editor/surface_upgrade_tool.cpp
@@ -33,29 +33,14 @@
 #include "editor/editor_file_system.h"
 #include "editor/editor_node.h"
 #include "editor/editor_settings.h"
+#include "scene/scene_string_names.h"
 #include "servers/rendering_server.h"
 
 SurfaceUpgradeTool *SurfaceUpgradeTool::singleton = nullptr;
 
-void SurfaceUpgradeTool::_add_files(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths) {
+void SurfaceUpgradeTool::_add_files(EditorFileSystemDirectory *p_dir, Vector<String> &r_reimport_paths, Vector<String> &r_resave_paths) {
 	for (int i = 0; i < p_dir->get_subdir_count(); i++) {
-		_add_files(p_dir->get_subdir(i), r_paths);
-	}
-
-	for (int i = 0; i < p_dir->get_file_count(); i++) {
-		if (p_dir->get_file_type(i) == "Mesh" ||
-				p_dir->get_file_type(i) == "ArrayMesh" ||
-				p_dir->get_file_type(i) == "PackedScene") {
-			if (!FileAccess::exists(p_dir->get_file_path(i) + ".import")) {
-				r_paths.insert(p_dir->get_file_path(i));
-			}
-		}
-	}
-}
-
-void SurfaceUpgradeTool::_get_import_paths(EditorFileSystemDirectory *p_dir, Vector<String> &r_paths) {
-	for (int i = 0; i < p_dir->get_subdir_count(); i++) {
-		_get_import_paths(p_dir->get_subdir(i), r_paths);
+		_add_files(p_dir->get_subdir(i), r_reimport_paths, r_resave_paths);
 	}
 
 	for (int i = 0; i < p_dir->get_file_count(); i++) {
@@ -63,42 +48,49 @@ void SurfaceUpgradeTool::_get_import_paths(EditorFileSystemDirectory *p_dir, Vec
 				p_dir->get_file_type(i) == "ArrayMesh" ||
 				p_dir->get_file_type(i) == "PackedScene") {
 			if (FileAccess::exists(p_dir->get_file_path(i) + ".import")) {
-				r_paths.append(p_dir->get_file_path(i) + ".import");
+				r_reimport_paths.append(p_dir->get_file_path(i) + ".import");
+			} else {
+				r_resave_paths.append(p_dir->get_file_path(i));
 			}
 		}
 	}
 }
 
 void SurfaceUpgradeTool::_try_show_popup() {
-	if (singleton->show_requested) {
+	if (singleton->show_requested || singleton->popped_up) {
 		return;
 	}
 	singleton->show_requested = true;
 
+	RS::get_singleton()->set_warn_on_surface_upgrade(false);
+
 	if (EditorFileSystem::get_singleton()->is_importing()) {
 		EditorFileSystem::get_singleton()->connect("resources_reimported", callable_mp(singleton, &SurfaceUpgradeTool::_show_popup), CONNECT_ONE_SHOT);
-	} else {
+	} else if (EditorNode::get_singleton()->is_inside_tree()) {
 		singleton->_show_popup();
 	}
+
+	// EditorNode may not be ready yet. It will call this tool when it is.
 }
 
 void SurfaceUpgradeTool::_show_popup() {
 	MutexLock lock(mutex);
-
-	if (popped_up) {
+	if (!show_requested || popped_up) {
 		return;
 	}
-
+	show_requested = false;
 	popped_up = true;
-	RS::get_singleton()->set_warn_on_surface_upgrade(false);
 
 	bool accepted = EditorNode::immediate_confirmation_dialog(TTR("This project uses meshes with an outdated mesh format from previous Godot versions. The engine needs to update the format in order to use those meshes.\n\nPress 'Restart & Upgrade' to run the surface upgrade tool which will update and re-save all meshes and scenes. This update will restart the editor and may take several minutes. Upgrading will make the meshes incompatible with previous versions of Godot.\n\nPress 'Upgrade Only' to continue opening the scene as normal. The engine will update each mesh in memory, but the update will not be saved. Choosing this option will lead to slower load times every time this project is loaded."), TTR("Restart & Upgrade"), TTR("Upgrade Only"), 500);
 	if (accepted) {
 		EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "run_on_restart", true);
 
-		Vector<String> paths;
-		_get_import_paths(EditorFileSystem::get_singleton()->get_filesystem(), paths);
-		EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "import_paths", paths);
+		Vector<String> reimport_paths;
+		Vector<String> resave_paths;
+		_add_files(EditorFileSystem::get_singleton()->get_filesystem(), reimport_paths, resave_paths);
+
+		EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "reimport_paths", reimport_paths);
+		EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "resave_paths", resave_paths);
 
 		EditorNode::get_singleton()->restart_editor();
 	} else {
@@ -118,23 +110,22 @@ void SurfaceUpgradeTool::finish_upgrade() {
 	EditorNode::get_singleton()->trigger_menu_option(EditorNode::FILE_CLOSE_ALL, true);
 
 	// Update all meshes here.
-	HashSet<String> paths;
-	_add_files(EditorFileSystem::get_singleton()->get_filesystem(), paths);
-
-	EditorProgress ep("Re-saving all scenes and meshes", TTR("Upgrading All Meshes in Project"), paths.size());
+	Vector<String> resave_paths = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "resave_paths", Vector<String>());
+	EditorProgress ep("surface_upgrade_resave", TTR("Upgrading All Meshes in Project"), resave_paths.size());
 
-	for (const String &file : paths) {
-		Ref<Resource> res = ResourceLoader::load(file);
-		ep.step(TTR("Attempting to re-save ") + file);
+	for (const String &file_path : resave_paths) {
+		Ref<Resource> res = ResourceLoader::load(file_path);
+		ep.step(TTR("Attempting to re-save ") + file_path);
 		if (res.is_valid()) {
 			// Ignore things that fail to load.
 			ResourceSaver::save(res);
 		}
 	}
+	EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "resave_paths", Vector<String>());
 
 	// Remove the imported scenes/meshes from .import so they will be reimported automatically after this.
-	Vector<String> import_paths = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "import_paths", Vector<String>());
-	for (const String &file_path : import_paths) {
+	Vector<String> reimport_paths = EditorSettings::get_singleton()->get_project_metadata("surface_upgrade_tool", "reimport_paths", Vector<String>());
+	for (const String &file_path : reimport_paths) {
 		Ref<ConfigFile> config;
 		config.instantiate();
 		Error err = config->load(file_path);
@@ -155,7 +146,7 @@ void SurfaceUpgradeTool::finish_upgrade() {
 			EditorNode::get_singleton()->add_io_error(TTR("Cannot remove:") + "\n" + remap_path + "\n");
 		}
 	}
-	EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "import_paths", Vector<String>());
+	EditorSettings::get_singleton()->set_project_metadata("surface_upgrade_tool", "reimport_paths", Vector<String>());
 
 	emit_signal(SNAME("upgrade_finished"));
 }
diff --git a/editor/surface_upgrade_tool.h b/editor/surface_upgrade_tool.h
index 7067f5b0b5..70e07c58a1 100644
--- a/editor/surface_upgrade_tool.h
+++ b/editor/surface_upgrade_tool.h
@@ -44,11 +44,9 @@ class SurfaceUpgradeTool : public Object {
 	bool popped_up = false;
 	Mutex mutex;
 
-	void _get_import_paths(EditorFileSystemDirectory *p_dir, Vector<String> &r_paths);
-
 	static void _try_show_popup();
 	void _show_popup();
-	void _add_files(EditorFileSystemDirectory *p_dir, HashSet<String> &r_paths);
+	void _add_files(EditorFileSystemDirectory *p_dir, Vector<String> &r_reimport_paths, Vector<String> &r_resave_paths);
 
 protected:
 	static void _bind_methods();
@@ -56,6 +54,9 @@ protected:
 public:
 	static SurfaceUpgradeTool *get_singleton() { return singleton; };
 
+	bool is_show_requested() const { return show_requested; };
+	void show_popup() { _show_popup(); }
+
 	void begin_upgrade();
 	void finish_upgrade();
 

[clayjohn]

I have merged in @YuriSizov's latest diff and tested locally. In all my test scenes it appears to be working well.

@Lippanon @chrisl8 could you both test this PR again please?

[Lippanon]

Trying again to on a project last ran on v4.2.dev.custom_build [f7bc653] (it ran with no errors or warnings). It does finish importing assets before the prompt, however upon restart it still crashes with the same backtrace. It also crashes if I don't delete the .godot folder. I'm going to assume it's unrelated to the upgrade tool, but I can't really test until I get past the crash. I'll start bisecting but it will take me a while. Thanks for your work.

[YuriSizov]

@Lippanon I would recommend you start with any tool scripts, plugins, and extensions that you have. Possibly removing them completely. The trace that you show doesn't really point to anything of note that we can use to debug it. I would also try running Godot from the command line, using the .console.exe executable and with the --verbose flag set. So

godot.<...>.console.exe -e --path <path/to/your/project> --verbose

Edit: If you ever reference node paths relative to imported scenes, you may also want to pull #84271 into your build, which should make sure that paths for existing assets remain compatible with Godot 4.1.

[chrisl8]

@clayjohn I've run through this several times with my repo, which is the same one I'm sure you tested with, and it appears to work perfectly.

The popup showed up as expected, and the Restart & Upgrade option appears to have worked.
After that, I never see any messages about meshes needing to be updated.

The result appears to be the same whether I open a fresh clone for the first time (no .godot folder) or open it in 4.1 first and then in 4.2 (existing .godot folder).

This appears to be a success from my small perspective. Thank you.

For verification:
v4.2.beta.custom_build [be386e1]

[Lippanon]

Testing again in conjunction with #84271 allows me to successfully start the project and it seems to upgrade everything correctly. It takes me an extra restart and then there are finally no errors on project launch. I've played around with my ingame dev tools and don't notice any regressions with any models. Seems to be working well! 🎉 Thank you.

[YuriSizov]

Approving on the account of successful testing and our collective work on the editor workflow. But since I'm a co-author now, would be good to have some more eyes to check the code.

[akien-mga]

Looks good to me. Great teamwork!

[akien-mga]

Thanks!

[Parsnip]

I tried this with the Kenney's 3D Platformer Starter Kit (from Asset Library), loading that project and doing the Restart & Upgrade step will hang the Godot editor on Win 11.
However when I kill the process and relaunch Godot and the project, the conversion step seems to go through without any additional input from me as if the restart was normal. Everything seems to work as expected after that.

Doing Upgrade Only as the first step, and then manually reloading the project, and then doing Restart & Upgrade step also works. Not sure why the first launch is different. 🤔

Did some further testing and I can only reproduce this on Kenney's 3D Starter Kit so far but I don't have that many test cases. The space game that was mentioned earlier works fine on my end too, just as an example.

[YuriSizov]

@Parsnip Since it's a Godot 4.1 project, I would recommend opening it in 4.1 first. It shouldn't be required, but the issue here is that when you open it anew in 4.2, it tries to import a bunch of stuff. This in turn triggers thumbnail generation for a bunch of resources. On editor exit we try to terminate this process, but it seems to get stuck in a lock.

If you first open it in 4.1 and let it import, it should then correctly open in 4.2 with the option to convert it and such, which in my testing works as expected.

[Parsnip]

@YuriSizov Yup yup.
I was just testing it since I noticed some collision anomalies previously when I converted that project and was wondering if this would fix those (it did 👍 ), but the hang is obviously an undesired side effect.

Since I could only reproduce it on one project it felt like maybe it was hitting some weird edge case so I figured I'd mention it if people smarter than me want to take a look.

[YuriSizov]

We ended up re-evaluating this issue as a critical one, so I made a potential fix #84716.