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 how scripts reload outside of ScriptEditor #89261

Merged

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Mar 7, 2024

#82113 made script being reloaded from outside ScriptEditor. The current Resource::reload_from_file() implementation indirectly calls Object::set_edited(true) on the script resource, so any script reloaded that way ends up being overwritten by the engine when a scene they are in is saved.

This is simply making sure we apply the same logic in Script::reload_from_file() as in ScriptEditor::reload_scripts(). Although, it feels quite weird that we purposefully call that method when Script::editor_can_reload_from_file() returns false.

@paulloz paulloz added this to the 4.3 milestone Mar 7, 2024
@paulloz paulloz requested a review from a team as a code owner March 7, 2024 19:29
@paulloz paulloz force-pushed the core/fix-script-reloading-outside-script-editor branch from 5d9d291 to 9200277 Compare March 7, 2024 19:46
@akien-mga
Copy link
Member

CC @nongvantinh

@nongvantinh
Copy link
Contributor

nongvantinh commented Mar 9, 2024

Hi @paulloz,

When Script::editor_can_reload_from_file() returns false, it means the Editor won't update to reflect new changes in that script. It will be updated through the internal script editor instead.

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

However, If the file is edited outside the editor and we don't use the internal editor, there's no way to get the latest changes. Consequently, whenever we use data of that file from ResourceCache, we always get outdated data, which can lead to issues requiring the most current and valid data. For instance, when we press Ctrl + S and the resource's state is marked as edited, then we save the current state of that script using state taken from ResourceCache, causing it to override the changes we made previously.

This problem stems from the Script::reload_from_file() method, which hasn't been used before. If it were, this issue would have occurred long ago. However, thanks to #82113, we now know that it doesn't function correctly.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the issue with GDScript, but the code looks fine.

@paulloz paulloz force-pushed the core/fix-script-reloading-outside-script-editor branch from 9200277 to 6367464 Compare March 18, 2024 09:29
akien-mga added a commit that referenced this pull request Mar 24, 2024
…ide-script-editor

Fix how scripts reload outside of ScriptEditor
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 06abc86 Mar 24, 2024
@akien-mga
Copy link
Member

Thanks!

@paulloz paulloz deleted the core/fix-script-reloading-outside-script-editor branch April 19, 2024 20:09
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.

4.3 csharp files are changed on editor save causing issues in IDEs - regression? Save in VS code in 4.3-dev5
5 participants