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 script overwriting with external editor #96007

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Aug 23, 2024

#82956 does not fix the crash in #71016 if the external editor setting is turned off.
Basically this undoes #82956 and has an alternate fix for #71016.

#71016 happens because ScriptEditor::_reload_scripts it is called from a thread, and it is not safe to do so.
ERR_MAIN_THREAD_GUARD won't work here since GDScriptLanguageServer::thread_main sets set_current_thread_safe_for_nodes to true, even though its not really thread safe.

For instance, the crash is potentially caused by GDScriptSyntaxHighlighter::_get_line_syntax_highlighting_impl being called from TextEdit's draw in the main thread, while ScriptEditor::reload_scripts calls ScriptTextEditor::reload_text(), ScriptTextEditor::_validate_script(), then GDScriptSyntaxHighlighter::_update_cache, which changes things while it is trying to be drawn.

@kitbdev kitbdev requested a review from a team as a code owner August 23, 2024 20:49
@Chaosus Chaosus added this to the 4.4 milestone Aug 24, 2024
@radiantgurl
Copy link
Contributor

Will this reload also solve resource desyncs between the external editor and internal one?

@kitbdev
Copy link
Contributor Author

kitbdev commented Aug 24, 2024

Will this reload also solve resource desyncs between the external editor and internal one?

This should only affect script resources, they will be synced. Other resources should behave the same. Is there an issue for non-script resource desyncs?

@radiantgurl
Copy link
Contributor

Will this reload also solve resource desyncs between the external editor and internal one?

This should only affect script resources, they will be synced. Other resources should behave the same. Is there an issue for non-script resource desyncs?

No, i was just talking about script desyncs. Thank you for fixing it!

@petzki
Copy link

petzki commented Aug 27, 2024

This should fix #94080.

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 27, 2024
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Aug 27, 2024
@akien-mga akien-mga merged commit 61f2a33 into godotengine:master Aug 27, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@kitbdev kitbdev deleted the fix-external-script-reloading branch August 27, 2024 19:47
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
@BlackDragonBE
Copy link

Does this fix #82266 as well?

@akien-mga
Copy link
Member

Does this fix #82266 as well?

Since you've been looking into it, the easiest way to confirm would be for you to test https://godotengine.org/article/dev-snapshot-godot-4-4-dev-2/ which includes this fix.

@BlackDragonBE
Copy link

@akien-mga I just tested the steps to reproduce the bug with 4.4-dev2 and I can confirm that it's fixed. #82266 can be closed as well.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants