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

Add static methods to ClassDB for the methods bound to the ClassDB singleton #1164

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Jul 7, 2023

This works by:

  • Generating a ClassDBSingleton class for the singleton, to avoid a conflict with the hand-written ClassDB that's used for registering classes, methods, etc
  • Creating static methods on ClassDB that forward to the methods on ClassDBSingleton. These forward methods are generated into a preprocessor macro in class_db_singleton.hpp that is expanded inside the ClassDB class

This is an alternate way to implement PR #936 as I described in this comment.

The primary difference between that PR and this one, is that you can call the methods statically on ClassDB, for example, as ClassDB::get_class_list() rather than ClassDB::get_singleton()->get_class_list(), for better source compatibility with Godot modules.

NOTE: This is currently marked as a draft, because it's primarily here for discussion purposes! However, if we do want to go this way, then I'd like to add an automated test as well.

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Jul 7, 2023
@dsnopek dsnopek requested a review from a team as a code owner July 7, 2023 02:24
@dsnopek dsnopek force-pushed the classdb-singleton-alternate branch from 1a868cc to 65aaceb Compare July 7, 2023 02:27
@dsnopek dsnopek marked this pull request as draft July 7, 2023 02:37
@dsnopek dsnopek force-pushed the classdb-singleton-alternate branch 2 times, most recently from c7e0132 to 45290d4 Compare July 7, 2023 02:56
@Daylily-Zeleen
Copy link
Contributor

Daylily-Zeleen commented Jul 7, 2023

I need GDExtension have better compatibility with Godot modules, too.

Can we make other singletons have consistency with Godot source code?
Like ResourceUID, Engine work as singleton style, and ResourceLoader, ResourceSaver work as static method style.

If we can't achive it in a appropriate way, I think keep all singletons in @GlobalScope are work as singleton is make more sense than only make ClassDB work as static style.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Jul 7, 2023

Can we make other singletons have consistency with Godot source code?
Like ResourceUID, Engine work as singleton style, and ResourceLoader, ResourceSaver work as static method style.

Personally, I'd be for that!

If a method would be called statically from within a Godot module, I personally think we should generate the code in godot-cpp to do the same for source compatibility.

The problem, of course, is that the engine itself is somewhat inconsistent in this regard. We'll probably end up having to hard-code a list of singletons that should be treated this way in binding_generator.py.

This is a good topic for a GDExtension meeting!

If we can't achive it in a appropriate way, I think keep all singletons in @GlobalScope are work as singleton is make more sense than only make ClassDB work as static style.

I think if we can agree on the principle, we don't need to do them all in one PR - we could do ClassDB first, and address the others in a follow-up PR (or PRs).

@Daylily-Zeleen
Copy link
Contributor

Daylily-Zeleen commented Jul 7, 2023

I think if we can agree on the principle, we don't need to do them all in one PR - we could do ClassDB first, and address the others in a follow-up PR (or PRs).

For maintainability, I think we should make it generating a list automatically, to mark which classes should be singleton style and which classes should be static style, then use this list to generate in binding_generator.py, instead of special processing for some classes.

At least we should make this list as a independent file or a variable in binding_generator.py by hard coding (because these class are not changed frequently).

@dsnopek dsnopek force-pushed the classdb-singleton-alternate branch 2 times, most recently from 2cfc6ea to de5e8ef Compare July 7, 2023 20:36
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jul 27, 2023

Discussed at the GDExtension meeting, and we agree that it's a design goal for GDExtension to have an API that is as close as possible to modules, such that code can be copy-pasted between them, or a code base could be compiled either as a GDExtension or module. For the other cases where we have API's already in godot-cpp that contradict their Godot equivalents, we can open other issues/PRs to address them.

This specific implementation on this PR still needs further review and discussion.

@dsnopek dsnopek force-pushed the classdb-singleton-alternate branch from de5e8ef to c9fdd4d Compare July 31, 2023 21:02
@dsnopek dsnopek force-pushed the classdb-singleton-alternate branch from c9fdd4d to 6f91356 Compare July 31, 2023 21:04
@dsnopek dsnopek marked this pull request as ready for review July 31, 2023 21:43
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jul 31, 2023

Taking this out of draft so that it can get some wider review!

@DmitriySalnikov
Copy link
Contributor

get some wider review!

I'm not new to this PR, but I checked the update anyway.
I used #1165 and checked several methods from ClassDB and ClassDBSignleton and everything worked correctly (including my class exclusion).
The new alias system seems to be working correctly. Now it remains only to make all singletons static as well 😅

@DmitriySalnikov
Copy link
Contributor

I didn't have any problems with ClassDB while writing an API generator for C# (10 different uses of ClassDB::)

Does this require more testers or do not everyone still agree with this approach?

@dsnopek
Copy link
Collaborator Author

dsnopek commented Sep 16, 2023

Does this require more testers or do not everyone still agree with this approach?

The last time we discussed this at a GDExtension meeting, we agreed on the high-level idea, but that another godot-cpp maintainer should review the code in the PR and the specific approach (ie. generating code into a macro to forward function calls), but I don't think anyone has had time to do that yet.

Ping @Faless @vnen @BastiaanOlij :-)

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.

LGTM

@dsnopek dsnopek merged commit 0d6de7a into godotengine:master Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants