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

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

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

Zylann
Copy link
Collaborator

@Zylann Zylann commented Jul 17, 2023

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
Copy link
Collaborator

dsnopek commented Jul 17, 2023

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

@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

binding_generator.py Outdated Show resolved Hide resolved
@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
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!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants