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 CallableCustom that devs can use in their GDExtensions #1280

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Oct 22, 2023

In PR #1155, we added the ability to use callable_mp() and callable_mp_static() to create custom callables for C++ function pointers.

However, it would also be great to allow developers to create their own sort of custom callables. For example, imagine a GDExtension that adds a scripting language with lambda's, and being able to create callables out of those lambdas. (See this discussion on the Godot Lua API issue queue.)

This PR aims to add a CallableCustom class, similar to the CallableCustom class in the engine, which developers can extend to create a new kind of custom callable.

It renames some of the functions added in PR #1155 to make it easier to distinguish which go with which kind of custom callable.

It's a draft because I haven't actually tested it yet :-)

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Oct 22, 2023
@dsnopek dsnopek added this to the 4.x milestone Oct 22, 2023
@dsnopek dsnopek requested a review from a team as a code owner October 22, 2023 04:19
@dsnopek dsnopek marked this pull request as draft October 22, 2023 04:19
@dsnopek dsnopek force-pushed the callable-custom branch 2 times, most recently from c1f3235 to b733a16 Compare October 22, 2023 14:23
@dsnopek dsnopek marked this pull request as ready for review October 22, 2023 14:23
@dsnopek dsnopek changed the title [DRAFT] Add CallableCustom that devs can use in their GDExtensions Add CallableCustom that devs can use in their GDExtensions Oct 22, 2023
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 22, 2023

I just added a automated test that tests the functionality this PR adds! So, this should now be ready for review :-)

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Trey2k
Copy link
Contributor

Trey2k commented Oct 22, 2023

I am in the process of testing these changes for my extension now. So far it seems great with a couple of notes that make using the same code base for module and GDExtension builds slightly annoying.

  • call method
    This one is not too big of a deal as I think this happens a lot with the error type in godot-cpp, but the type of r_call_error in a module would be a Callable::CallError but GDExtensionCallError is used here instead.
  • is_valid
    For some reason currently is_valid is a virtual in godot-cpp. This is not the case in the base engine.
  • get_object
    The godot-cpp version of this method returns an actual Object* where in the base engine we are only returning the instance ID of the object.
  • Callable class its self
    Another very important thing for my extension is the ability to get a CallableCustom from a callable. In module builds we use the Callable::get_custom method but this is not present in godot-cpp currently

Besides these small annoyances overall the changes seem great so far.

Edit:
One I forget to mention was the compare_less and compare_equal. In godot-cpp these are currently returning a GDExtensionBool instead of a bool. Causing some extra ifdefs.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 22, 2023

@Trey2k Thanks for testing and the feedback!

Yeah, it's tricky to match the API exactly to what's in the engine.

  • call method
    This one is not too big of a deal as I think this happens a lot with the error type in godot-cpp, but the type of r_call_error in a module would be a Callable::CallError but GDExtensionCallError is used here instead.

This one we may be able to fix in the future. We don't have Callable::CallError in godot-cpp, but we could certainly add it! This should probably be its own PR, though, because we'd have to update all the other places we're using GDExtensionCallError.

  • is_valid
    For some reason currently is_valid is a virtual in godot-cpp. This is not the case in the base engine.

It is virtual in the engine too, it's just not pure virtual, it has a default implementation. I suppose we could add a similar default implementation? I'll see what I can do.

  • get_object
    The godot-cpp version of this method returns an actual Object* where in the base engine we are only returning the instance ID of the object.

This was a judgement call I made: the API in gdextension_interface.h takes an Object *, and so we could have get_object() return ObjectID, but then we'd have to look up the Object * anyway, and I know that on the other side Godot is going to turn that back into an ObjectID, so it'd be a useless lookup. The only good fix here is to change the API in gdextension_interface.h to take an ObjectID, which maybe we could try to sneak in before Godot 4.2?

  • Callable class its self
    Another very important thing for my extension is the ability to get a CallableCustom from a callable. In module builds we use the Callable::get_custom method but this is not present in godot-cpp currently

Ah, we could add this! We have a function to do it in gdextension_interface.h. I'll add it when I have a chance.

One I forget to mention was the compare_less and compare_equal. In godot-cpp these are currently returning a GDExtensionBool instead of a bool. Causing some extra ifdefs.

I think this one is unavoidable. Those function pointers are passed to the Godot side, and we can't pass around function pointers that use bool because it's size is undefined - it could be different depending on the compiler, which is why we have GDExtensionBool.

