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

GDExtension: Save and compare modification times separately for reload #84315

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Nov 1, 2023

In the current GDExtension reload code, we're combining the last modification time of the .gdextension file and the library file by storing only the highest one, and then checking if it's changed by checking if either files' new modification time is greater than that value. This works great on Linux, where modification times move forward in time by default, even when copying existing files (unless you go out of your way to preserve the modification time).

However, this can have some issues on Windows because copied files will have the same modification time of the original file by default.

So, this PR switches to storing each last modification time individually, and simply checks if the modification time has changed (not that it's greater, just that it's different). As far as I can tell, this matches how the editor decides to reload normal resources, which also have "the two file problem" where you have the actual content file (ex file.png) and the import settings file (ex file.png.import).

Fixes #82738

@Bromeon
Copy link
Contributor

Bromeon commented Nov 1, 2023

Nice catch! I think it would be great to document your observations also in the code; that way it's clear why it was done, and won't accidentally be reverted.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@dsnopek dsnopek force-pushed the gdextension-windows-modification-time branch from 2ba3bdd to f86054e Compare November 2, 2023 13:29
@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 2, 2023

Thanks!

@Bromeon:

I think it would be great to document your observations also in the code; that way it's clear why it was done, and won't accidentally be reverted.

I'm not so sure this change is at a big risk of being accidentally reverted, however, just to be safe :-) I added a short comment to GDExtension::has_library_changed() explaining why doing the check like this is important on Windows.

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks 🙂

@YuriSizov YuriSizov merged commit 1b42673 into godotengine:master Nov 2, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@dsnopek dsnopek deleted the gdextension-windows-modification-time branch July 22, 2024 15:29
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.

GDExtension - failure to reload classes
4 participants