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 #64975 by introducing a new initialization level. #65018

Closed
wants to merge 1 commit into from

Conversation

Splizard
Copy link
Contributor

@Splizard Splizard commented Aug 29, 2022

Previously, GDExtension libraries would fail to return Server singletons
'global_get_singleton' at any initialization level #64975 (they were registered at the very end of setup2).

This enables Server singletons to be retrieved at the 'new'
GDNATIVE_INITIALIZATION_COMPLETE.

@Splizard Splizard requested a review from a team as a code owner August 29, 2022 06:06
@akien-mga akien-mga added this to the 4.0 milestone Aug 29, 2022
@akien-mga akien-mga requested a review from a team August 29, 2022 06:09
@Splizard
Copy link
Contributor Author

Splizard commented Aug 29, 2022

Looks like some of the singletons are now returning as nil, so perhaps this isn't quite the fix that is needed. Happy to have some guidance here. EDIT: has been fixed.

bruvzg
bruvzg previously requested changes Aug 29, 2022
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

register_server_singletons is already exactly where it supposed to be, you can't register singletons before servers are created and initialized, and the last (navigation server) one is initialized just one line before the current location of register_server_singletons.

@Splizard
Copy link
Contributor Author

Splizard commented Aug 29, 2022

@bruvzg
got it, would it be a better fix be to introduce a new initialization level for GDExtension (and trigger this at the end of setup2)? or are singletons not meant to be retrievable by a GDExtension at 'initialization' time.

@bruvzg
Copy link
Member

bruvzg commented Aug 29, 2022

I guess the proper way to fix it, is to register each singleton as soon as server is created (from the server constructors), instead of dedicated register_server_singletons. This way, availability of servers for GDExtension will be the same as for the engine.

@Splizard
Copy link
Contributor Author

Splizard commented Aug 29, 2022

Adding the new initialization level and using it fixes the issue and enables all server singletons to be retrieved. Perhaps GDNATIVE_INITIALIZATION_COMPLETE should be called something else?

  • GDNATIVE_INITIALIZATION_READY
  • GDNATIVE_INITIALIZATION_FINAL
  • GDNATIVE_INITIALIZATION_DONE

Let me know and I can change this.

@Splizard Splizard requested a review from bruvzg August 29, 2022 06:53
@touilleMan
Copy link
Member

My 2c here: I'd rather introduce a GDNATIVE_INITIALIZATION_SINGLETONS step in order to make it obvious where the singleton should be registered (and hence that the singleton cannot be fetched before that !)

I've myself spend quite some time to understand how to fetch a singleton (I thought I was using the API wrong when I was just querying it to soon)

As a more general wish, I think it would be great to add a small comment in gdnative_interface.h for each initialisation level to give examples of what kind of extension should be initialized when

@Splizard Splizard changed the title Fix #64975 by registering server singletons earlier. Fix #64975 by introducing a new initialization level. Aug 29, 2022
@bruvzg bruvzg dismissed their stale review September 2, 2022 08:01

I guess new init level is also OK.

@Splizard
Copy link
Contributor Author

Splizard commented Sep 5, 2022

Would it be possible to rerun the workflows for this PR? Also please let me know if I should squash my commits (requires me to make a force push to the branch).

@akien-mga
Copy link
Member

Yes the commits should be squashed. I would also recommend rebasing at the same time so that you get the latest master commits.

Fixes godotengine#64975 by enabling server singletons to be retrieved at
initialization time.

Previously, GDExtension libraries would fail to return Server singletons
ie. 'global_get_singleton' at any defined initialization level.

This enables Server singletons to be retrieved at
GDNATIVE_INITIALIZATION_COMPLETE time so that GDExtensions can load all
singletons on startup.
@Splizard
Copy link
Contributor Author

Splizard commented Oct 12, 2022

Have rebased against master, considering the feature freeze, will this PR and/or alternatively fixing #64975 still make it in before 4.0 release?

@Faless
Copy link
Collaborator

Faless commented Oct 12, 2022

I don't think we need another initialization level, but definitely it should not go after GDNATIVE_INITIALIZATION_EDITOR.
See the discussions in #58629, #60317 and #60723 .
As proposed by @bruvzg we should configure the singleton earlier, and have them ready as soon as possible.
If, if, we add a new level, it MUST be before the EDITOR level (which is optional).

@groud
Copy link
Member

groud commented Dec 15, 2022

We reviewed this PR in the GDextension meeting, and we don't think this is the right solution to the problem. The singletons should be available a lot earlier than what they are right now so adding another initialization level is not the right solution to the issue. Singletons should instead be loaded earlier on.

Since the solution we would prefer is very different from that proposed here, we'll close this PR. We've also added the corresponding issue to the 4.0 release milestone. Thank you nonetheless.

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.

6 participants