-
-
Notifications
You must be signed in to change notification settings - Fork 21k
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
Allow GDExtension to register unexposed classes. #70329
Allow GDExtension to register unexposed classes. #70329
Conversation
You should have been able to rebase this and the other PR, see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html for details. But now that you opened new PRs it's fine, just something to keep in mind for later. Both new PRs have unsuitable commit messages, "remake" is not informative. The commit message should say how the PR affects the engine, not what you did. |
5057d54
to
b2dac6e
Compare
Thanks for your warning. I fixed the commit message. |
b2dac6e
to
c2835a5
Compare
Does this actually prevent the class from being added to the Add X dialogs and Editor Help? |
The variable I do a little change in
You can avoid temporarily instantiation by registering abstract class. This is not the issue of this pr. |
c2835a5
to
efd4f9f
Compare
Overall, I think this is a great change! However, it will need to be done in a slightly different way to ensure compatibility. We haven't yet made a change to a struct in
Anyway, like I said, we haven't done one of these yet, so exact details (especially around naming) may take a different shape as this PR (or others which need to do a similar thing) are discussed. |
Now that PR #78634 has been merged, the @Daylily-Zeleen Is this a change you have time to do? If not, I'd be happy to take this over in a new PR :-) |
efd4f9f
to
1055435
Compare
@dsnopek OK, now I moved it to But I can't do a appropriate test about this, the editor help (F1) can't show classes made by GDExtension currently , I'm not sure if this change is working correctly (maybe it is related to #76796) , I just create a editor script like this:
And the Output panel show the reasult:
And if the class which is inherit from |
1055435
to
763e0af
Compare
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.
Thanks! The changes for GDExtension look great.
I'm not so sure about the other changes to class_db.h
, though. None of those are actually needed for GDExtension (only the changes to class_db.cpp
), and may require wider discussion because it affects a core Godot API.
That said, I do personally like the addition of ClassDB::register_internal_class()
and GDREGISTER_INTERNAL_CLASS()
because we'll need something like that on godot-cpp side, and this way we can give godot-cpp the same API as Godot. But it may be best to leave the rest out (or at least save it for a different PR so the changes we need for GDExtension aren't delayed by discussion of the core API changes).
@dsnopek To be honest, I think the changes your mention about are not needed, too. And there already have I think the best way is remove But I guss that Let's return to this pr, do you means that only provide |
Yes, in my opinion, that would be enough. |
763e0af
to
4baf953
Compare
@dsnopek OK, change again both this pr and godot-cpp/pull/970. |
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.
Thanks, this looks great to me!
I tested this PR together with PR godotengine/godot-cpp#970 and my SG Physics 2D extension, by registering the editor plugins with GDREGISTER_INTERNAL_CLASS()
: the editor plugins still worked just fine (meaning they are registered correctly in ClassDB
), but they were no longer listed as options on the "Create New Node" dialog. So, it appears to be working great as well!
4baf953
to
41ffe54
Compare
@akien-mga Done. |
For some reason this PR likes to sometimes fail CI in the "Linux / Editor with doubles and GCC sanitizers" job with this error:
The stack trace appears to point to freeing a If I re-run the tests enough times, it does eventually pass, but it fails often enough that I'm questioning if there's something I'm missing? |
It's occuring generally even for completely trivial PRs |
I am not against this change, but it looks like it breaks compatibility, @dsnopek can you clarify why do you believe it does not? |
@reduz This adds the new The |
Thanks! |
Because of renaming from GDNative to GDExtension, I need to remake #69653. Otherwise I can't keep 1 commit.
For #970.