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 updating cached singletons when reloading GDScripts #85373

Merged
merged 1 commit into from Dec 4, 2023
Merged

Fix updating cached singletons when reloading GDScripts #85373

merged 1 commit into from Dec 4, 2023

Conversation

DmitriySalnikov
Copy link
Contributor

Fixes #85004

Since GDScriptLanguage::reload_all_scripts() is always called when the GDExtension reload is completed, I decided to update the singleton cache there.
I used the original singleton caching code, added a key existence check, and just overwrite the values.

Without globals.has(E.name) singletons that were registered via GDScript itself or for example GDScriptLanguageProtocol can be added to the cache after the GDExtension reload and used as the rest of the singletons in GDScript.

#52162 could be fixed if calling reload_all_scripts somewhere after loading the editor (there?) and remove globals.has(E.name). I tried updating scripts in Engine::add_singleton, but it is advisable to do it deferred, and Engine is a regular class that cannot call methods via callable_mp (although it was possible to add a static method and a static boolean variable to prevent multiple calls in one frame). This can be tested in a separate PR, but I'm not really interested in it.

It would be cool to merge this before the 4.2 release.

@DmitriySalnikov DmitriySalnikov requested a review from a team as a code owner November 26, 2023 08:19
@AThousandShips AThousandShips added this to the 4.3 milestone Nov 26, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 26, 2023
@DmitriySalnikov
Copy link
Contributor Author

I simplified this commit. Now everything happens in reload_all_scripts. Singletons are still updated only in the editor. But I'm not sure if it's necessary to place this code inside a block with a lock?

Right now there are only two cases when a reload_all_scripts is called: RemoteDebugger in a running game, after saving scripts in the editor, GDExtensionManager in the editor when the libraries are reloaded. Therefore, only the second case will be affected here, while nothing is added or deleted, only pointers are updated, which without this change do not allow using singletons with GDExtension hot reload.

Btw 4.2 will be the first release with hot reload support for GDExtension. 4.2 RC2 isn't the last release candidate, is it?

@akien-mga
Copy link
Member

Btw 4.2 will be the first release with hot reload support for GDExtension. 4.2 RC2 isn't the last release candidate, is it?

A release candidate should always expect to be the last, otherwise it's not a release candidate ;)

This doesn't seem critical (doesn't prevent shipping games, only impacts hot reload during extension development), so it doesn't seem urgent to merge mere days before the stable release. Like other bug fixes, it can be merged afterwards and cherry-picked for 4.2.1.

@ValorZard
Copy link

I'd like to offer a counterpoint. Hot Reloading for GDExtension seems like it will be a very popular feature for 4.2, which means making sure its bug free and ready to be used is probably important. Although, I'm not sure how annoying this bug is, so YMMV.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

This fixes the issue with the MRP from #85004 in my testing.

The code makes sense to me based on looking at the references from the PR description, but it should probably be reviewed by someone on the GDScript team.

modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Refreshing the pointers to the singletons are necessary if the extensions are reloaded.

@akien-mga akien-mga changed the title Fixed updating cached singletons when reloading GDScripts Fix updating cached singletons when reloading GDScripts Dec 1, 2023
@akien-mga akien-mga merged commit d75c446 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@DmitriySalnikov DmitriySalnikov deleted the update_singletons branch December 5, 2023 07:06
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

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.

GDScript does not update cached singleton references when GDExtension is reloaded
7 participants