-
-
Notifications
You must be signed in to change notification settings - Fork 589
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
Add static methods to ClassDB
for the methods bound to the ClassDB
singleton
#1164
Conversation
1a868cc
to
65aaceb
Compare
c7e0132
to
45290d4
Compare
I need GDExtension have better compatibility with Godot modules, too. Can we make other singletons have consistency with Godot source code? If we can't achive it in a appropriate way, I think keep all singletons in |
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 This is a good topic for a GDExtension meeting!
I think if we can agree on the principle, we don't need to do them all in one PR - we could do |
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 At least we should make this list as a independent file or a variable in |
2cfc6ea
to
de5e8ef
Compare
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. |
de5e8ef
to
c9fdd4d
Compare
c9fdd4d
to
6f91356
Compare
Taking this out of draft so that it can get some wider review! |
I'm not new to this PR, but I checked the update anyway. |
I didn't have any problems with 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 :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This works by:
ClassDBSingleton
class for the singleton, to avoid a conflict with the hand-writtenClassDB
that's used for registering classes, methods, etcClassDB
that forward to the methods onClassDBSingleton
. These forward methods are generated into a preprocessor macro inclass_db_singleton.hpp
that is expanded inside theClassDB
classThis 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, asClassDB::get_class_list()
rather thanClassDB::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.