Add implementation of Callable #231

lilizoey · 2023-04-17T17:33:19Z

Add an initial implementation of callable, mostly just wrapping InnerCallable.

bind and its relatives will crash if you attempt to use them. i experimented with a purely rust-based solution but it wouldn't work when passed to gdscript and if we're only using rust then we might as well use closures.

call and call_deferred will also crash when called.

Add a Gd::callable method to more easily make Callables from a Gd<T>, it just defers to Callable::from_object_method.

New?

We should probably have a new constructor, but what should it be? should it just create an empty callable, i.e just do Self::default, or should it call from_object_method? maybe a function like:

pub fn new<T, S>(object: impl Into<Option<Gd<T>>>, method: S) -> Self
where
    T: GodotClass,
    S: Into<StringName>,
{
    let Some(object) = object.into() else { return Self::default() };
    Self::from_object_method(object, method)
}

Other

Add vararg functions for builtin inner classes. initially i planned to use these for call and call_deferred, but it seems like these functions dont work at the moment. i believe for the same reason as #169

Add length and is_empty to StringName. is_empty is used in Callable::method_name, and it just felt weird to me to have is_empty but not length.

Bromeon

Thanks a lot for this initial implementation! Also great that you elaborate why some parts are missing as part of the docs.

It's a bit a pity that Godot allows all these half-initialized callables with implicit different null states, but I don't think there's much we can/should do about it in Rust.

We should probably have a new constructor, but what should it be? should it just create an empty callable, i.e just do Self::default, or should it call from_object_method? maybe a function like:

If there's no clear answer what the intuitive behavior for new is, that's usually a good indicator that we shouldn't have such a constructor. It's better to have named constructors with clear semantics.

Add length and is_empty to StringName. is_empty is used in Callable::method_name, and it just felt weird to me to have is_empty but not length.

Nice addition! In the future, could you split such changes in a separate commit? No need to change anything now, it's a minor detail 🙂

Bromeon · 2023-04-17T20:11:25Z

godot-core/src/builtin/callable.rs

+    /// Calls the method represented by this callable. Arguments passed should match the method's
+    /// signature. If called with more arguments than expected by the method, the extra arguments will be
+    /// ignored. If called with fewer arguments than expected it will crash. If called with arguments of the
+    /// wrong type then an error will be printed and the call fails.
+    ///
+    /// _Godot equivalent: `callv`_


Please start docs with a brief description that is not longer than one line. This one will be displayed in the overview/title of the section.

Bromeon · 2023-04-17T20:12:42Z

godot-core/src/builtin/callable.rs

+    /// Returns the name of the method represented by this callable. If the callable is a lambda function,
+    /// returns the function's name.


If the callable is a lambda function, returns the function's name.

What does that mean? Lambdas have no name, at least not from the user perspective.

Since we have Option return type, would it be possible to return None in cases where the function name is not meaningful for the user?

lambdas can in fact have names in GDScript, which is kinda counter-intuitive since that's not what lambda means. Here is an example of a lambda with a name:

func _ready(): var foo = func bleh(): pass print(foo)

this will print

bleh(lambda)

Though i realize now that the godot docs are actually wrong and trying to use get_method on a lambda does not in fact return the function's name but rather just errors.

godotengine/godot#73052 this is a known bug apparently, not sure how we should handle that in our case

Ah, I see! Regarding the bug, it's probably OK to rely on it working, hopefully it will be fixed at some point. We could maybe add a mention in the doc.

Bromeon · 2023-04-17T20:13:16Z

godot-core/src/builtin/callable.rs

+    /// Returns the object on which this callable is called.
+    ///
+    /// _Godot equivalent: `get_object`_


Should mention in which cases None is returned.

Bromeon · 2023-04-17T20:22:48Z

godot-core/src/obj/gd.rs

+
+    /// Returns a callable referencing a method from this object named `method_name`.
+    pub fn callable<S: Into<StringName>>(&self, method_name: S) -> Callable {
+        Callable::from_object_method(self.share(), method_name)
+    }


I see the convenience of this method; though over time, as we're adding more and more interactions between Gd and other types, we might want to decide where those should live (and I'd tend to "outside of Gd").

It's fine for now, but this may still change in the future.

Yeah, it's convenient for now but we should have a proper discussion of what the API for using callable should be.

Bromeon · 2023-04-17T20:27:18Z

godot-core/src/builtin/callable.rs

+    /// Returns true if this callable is a standard callable. This method is the opposite of [`is_custom`].
+    /// Returns `false` if this callable is a lambda function.
+    ///
+    /// _Godot equivalent: `is_standard`_
+    pub fn is_standard(&self) -> bool {
+        self.as_inner().is_standard()
+    }


