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

Implement callable_mp() and callable_mp_static() #1155

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Jun 29, 2023

This is the godot-cpp counterpart to PR godotengine/godot#79005 (which this PR depends on).

It very minimally works! It adds a single test case that passes :-)

However, it's a draft because:

1. I want to add more test cases to ensure it works with void functions and const methods that return a value
2. I haven't added callable_mp_static() yet

This is now ready for review and has full test coverage!

Unfortunately, the tests will fail on GitHub CI until the Godot PR is merged.

Fixes #1089

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Jun 29, 2023
@dsnopek dsnopek added this to the 4.1 milestone Jun 29, 2023
@dsnopek dsnopek requested a review from a team as a code owner June 29, 2023 17:09
@dsnopek dsnopek marked this pull request as draft June 29, 2023 17:09
@dsnopek dsnopek force-pushed the callable-mp branch 3 times, most recently from b95f34a to 46b5cf4 Compare June 29, 2023 17:17
@dsnopek dsnopek removed this from the 4.1 milestone Jun 29, 2023
@dsnopek dsnopek changed the title [DRAFT] Implement callable_mp() and callable_mp_static() Implement callable_mp() and callable_mp_static() Jun 29, 2023
@dsnopek dsnopek marked this pull request as ready for review June 29, 2023 18:43
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jun 29, 2023

callable_mp_static() is implemented in my latest push, and there's now full test coverage. So, this is ready for review!

Unfortunately, the tests will fail on GitHub CI until the Godot PR is merged.

@rburing
Copy link
Member

rburing commented Jun 30, 2023

This looks great! This will remove the need for a (registered) dummy "factory" class/method to register a physics server from a GDExtension, and allow using a static callback instead, as it's done in core.

cc @mihe for Godot-Jolt.

@ozzr
Copy link

ozzr commented Jun 30, 2023

I have been waiting for this.
The engine has way too many useful macros that should be exposed to GDExtension

@dsnopek
Copy link
Collaborator Author

dsnopek commented Jul 12, 2023

I just updated this to work with @maiself's alternative PR godotengine/godot#79005

@akien-mga
Copy link
Member

The upstream PR was merged, so this can be revisited.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Sep 19, 2023

I've just rebased this on the latest master. It should finally pass tests (which include some unit tests actually using callable_mp() and callable_mp_static()).

As discussed on this comment, I would like to expand what godot-cpp does with the new custom callable API that was added in Godot:

so the Godot PR adds the API to gdextension_interface.h to create custom callables, but the godot-cpp PR only uses that API to implement callable_mp() and callable_mp_static(), it doesn't provide a base class for making your own custom callables. But that's a good idea! When I have a chance I think I'll try to extend the godot-cpp PR to include something like that.

But I think that can be saved for a follow-up PR. What's added here is already super useful, and it doesn't get in the way of expanding the functionality later.

Copy link
Collaborator

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Reviewed during GDExtension meeting, this is looking really good!

@dsnopek dsnopek merged commit c44c3d5 into godotengine:master Sep 21, 2023
11 checks passed
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.

Expose callable_mp macros in godot-cpp?
5 participants