Skip to content

Don't cache null forever if a singleton isn't available yet #1181

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 27, 2023
Merged

Don't cache null forever if a singleton isn't available yet #1181

merged 1 commit into from
Jul 27, 2023

Conversation

@Zylann
Copy link
Collaborator

@Zylann Zylann commented Jul 17, 2023 •
edited
Loading

This fixes the third point of issue #1180 (and perhaps the second one if that was the cause all along, but havent checked that yet)
If you call get_singleton too early and it returns nullptr, it would keep returning nullptr forever.
@Zylann Zylann requested a review from a team as a code owner July 17, 2023 01:01
@Calinou Calinou added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Jul 17, 2023
@dsnopek dsnopek mentioned this pull request Jul 17, 2023
Closed
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 17, 2023 •
edited
Loading

Thanks!

Since GDExtensions can try to access singletons before they are available, and we don't want to permanently punish a GDExtension for trying, this makes sense to me.

However, it would also be great to make the core singletons available a little bit earlier - see PR godotengine/godot#79584

FYI, this will conflict with PR #1176 so one or the other will end up needing to be rebased, depending on which is merged first
@Zylann Zylann mentioned this pull request Jul 18, 2023
Merged
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 22, 2023

This needs a rebase now that #1176 is merged
@Zylann Zylann force-pushed the fix_singleton_caching branch from 1ffa645 to 47d5eef Compare July 22, 2023 14:48
@Zylann
Copy link
Collaborator Author

Zylann commented Jul 22, 2023

Rebased
dsnopek
dsnopek requested changes Jul 22, 2023
View reviewed changes
@Zylann Zylann force-pushed the fix_singleton_caching branch from 47d5eef to 548c758 Compare July 22, 2023 15:31
@Zylann
Copy link
Collaborator Author

Zylann commented Jul 22, 2023

Updated
@dsnopek dsnopek self-requested a review July 22, 2023 15:41
dsnopek
dsnopek approved these changes Jul 22, 2023
View reviewed changes
Copy link
Collaborator

@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 looks great to me!
dsnopek
dsnopek approved these changes Jul 27, 2023
View reviewed changes
Copy link
Collaborator

@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.

Discussed at the GDExtension meeting, and we think this makes sense!
@dsnopek dsnopek merged commit c5d8447 into godotengine:master Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation

3 participants

@Zylann @dsnopek @Calinou