Should we remove either is_standard/is_custom, so that the provided getter methods are actually orthogonal to each other?

Im not sure, i feel like a Callable if implemented in rust would effectively be a kind of enum, something like

enum Callable { Standard(Gd<Object>, StringName), Custom(Box<dyn CustomCallable>) }

in which case it is fairly common to have an is_x for every case.

Yeah, but at the moment it's more like

enum Callable { Standard(Gd<Object>, StringName), Invalid(Gd<Object>, StringName), // method does not exist Custom(Box<dyn CustomCallable>), Null, }

But even that is not accurate, because standard == !custom according to Godot docs.

It's just that the mental model is already quite complex with multiple states, and having redundant methods doesn't exactly help.

i guess we could remove is_standard then, since callables are generally assumed to be a standard callable, and custom callables are the exception.

Sounds good to me!
Maybe we can still mention the relation of standard/custom inside is_custom, or provide a doc alias...

we should probably provide doc aliases for all functions that diverge from the godot name tbh, but yeah a doc alias in this case at least.

Bromeon · 2023-04-17T20:27:59Z

godot-core/src/builtin/callable.rs

+    /// Returns the 32-bit hash value of this callable's object.
+    ///
+    /// _Godot equivalent: `hash`_
+    pub fn hash(&self) -> u32 {
+        self.as_inner().hash().try_into().unwrap()
+    }


Is this guaranteed not to panic? The values returned by Godot are unsigned?

according to Array::hash at least yeah. i think i checked this myself at some point too.

also if this ever breaks/changes then it's likely gonna break for every call to hash because of how hashes generally work. so we'd notice in for instance the callable_hash itest.

Bromeon

Thanks! A few more comments, then maybe squash the "feedback" commit 🙂

Bromeon · 2023-04-19T16:37:26Z

godot-core/src/builtin/callable.rs

+/// A `Callable` represents a function in Godot.
+///
+/// Usually a callable is a reference to an `Object` and a method name, this is a standard callable. See
+/// [`Callable::is_standard`]. But can also be a custom callable, which is usually created from `bind`,
+/// `unbind`, or a GDScript lambda. See [`Callable::is_custom`].
+///
+/// Currently it is impossible to use `bind` and `unbind` in GDExtension, see [godot-cpp#802].
+///
+/// [godot-cpp#802]: https://github.com/godotengine/godot-cpp/issues/802


Callable::is_standard is no longer here 😬

Bromeon · 2023-04-19T16:39:19Z

godot-core/src/builtin/callable.rs

+    /// Create a callable for the method named `method` in the specified `object`.
+    ///
+    /// _Godot equivalent: `Callable(Object object, StringName method)`_
+    pub fn from_object_method<T, S>(object: Gd<T>, method: S) -> Self


method_name would be clearer as parameter name, especially since the type is generic.

Doc could then just be somthing like

/// Create a callable for the method `object::method_name`.

Bromeon · 2023-04-19T16:40:39Z

godot-core/src/builtin/callable.rs

+    /// Calls the method represented by this callable.
+    ///
+    /// Arguments passed should match the method's signature. If called with more arguments than expected by
+    /// the method, the extra arguments will be ignored. If called with fewer arguments than expected it will
+    /// crash. If called with arguments of the wrong type then an error will be printed and the call fails.
+    ///
+    /// _Godot equivalent: `callv`_
+    pub fn callv(&self, arguments: VariantArray) -> Variant {
+        self.as_inner().callv(arguments)
+    }


Error conditions are a bit vague:

"it will crash" -- UB or what?

"the call fails" -- panic? NIL returned?

It's also not clear what happens when the callable is null (empty) or invalid (bad method name).

Could maybe be worth adding as tests too. These things tend to change in Godot patch versions 😁

the ones that can be tested without crashing our integration tests entirely are tested now. also we can't check that an error is printed in godot.

That's more than enough, thanks a lot! 👍

Bromeon · 2023-04-19T16:43:49Z

godot-core/src/builtin/callable.rs

+        // Increment refcount because we're getting a reference, and `InnerCallable::get_object` doesn't
+        // increment the refcount.
+        self.as_inner().get_object().map(|object| {
+            <<Object as GodotClass>::Mem as crate::obj::mem::Memory>::maybe_inc_ref(&object);


I don't think both expansions (as GodotClass, as Memory) are necessary -- we have statements like

<engine::Object as GodotClass>::Declarer::scoped_mut(...)

in other places 🙂

Bromeon · 2023-04-19T16:45:49Z

godot-core/src/builtin/callable.rs

+    /// Returns the ID of this callable's object, see also [`Gd::instance_id`].
+    ///
+    /// Returns `None` when this callable doesn't have any target to call a method on.
+    ///
+    /// _Godot equivalent: `get_object_id`_


Also None when the state is invalid?

Bromeon · 2023-04-19T16:48:15Z

godot-core/src/builtin/callable.rs

+        // Equality depends on equality of the object/custom object of the callable's target.
+        // Which may be partial, so we cannot be sure that equality of Callable is total.
+        PartialEq => callable_operator_equal;


Is it though?

We have a blanket

impl<T: GodotClass> Eq for Gd<T> {} impl<T: GodotClass> PartialEq for Gd<T> { /// ⚠️ Returns whether two `Gd` pointers point to the same object. /// /// # Panics /// When `self` or `other` is dead. fn eq(&self, other: &Self) -> bool { // Panics when one is dead self.instance_id() == other.instance_id() } }

So equality does not depend on contents of T.

i think it is actually a total equality yeah, but custom callables in godot are free to implement equality however they want. from what i can tell it is always either just a pointer equality or object + method equality (and if references to two custom callables are the same pointer then they are always equal). but im not sure if godot guarantees that all custom callables in godot will be implemented to have a total equality.

I see, but the current comment says it depends on the object (which doesn't make clear that they are free to implement however they want).

fair, i wrote object/custom object where custom object was meant to refer to custom callable objects. but i can be more clear about that

Bromeon · 2023-04-19T16:51:45Z

itest/rust/src/callable_test.rs

+    assert!(obj.callable("foo").is_valid());
+    assert!(Callable::default().is_null());
+    assert!(!Callable::default().is_custom());


Could you also check all 3 methods for these 2 cases?

I.e. we would have 3x3 assert statements in total.

im also adding .object().is_some()/is_none()

lilizoey · 2023-04-19T19:42:53Z

bors r+

bors · 2023-04-19T19:51:44Z

Build succeeded:

full-ci

233: Refactor `GodotString`, `NodePath`, and `StringName` r=lilizoey a=lilizoey Creates a new module in `builtin` that contains all the string-related modules. Make all the string-files look more similar where it made sense. And add things that only existed in one of the three but not the others, such as: `Hash` impl (now using godot's hashing), new-constructor, conversions. Added pass-by-value From impls in addition to pass-by-reference From impls, so we can do just ```rs let node_path: NodePath = string.into(); ``` instead of needing to do ```rs let node_path: NodePath = NodePath::from(&string); ``` Moves `String`-specific stuff into `builtin/string/mod.rs` and renamed `string.rs` to `godot_string.rs`. And adds a `VariantMetadata` impl for `String`. Adds some more tests to test all the types a bit more extensively. Since this is gonna conflict with #231 (as i added some stuff to StringName there) i wanna wait with merging this until that is merged. but otherwise the PR is ready to be merged. Co-authored-by: Lili Zoey <lili.andersen@nrk.no>

lilizoey mentioned this pull request Apr 17, 2023

Project status overview #24

Open

lilizoey added feature Adds functionality to the library c: core Core components labels Apr 17, 2023

Bromeon reviewed Apr 17, 2023

View reviewed changes

lilizoey mentioned this pull request Apr 18, 2023

Refactor GodotString, NodePath, and StringName #233

Merged

Bromeon reviewed Apr 19, 2023

View reviewed changes

lilizoey force-pushed the feature/callable branch from bb48d74 to 7cb77f2 Compare April 19, 2023 18:40

Add implementation (wrapping InnerCallable) of Callable

7c786cd

lilizoey force-pushed the feature/callable branch from 7cb77f2 to 7c786cd Compare April 19, 2023 18:46

Bromeon approved these changes Apr 19, 2023

View reviewed changes

bors bot merged commit 21f95f7 into godot-rust:master Apr 19, 2023

lilizoey mentioned this pull request Jun 14, 2023

Tracking issue for builtin feature-completeness #310

Open

33 tasks

		/// Returns the name of the method represented by this callable. If the callable is a lambda function,
		/// returns the function's name.

Add implementation of Callable #231

Add implementation of Callable #231

Conversation

lilizoey commented Apr 17, 2023 • edited Loading

New?

Other

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey commented Apr 19, 2023

bors bot commented Apr 19, 2023

lilizoey commented Apr 17, 2023 •

edited

Loading

lilizoey Apr 17, 2023 •

edited

Loading

lilizoey Apr 19, 2023 •

edited

Loading