Allow Editor to reload external changes of scripts

[nongvantinh]

Allow Editor to reload external changes of scripts

Fixes: #50163

[nongvantinh]

I have found the root cause:
Commit da0ec37
Author: Juan Linietsky reduzio@gmail.com
Authored Date: Wednesday, November 21, 2018 7:47 AM
Committer: Juan Linietsky reduzio@gmail.com
Commit Date: Wednesday, November 21, 2018 7:48 AM
Parent: f2cc969

Reworked how non-imported resources are reloaded on change, fixes #19852

Commit efdcf20
Author: Juan Linietsky reduzio@gmail.com
Date: Monday, June 27, 2016 11:17 PM
Parent: 88e28af

Make most resources (save for packedscenes and scripts) reload if they change on disk. Closes #4059.

the editor is still scanning and has detected that the C# file has changed. However, editor_can_reload_from_file returns false

godot/core/object/script_language.h

Lines 111 to 112 in fe5b1c8

	protected:
	virtual bool editor_can_reload_from_file() override { return false; } // this is handled by editor better

Additionally, with this statement, it skips the reload.

godot/editor/editor_node.cpp

Lines 855 to 856 in fe5b1c8

	if (!res->editor_can_reload_from_file()) {
	continue;

[raulsntos]

Changing this would break behavior compatibility for every ScriptLanguage implementation that doesn't override this method, not sure if this only affects modules or also GDExtensions.

[KoBeWi]

If the issue is about C#, why not just override editor_can_reload_from_file() in CSharpScript? Your current code is effectively the same as you did before.
Script language disables reloading by default, because the editor can detect and reload scripts. Not sure how does it handle scripts that aren't opened, but they seem to be correctly reloaded somehow.

I suspect some bug with C#; the editor never changes the script (or at least the user does not edit it within the editor) so there is no reason for saving it.

EDIT: Nevermind, I tested and scripts that aren't opened are not reloaded. Which makes it more likely that it's some bug that causes the script to be saved when it shouldn't.

[nongvantinh]

I tested and scripts that aren't opened are not reloaded. Which makes it more likely that it's some bug that causes the script to be saved when it shouldn't

What do you mean in this context? Is the script you are talking about open in an external editor or internal editor? Can you give me more information so I can troubleshoot it easier?

[KoBeWi]

I added a print to GDScript::reload(). When you have a script opened in script editor and save it, the reload is called. Even if you save it externally, the reload is also called. But when you save a script externally and it's not opened in the internal script editor, the script is not reloaded, even if it's part of the scene.

What I mean is that there is no reason to force reloading a script. The fact that C# scripts are cleared is a bug unrelated to reloading. GDScript is not being reloaded, but it doesn't have the same problem.

[nongvantinh]

The fact that C# scripts are cleared is a bug unrelated to reloading

I have commented on the bug ticket, but I'll also mention it here. Previously, I fixed a part of the bug in the given image, but that only fixed a portion of the issue, and it only worked when the user pressed Ctrl + S to save the scene, and the script had to be marked as edited. However, as I delved deeper into this case, I realized that the bug is actually caused by the script not reloading, even though the editor knows it has been changed. Because of that, I came up with this PR.

It has been confirmed that the guess I provided earlier is correct. The caching system does not recognize changes made outside the editor, causing it to overwrite the modified C# file with cached data.

[KoBeWi]

The caching system does not recognize changes made outside the editor, causing it to overwrite the modified C# file with cached data.

The question is why does it overwrite it? And why GDScript, which also doesn't get reloaded, doesn't have this problem? (unless I missed something when testing and it is reloaded, idk)

[nongvantinh]

When you save a script externally, and it's not opened in the internal script editor, the script is not reloaded, even if it's part of the scene.

For this, you may need to read the Try with Internal editing section in this comment. I have monitored these cases, and it is a mechanism of the internal editor . However, both GDScript and C# suffer from the same problem, but only people writing code in C# use an external editor.

Here is the place where the internal editor load the file from disk:

godot/editor/plugins/script_editor_plugin.cpp

Lines 2615 to 2622 in fcbc50e

	Ref<Script> scr = edited_res;
	if (scr.is_valid()) {
	Ref<Script> rel_scr = ResourceLoader::load(scr->get_path(), scr->get_class(), ResourceFormatLoader::CACHE_MODE_IGNORE);
	ERR_CONTINUE(!rel_scr.is_valid());
	scr->set_source_code(rel_scr->get_source_code());
	scr->set_last_modified_time(rel_scr->get_last_modified_time());
	scr->reload(true);
	}

Which is different than the case script gets clear/reset when user press Ctr + S:

godot/editor/editor_node.cpp

Lines 1677 to 1692 in fcbc50e

	for (const String &E : edited_resources) {
	Ref<Resource> res = ResourceCache::get_ref(E);
	if (!res.is_valid()) {
	continue; // Maybe it was erased in a thread, who knows.
	}
	Ref<PackedScene> ps = res;
	if (ps.is_valid()) {
	continue; // Do not save PackedScenes, this will mess up the editor.
	}
	ResourceSaver::save(res, res->get_path(), flg);
	saved++;
	}
	EditorUndoRedoManager::get_singleton()->set_history_as_saved(EditorUndoRedoManager::GLOBAL_HISTORY);
	return saved;

Commit 9eb5f2a
Authored Date: Thursday, June 23, 2022 12:03 AM
Commit Date: Friday, June 24, 2022 2:25 AM
Parent: 3ccff61

Simplify Subresource Saving

Redo edited subresource (and resource) saving in a much more simplified way.
I think this should work (unless I am missing something) and be faster than what is there.
It should also supersede #55885.

I am not 100% entirely convinced that this approach works, but I think it should so please test.

[nongvantinh]

GDScript, which also doesn't get reloaded, doesn't have this problem?

I just tested the steps to reproduce #50163 (comment) with GDScript and found out it also suffer from the same problem, This issue extends to all other Scripting Languages that doesn't override editor_can_reload_from_file to return true

But when you save a script externally and it's not opened in the internal script editor, the script is not reloaded, even if it's part of the scene.

This PR address this problem, you can refer to my earlier comment: #82113 (comment)

[eterlan]

GDScript, which also doesn't get reloaded, doesn't have this problem?

I just tested the steps to reproduce #50163 (comment) with GDScript and found out it also suffer from the same problem, This issue extends to all other Scripting Languages that doesn't override editor_can_reload_from_file to return true

But when you save a script externally and it's not opened in the internal script editor, the script is not reloaded, even if it's part of the scene.

This PR address this problem, you can refer to my earlier comment: #82113 (comment)

Test it with gdscript, seems like it still doesn't reload after I change code in vscode and save it, unless I save the script in editor.

[nongvantinh]

There are three options available to permanently resolve the issue where the editor fails to reload a script edited outside of the editor for scripting languages:

1. Modify the behavior of editor_can_reload_from_file for the Script to always return true.
2. Introduce a check:

		Ref<Script> is_script = res;
		if (!res->editor_can_reload_from_file() && is_script.is_null()) 

3. Remove the editor_can_reload_from_file check during reloading.

Option 1, while effective, introduces breaking changes for all scripting languages. However, both major scripting languages affected by this issue—C# and GDScript—share the same problem. How many scripting languages escape this issue?

Option 2 is more viable, but it does add redundant code.

Option 3 is not reasonable. So, we're left with two options. But why would anyone want their script not to be reloaded by the editor? Does their script contain the secret to a nutri bomb or something? Upon reviewing the source code again, I found that only one place uses the editor_can_reload_from_file method, which is the EditorNode. This is causing the editor to not recognize changes and poses potential dangers to scripts edited outside the editor.

Because of this, I choose option 1. If anyone wishes their script implementation to suffer from numerous issues related to the editor being unable to reload files edited outside the editor, they can override editor_can_reload_from_file to return false to achieve that behavior.

Currently, there are two classes: PackedScene and TextFile, both of which override editor_can_reload_from_file to return false. If these files were edited outside the editor, they would encounter this issue because the ResourceCache fails to recognize the changes, and the editor heavily relies on ResourceCache. Who knows where it might cause a bug in the future? The statement

// this is handled by the editor better

is no longer true.

[eterlan]

There are three options available to permanently resolve the issue where the editor fails to reload a script edited outside of the editor for scripting languages:

1. Modify the behavior of editor_can_reload_from_file for the Script to always return true.
2. Introduce a check:

		Ref<Script> is_script = res;
		if (!res->editor_can_reload_from_file() && is_script.is_null()) 

3. Remove the editor_can_reload_from_file check during reloading.

Option 1, while effective, introduces breaking changes for all scripting languages. However, both major scripting languages affected by this issue—C# and GDScript—share the same problem. How many scripting languages escape this issue?

Option 2 is more viable, but it does add redundant code.

Option 3 is not reasonable. So, we're left with two options. But why would anyone want their script not to be reloaded by the editor? Does their script contain the secret to a nutri bomb or something? Upon reviewing the source code again, I found that only one place uses the editor_can_reload_from_file method, which is the EditorNode. This is causing the editor to not recognize changes and poses potential dangers to scripts edited outside the editor.

Because of this, I choose option 1. If anyone wishes their script implementation to suffer from numerous issues related to the editor being unable to reload files edited outside the editor, they can override editor_can_reload_from_file to return false to achieve that behavior.

Currently, there are two classes: PackedScene and TextFile, both of which override editor_can_reload_from_file to return false. If these files were edited outside the editor, they would encounter this issue because the ResourceCache fails to recognize the changes, and the editor heavily relies on ResourceCache. Who knows where it might cause a bug in the future? The statement

// this is handled by the editor better

is no longer true.

May I ask a question? If I use tool to generate code without lsp connected, the option 1 can handle this situation right?

[nongvantinh]

@eterlan The fix aims only to address the issue where the editor fails to reload the file when it is edited outside the editor. However, with these new changes, it applies to all scripting languages not just C#, so you can try it with the bug you reported.

[YuriSizov]

Do I understand correctly, that this PR relies on #85504?

[nongvantinh]

Yes.

[YuriSizov]

Let's convert it to draft for now then.

[akien-mga]

Thanks!