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

Core: Allow methods of built-in Variant types to be used as Callables #82264

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Sep 24, 2023

Makes the following code work:

var array: Array = [1, 2, 3]
print(array) # [1, 2, 3]
var callable: Callable = array.clear
callable.call()
print(array) # []

Note that signals can still only be connected to Object-based callables. Fixed by #82695.

@adamscott adamscott requested a review from vnen September 26, 2023 13:32
doc/classes/Object.xml Outdated Show resolved Hide resolved
@dalexeev dalexeev force-pushed the core-builtin-methods-as-callables branch from 75e4f09 to b042636 Compare October 25, 2023 13:46
@dalexeev
Copy link
Member Author

dalexeev commented Oct 25, 2023

It makes sense to change the argument type in the constructor from Object to Variant, but I think it's hard to do with the current system. The only thing we can do is declare constructors for all Variant types. It might make sense to add a constructor only for the Dictionary, since it does not support methods as callables due to ambiguity with properties. Or add a static method, which is much easier to bind.

@akien-mga
Copy link
Member

akien-mga commented Oct 28, 2023

This is quite important IMO, as currently there is no proper type safe way to do call_deferred on built-in Variant type methods.

See #84046 (comment) for context, where currently a lambda needs to be used instead of doing some_signal.emit.call_deferred() to emit a signal from a thread.

BTW if this works in this PR, this could be worth adding as a unit test.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

No apparent issues to me. TIWAGOS, though.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Nov 2, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jan 2, 2024

I found an issue with free() (#86706), which seems related to this PR, but it doesn't fix that bug.

Note that signals can still only be connected to Object-based callables.

Does not work with free().

@akien-mga akien-mga merged commit b5c6e87 into godotengine:master Jan 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the core-builtin-methods-as-callables branch January 2, 2024 17:12
@nlupugla
Copy link
Contributor

Hi folks!

I ran into this commit while working on GDScript Structs #82198 and worried there might be an issue with this feature and built in Variants with dynamic properties, that is, Dictionaries and eventually Structs (which are just fancy arrays).

For example if I run

var dict: Dictionary = {&"clear" : "oops"}
var callable: Callable = dict.clear

The analyzer does not complain, but at runtime, I get "INTERNAL ERROR: Trying to assign value of type 'String' to a variable of type 'Callable'."

At the moment, my Struct branch is failing the unit test that was added for this feature:

var array: Array = [1, 2, 3]
print(array) # [1, 2, 3]
var callable: Callable = array.clear
callable.call()
print(array) # []

in the same way that the dictionary example I gave above fails as I have added named getters to arrays so that struct fields can be accessed like my_struct.my_field.

I'm not entirely sure what the best solution would be here, but one easy and temporary fix would be to change the test so that it tests using a method from a different Variant type as a callable, say, Vector2. For example,

var vec: Vector2 = Vector2(1.5, 2.5)
print(vec)
var callable: Callable = vec.ceil
print(callable.call())

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.

GDScript: "Cannot find property emit on base Signal"
5 participants