Hot-reloading of scripts ("Synchronize Script Changes") doesn't work when using external editor (VSCode)

[retro-git]

Godot version

4.0.beta17

System information

MacOS

Issue description

When the "Synchronize Script Changes" option is selected, scripts should hot-reload when changes are saved. However, this does not work when using Godot 4 with an external editor (only tested on VSCode). It does work when using the in-built text editor, or when using Godot 3.

Steps to reproduce

1. Configure Godot to use VSCode as an external editor.
2. Enable Synchronize Script Changes.
3. Edit a script file within VSCode while the game is running.

Minimal reproduction project

N/A

[rosshadden]

I don't want to turn this into a "me too!" comment stream, but I will say it's not just VS Code. I have this problem with Neovim as well, so it is very likely simply Godot isn't monitoring for inotify events or whatever equivalents on non-Unix systems.

I'm guessing the integrated editor runs some code on save that initiates the reloading, which would certainly be more efficient than monitoring the filesystem when applicable so it is very understandable. But it would be great if it did monitor the filesystem when users work with external editors.

[PathogenDavid]

I did some digging into this and I believe I've found the underlying cause as well as a potential fix.

This code in ScriptEditor::_update_modified_scripts_for_external_editor is what's typically responsible for telling the running game that it needs to reload scripts via _trigger_live_script_reload:

godot/editor/plugins/script_text_editor.cpp

Lines 728 to 739 in 013a457

	uint64_t last_date = scr->get_last_modified_time();
	uint64_t date = FileAccess::get_modified_time(scr->get_path());
	if (last_date != date) {
	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->update_exports();
	_trigger_live_script_reload();
	}

Separately: When an external editor is connected to the language server, it sends a didSave event when you save a file, which in turn calls GDScriptTextDocument::didSave, which reloads the script using GDScript::load_source_code:

godot/modules/gdscript/language_server/gdscript_text_document.cpp

Line 104 in 550a779

if (scr.is_valid() && (scr->load_source_code(path) == OK)) {

This in turn causes the script's last_modified_time to be updated.

Since the didSave callback happens well before _update_modified_scripts_for_external_editor, the date check in ScriptEditor::_update_modified_scripts_for_external_editor will never succeed as the timestamps will always be the same.

---

I did not have time to investigate fully, but I suspect this issue was introduced by #66867 as it introduced reloading the script within didSave.

---

I found that I can fix the issue by calling ScriptEditor::_trigger_live_script_reload from GDScriptTextDocument::didSave. See here: PathogenDavid@210edab

(As an added bonus this also fixes #73808, but only if you're connected to the language server)

However today is the first time I've ever really used Godot, so it's not immediately clear to me whether this is the most appropriate fix. (If it is, would be more than happy to clean things up and make a PR!)

[OmarShehata]

Thank you so much for the code snippet here @PathogenDavid . I've been using a custom build of Godot 4 with your patch and it has significantly improved my workflow. It would be great to have a PR for this. Maybe this could even only run if Use an external editor is on? If there's any concern on this triggering more than is needed for users who aren't using an external editor?

EDIT: Actually there is one potential issue: if you are typing in VSCode, and save incorrect syntax, the editor will come into focus. You then need to click on the editor, then click back on VSCode.

[clemens-tolboom]

<C++ Source> modules/gdscript/gdscript_vm.cpp:697 @ call()

In #75550 we use the build-in GDScript editor which @dakennedyd thinks relates to this issue. Does it?

[Dexterminator]

Is there any reason for not making the fix @PathogenDavid made into a PR? PathogenDavid@210edab

[PathogenDavid]

@Dexterminator I mainly left my comment to share my findings for someone who wishes to investigate this further and to share my quick and dirty fix (and perhaps learn if someone more involved with Godot's development thought it was good enough as-is.)

Based on the quirk @OmarShehata ran into, I suspect the real fix will need to be more complicated.

A lot has changed for me in the past 6 months and I'm no longer using Godot, so I most likely won't be the one to investigate further.

[geowarin]

@PathogenDavid I think it's a good change.
From what I understand, the LanguageServerProtocol will trigger a didSave request, which will be forwarded to the gdscript_text_document, which will trigger a reload.

You should open a PR so that people may test the change easily and maintainers can get a chance to review it.

[PathogenDavid]

From the Godot contribution guidelines:

[...] when contributing bug fixes - it's always best to discuss the implementation in the bug report first if you are not 100% about what would be the best fix.

My understanding is this is the appropriate and preferred place to discuss the change. I don't think my fix will be the whole fix--at the very least the issue Omar ran into needs to be addressed.

Additionally, I don't see how making a PR would make it any easier for people to test the change.

[geowarin]

A PR is the best way to discuss a code change in the engine.
Many PRs have limitations that can be discussed with the maintainers.

It is the normal review process to checkout a PR and test a change locally. Checking out a commit that is out of tree is more involved.

The issue of the editor requesting focus after a script error is, IMHO a minor inconvenience VS having no reload.

Anyway, if you want to discuss this change with somebody that knows the ins and outs of the engine, you have a better chance with a PR than waiting for one of those people to stumble into a code change mentioned in an issue.

[OmarShehata]

Just to share my 2 cents here: I think it's clear David is no longer able to commit time/resources to this. His snippet posted above is super valuable for anyone wanting to pick this up, and anyone is free to do so and open a PR. It makes sense that whoever opens a PR will likely also be the one to collect feedback from engine maintainer/test it/update it etc.

Anyway, since I do have an interest in seeing this fixed, and have been testing it locally with a patched version of the engine, I'd be happy to open a PR sometime next week if no one gets to it before then.

[eterlan]

Just to share my 2 cents here: I think it's clear David is no longer able to commit time/resources to this. His snippet posted above is super valuable for anyone wanting to pick this up, and anyone is free to do so and open a PR. It makes sense that whoever opens a PR will likely also be the one to collect feedback from engine maintainer/test it/update it etc.

Anyway, since I do have an interest in seeing this fixed, and have been testing it locally with a patched version of the engine, I'd be happy to open a PR sometime next week if no one gets to it before then.

Seems like there is already an pr about this？#82113 (comment)
Never mind, seems like doesn't work.

[OmarShehata]

@eterlan I think that is maybe fixing a slightly different situation?

I just started re-testing this with the latest version of Godot. Had some trouble initially but turns out it was just the language server not properly connecting.

The fix described in the patch above still works. The obvious thing needed before merging would be figuring out a proper way to force reload the script (currently the patch just makes a private function public).

I'm still familiarizing myself with these files, one thing I wanted to try is whether we just need to flip this boolean in the case where an external editor is being used:

ScriptEditor::get_singleton()->reload_scripts(true);

I also wanted to double check what the flow is for updating when you save inside the Godot editor, and if it makes sense to match that when saving with an external editor.

If I can't make progress I'll just open the PR and ask in the engine devs chat for further guidance