Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reloading scripts already in use #97168

Merged

Conversation

Hilderin
Copy link
Contributor

Passing the parameter p_keep_state to reload in Script::reload_from_file fixed the issue.

But, I saw another problem when the scripts are opened in the editor, they were reloaded twice, once in the Script Editor and another time in EditorFileSystem. I added a check in EditorFileSystem::_update_script_documentation to reload script only if they are not opened in the Script Editor.

@ryevdokimov
Copy link
Contributor

Could you elaborate on the other problem? If I modify a single script externally the reload is still being fired multiple times based on a print_line() test, but maybe I'm misunderstanding something.

Also, not sure if you want to fix this here, but this comment from your related previous PR is confusing.

I'm assuming you meant: "The only reason the script is already loaded here is to allow editing it outside the editor, without being connected to the LSP server."

// The only have the script already loaded here is to edit the script outside the
// editor without being connected to the LSP server.

@AThousandShips AThousandShips added this to the 4.4 milestone Sep 19, 2024
@Hilderin Hilderin force-pushed the fix-reloading-scripts-already-in-use branch from fafc608 to a42d379 Compare September 19, 2024 10:35
@Hilderin
Copy link
Contributor Author

Could you elaborate on the other problem? If I modify a single script externally the reload is still being fired multiple times based on a print_line() test, but maybe I'm misunderstanding something.

The problem I fixed was related to using the internal editor. However, you are right — using an external editor connected to the LSP server (e.g., VSCode) was causing a double reload. If you edit the file in Notepad, for example, or use git pull to update a GDScript while VSCode is closed, there’s no LSP server to inform Godot that the script was updated. What can be tricky is that even if you update the file in Notepad while VSCode is open and connected via LSP, it’s the LSP server that will trigger the script reload. However, after some testing, it’s not always the case, and I couldn’t find a consistent pattern.

After further investigation, I found that scripts were already being reloaded twice when edited in an external editor connected via LSP — once from GDScriptTextDocument::didSave and once in EditorNode::_resources_changed. I made additional modifications to prevent reloading the script more than once when saved via an external editor, whether LSP is used or not. Using the get_last_modified_time of the resource and comparing it with the file’s last modification time on disk in EditorFileSystem::_should_reload_script and EditorNode::_resources_changed solved the issue.

Additionally, I discovered that both the ScriptEditor and LSP server use get_language()->reload_tool_script to reload tool scripts. I couldn't see any difference between that and a simple reload(true), but just in case, I modified Script::reload_from_file to do exactly the same thing.

Note: The GDScript::reload method is still called twice when invoking Script::reload_from_file — once in GDScriptCache::get_full_script and once directly in Script::reload_from_file. This behavior existed before, and I didn’t change it.

I'm assuming you meant: "The only reason the script is already loaded here is to allow editing it outside the editor, without being connected to the LSP server."

Indeed, good catch. However, I removed that comment in the new version and created a function to check if the script needs to be reloaded: EditorFileSystem::_should_reload_script.


I also found a bug where adding or removing exported properties in tool scripts doesn't update live when the script is saved. You need to select another node and then reselect the node to refresh the property list in the inspector. I tested this without the PR, and the same thing happens. I'll create a separate issue for that.

@Hilderin Hilderin force-pushed the fix-reloading-scripts-already-in-use branch from a42d379 to 019ed62 Compare September 19, 2024 10:37
@Hilderin Hilderin force-pushed the fix-reloading-scripts-already-in-use branch from 0522bd7 to 958f519 Compare September 20, 2024 12:30
@KoBeWi
Copy link
Member

KoBeWi commented Sep 20, 2024

Not sure if related, but there is a bug where trying to run a build-in tool script (with File -> Run) will make it completely unable to be modified until editor restart (even closing and re-opening the scene will not help). It prints the same "already in use error").

Example:

@tool
extends Node

func _process(delta: float) -> void:
	print("A")

It will keep printing A. Change it to B and it will print B. Press Ctrl+Shift+X, you will get error in the console and any further script modifications will be ignored.

I can open a new issue, but since it involves the same error, I though you could fix it too.

@Hilderin
Copy link
Contributor Author

It's definitively related, once you get the already in use error the script will never reload after that. I'll try your example.

@Hilderin Hilderin force-pushed the fix-reloading-scripts-already-in-use branch from 958f519 to a3c5e5a Compare September 20, 2024 14:47
@Hilderin
Copy link
Contributor Author

Hilderin commented Sep 20, 2024

It will keep printing A. Change it to B and it will print B. Press Ctrl+Shift+X, you will get error in the console and any further script modifications will be ignored.

I tested your script with this PR and it fixes the issue. I can update the script inside Godot, External script with LSP or not and it's updating perfectly!

The only problem was that the first time that I attached the script to a node, I has to close and reopen the scene. I think this PR should fix that issue: #92099

@KoBeWi
Copy link
Member

KoBeWi commented Sep 20, 2024

I tested your script with this PR and it fixes the issue.

It does not for me.

godot.windows.editor.dev.x86_64_T0WmKhy2i1.mp4

ReloadFail.zip

@Hilderin Hilderin force-pushed the fix-reloading-scripts-already-in-use branch from a3c5e5a to 9638220 Compare September 20, 2024 16:12
@Hilderin Hilderin requested a review from a team as a code owner September 20, 2024 16:12
@Hilderin
Copy link
Contributor Author

It doesn't for me.

My bad, I tested it too quickly. The issue you're describing existed in 4.3 stable when trying to run a tool script that was still in memory. It's the same problem: before running the script, a reload(false) was executed, causing the same error message. Simply replacing false with true fixed the issue. Everywhere else in the ScriptEditor, reload was called with true.

I also retested with an EditorScript, and it still works fine.

I searched through the codebase, and the only places where reload could be called with false are in GDScriptLanguage::reload_scripts and GDScriptLanguage::reload_tool_script. I assume these could be called outside the editor?! I'm still not sure why we would want to pass p_soft_reload as false.

@akien-mga akien-mga merged commit 621cadc into godotengine:master Sep 20, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@dmlary
Copy link

dmlary commented Sep 21, 2024

Is there any chance we can get this fix cherry-picked for 4.3.1? It's painful to work on tool scripts without it right now on 4.3.

@akien-mga
Copy link
Member

This is a fix for a regression specific to 4.4, I don't think it's applicable as is to 4.3. Do you have a bug report detailing what issue you have in 4.3?

@dmlary
Copy link

dmlary commented Sep 21, 2024

This is a fix for a regression specific to 4.4, I don't think it's applicable as is to 4.3. Do you have a bug report detailing what issue you have in 4.3?

This same error and behavior happens in 4.3 for @tool scripts edited in an external editor that add menus or connect to signals (Edit: made minimal project, doesn't require any signals). The error appears during the first hot reload, but the changes are applied. Any subsequent edit will not hot reload.

I can open a separate issue to document it if necessary.

@dmlary
Copy link

dmlary commented Sep 21, 2024

Opened #97280 to reflect a similar problem in 4.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERROR: Condition "!p_keep_state && has_instances" is true. Returning: ERR_ALREADY_IN_USE
6 participants