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

GDExtension: Mark virtual function as is_required in extension_api.json #93311

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jun 18, 2024

This is a draft exploring how to implement godotengine/godot-proposals#9982

TODO to finish:

  • Find all the required virtual methods and swap in the GDVIRTUAL*_REQUIRED() macro
  • Replace all GDVIRTUAL_REQUIRED_CALL() with GDVIRTUAL_CALL()

@dsnopek dsnopek added this to the 4.x milestone Jun 18, 2024
@dsnopek dsnopek requested review from a team as code owners June 18, 2024 15:10
@dsnopek dsnopek marked this pull request as draft June 18, 2024 15:10
@dsnopek dsnopek force-pushed the gdextension-required-virtuals branch 4 times, most recently from ca09f96 to 591f8fe Compare June 18, 2024 21:37
@TitanNano
Copy link
Contributor

TitanNano commented Jul 4, 2024

@dsnopek I migrated all the remaining instances of GDVIRTUAL_REQUIRED_CALL in e07dbec and removed the macro in 0590f31. You are welcome to add these commits to your PR.

EDIT: they are on this branch: https://github.com/TitanNano/godot/commits/gdextension-required-virtuals/

@dsnopek dsnopek force-pushed the gdextension-required-virtuals branch from 591f8fe to 3be645e Compare July 6, 2024 16:49
@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 6, 2024

@TitanNano Thanks! I added your changes.

After I've gone through and double-checked all the changes I'll take this out of draft.

@TitanNano
Copy link
Contributor

@dsnopek do you think this can make it into 4.4?

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 11, 2024

This would be great to get into 4.4, especially if we can get PR #86079 too. I'll try to find some time soon to dig back into both PRs.

….json`

Co-authored-by: Jovan Gerodetti <jovan.gerodetti@titannano.de>
@dsnopek dsnopek force-pushed the gdextension-required-virtuals branch from 3be645e to c2af6bc Compare September 11, 2024 21:48
@dsnopek dsnopek changed the title [DRAFT] GDExtension: Mark virtual function as is_required in extension_api.json GDExtension: Mark virtual function as is_required in extension_api.json Sep 11, 2024
@dsnopek dsnopek marked this pull request as ready for review September 11, 2024 22:01
@dsnopek dsnopek requested review from a team as code owners September 11, 2024 22:01
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 11, 2024

I just rebased, updated for new required virtual methods that have been added, and finally went back through everything and double-checked that everything that should be required is marked as required.

So, I'm taking this one of draft status now!

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

This looks great, thanks so much! I implemented the Rust part and while I haven't had the chance to test individual APIs yet, the is_required field is consistently present for all virtual methods 🙂

Maybe @TitanNano could also have another look?

One question, do you think is_override_required would be clearer than just is_required? Or clear enough from the context?

@akien-mga akien-mga removed request for a team September 24, 2024 16:26
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 24, 2024
@@ -215,6 +215,7 @@ enum MethodFlags {
METHOD_FLAG_VARARG = 16,
METHOD_FLAG_STATIC = 32,
METHOD_FLAG_OBJECT_CORE = 64,
METHOD_FLAG_VIRTUAL_REQUIRED = 128,
Copy link
Member

Choose a reason for hiding this comment

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

@reduz Can you confirm whether it's fine to add a new method flag?

Copy link
Member

Choose a reason for hiding this comment

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

Tried to ask three times but no luck, so let's merge and reduz will tell us if this was incorrect :)

@akien-mga akien-mga merged commit 8a9a26e into godotengine:master Sep 27, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

4 participants