@maiself
Copy link
Contributor

maiself commented Oct 23, 2023

Some of this has already been mentioned, but here are my findings and thoughts. (please forgive formatting and abbreviations, on mobile, may edit later)

  • typedef for compare funcs have different return types

    • solvable by implementing cmp funcs as static funcs which call the virtual ones
    • maybe also some template metaprograming to selectively do the above, skipping if types are same at abi level
  • core has default impl for is_valid()

  • call() has different error out arg type, Callable::CallError in core, PR has GDExtensionCallError

    • should probably add a Callable::CallError to godot-cpp
  • get_object() returns ObjectID in core, PR has Object*

    • core also has some wierdness around this, Callable::get_object() returns Object*, CallableCustom::get_object() returns ObjectID

    • core Callable also has a get_object_id()

    • core Callable::get_object() always calls ObjectDB::get_instance() on the ObjectID...

    • core uses ObjectID as its safer, if the id is valid then you get nullptr when converting, but if you were to use Object* you have no way of knowing if the object is still valid (dangling pointer)

    • gde interface uses object only to get its ObjectID

    • object_get_instance_id() exposed to gde interface directly, so it shouldnt be much harder to create custom callables if the type is changed...

    • core callable_mp converts ptr -> id alot...

    • my python bindings dont even use object, its set to nullptr in all cases...

    • given the above I don't really see a reason against changing the type from object ptr to object id, as long as we can do it now before 4.2...

  • if get_custom() is called on a callable_mp any calls to its members will lead to a crash. either callable_mp needs to inherit CallableCustom or a different token should be used to exclude callable_mp from get_custom

@dsnopek dsnopek marked this pull request as draft October 25, 2023 14:07
@dsnopek dsnopek force-pushed the callable-custom branch 4 times, most recently from 240543e to f15ec7d Compare October 25, 2023 15:20
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 25, 2023

Alright! The latest version of this PR has the following updates:

  • get_object() now returns ObjectID
  • There's a default implementation of is_valid(), although, I have mixed feelings about it. It's the same default implementation as in Godot, but I feel like it's not as good a default from GDExtension, since in most cases you don't care about the object.
  • Following @maiself's suggestion, the CompareEqualFunc and CompareLessFunc now return bool, by having an intermediate function that redoes some of the logic from Callable::operator==() and Callable::operator<() from Godot.
  • The two types of custom callables possible in godot-cpp (ones from callable_mp() or descending CallableCustom) now share a base class, so that Callable::get_custom() can safely return only valid CallableCustom * objects
  • I also needed to work around some cyclic includes to get ObjectID in all the right places, so its definition has moved to a new file, and there's some code to prevent generating a duplicate struct ObjectID from the "native_structures" in extension_api.json

@dsnopek dsnopek marked this pull request as ready for review October 25, 2023 15:31
@Trey2k
Copy link
Contributor

Trey2k commented Oct 25, 2023

There's a default implementation of is_valid(), although, I have mixed feelings about it. It's the same default implementation as in Godot, but I feel like it's not as good a default from GDExtension, since in most cases you don't care about the object.

I think this default is fine in my opinion, my extension the object is the lua VM its self, if its valid the method will still be valid as methods pulled as a Callable get copied. And if the object is not the determining factor then it can always be overridden.

@dsnopek dsnopek force-pushed the callable-custom branch 4 times, most recently from 29bd975 to 91eea26 Compare October 25, 2023 20:14
@dsnopek
Copy link
Collaborator Author

dsnopek commented Nov 16, 2023

@Trey2k Can you confirm if the latest version of the PR works for you? Looking PR WeaselGames/godot_luaAPI#180 for your bindings, it seems you're still using the older version, before it got rewritten to more closely resemble the Godot API.

@Trey2k
Copy link
Contributor

Trey2k commented Nov 17, 2023

@Trey2k Can you confirm if the latest version of the PR works for you? Looking PR WeaselGames/godot_luaAPI#180 for your bindings, it seems you're still using the older version, before it got rewritten to more closely resemble the Godot API.

Just updated the PR. New changes seem to work great! Thanks for the work you have done on this.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Nov 17, 2023

@Trey2k Thanks so much for testing!

I think I'm going to "live dangerously" and go ahead and finally merge this one :-)

@dsnopek dsnopek merged commit 4439a4a into godotengine:master Nov 17, 2023
12 checks passed
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Dec 1, 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