Skip to content

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Aug 24, 2025

Adds #[func(gd_self)] to virtual methods.

Functions annotated with this attribute are being moved from Trait Impl to Inherent Impl and are registered accordingly. (I'm not sure if I they should be renamed as well – i.e. __godot_virtual_{virtual_method_name} instead of {virtual_method_name}.

Multiple callbacks are being handled by enum/wrapper/whatever which holds the inner instance; It has been done so to keep storage and whatnot isolated. Initially I wanted to return dereferenced value (wrapper.recv()) but compiler was throwing tantrum and demanding polluting everything with lifetimes which was ultra silly – thus a respectable impl Deref<Target = T> + use<'a, T> was used instead (I think it is better design choice anyway?). Unfortunately it requires doing some silly things with std::mem::replace.

Supports everything, but errors which pop out while trying to use #[gd_self] with methods that have other receiver than self are annoying. I think I'll just hardcode all such cases later (:roll_eyes:) (so far only callbacks are "shielded").

@Yarwin Yarwin added feature Adds functionality to the library c: core Core components c: register Register classes, functions and other symbols to GDScript labels Aug 24, 2025
@Yarwin Yarwin force-pushed the add-gd-self-receiver-to-virtual-methods branch 2 times, most recently from 45f3a97 to 137aefd Compare August 24, 2025 18:55
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1282

@Yarwin Yarwin force-pushed the add-gd-self-receiver-to-virtual-methods branch from 137aefd to 885c3fc Compare August 24, 2025 19:07
@Bromeon Bromeon removed the c: core Core components label Aug 24, 2025
@Yarwin Yarwin force-pushed the add-gd-self-receiver-to-virtual-methods branch 2 times, most recently from 9ce4368 to edd3d17 Compare August 25, 2025 07:03
Copy link
Member

@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.

Thanks a lot, this is a fantastic feature addition! 💪

Comment on lines 716 to 718
type Recv: IntoVirtualMethodReceiver<Self>;
#[doc(hidden)]
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: empty line (in all places like this)

Comment on lines 180 to 188
pub enum VirtualMethodReceiverInner<'a, T: GodotClass> {
/// &self.
SelfRecv(RefGuard<'a, T>),
/// &mut self.
SelfMutRecv(MutGuard<'a, T>),
/// this: Gd<Self>.
GdRecv(Gd<T>),
/// Blank.
Uninit,
}
Copy link
Member

Choose a reason for hiding this comment

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

Should follow prior art with naming:

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub enum ReceiverType {
Ref,
Mut,
GdSelf,
Static,
}

The "Recv" is redundant with enum name "Receiver".

"Uninit" is not a valid state but a technical implementation detail needed for the swap idiom, right? Please add documentation, "blank" doesn't explain anything 😉

Copy link
Member

Choose a reason for hiding this comment

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

Also, why does it need to be pub if it's an inner type?

Comment on lines 224 to 232
// Marker structs.
// Used to extract proper type from storage and pass it to public API while defined as an associated item on the trait (`T::Recv::instance(storage)`).

#[doc(hidden)]
pub struct SelfRecv;
#[doc(hidden)]
pub struct SelfMutRecv;
#[doc(hidden)]
pub struct GdRecv;
Copy link
Member

Choose a reason for hiding this comment

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

You can make those enums, so they're uninhabitable (no instance can be created).

Naming see above, maybe to differ it from the enum values, you could use something like RecvRef/RecvMut/RecvGdSelf` or so...

@@ -255,7 +255,12 @@ fn make_forwarding_closure(
let before_method_call = match before_kind {
BeforeKind::WithBefore | BeforeKind::OnlyBefore => {
let before_method = format_ident!("__before_{}", method_name);
quote! { instance.#before_method(); }
if let ReceiverType::GdSelf = &signature_info.receiver_type {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let ReceiverType::GdSelf = &signature_info.receiver_type {
if let ReceiverType::GdSelf = signature_info.receiver_type {

It's Copy, no?

Comment on lines 683 to 688
/// #[godot_api]
/// impl INode for MyStruct {
/// #[func(gd_self)]
/// fn ready(this: Gd<Self>) {
/// godot_print!("I'm ready!");
/// }
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit out of place in the docs that introduce basic #[func]. Virtual methods with custom this should probably have their own section, and elaborate what motivates their need. There is no explanation about #[func(gd_self)] here, and since an attribute is required, we cannot make it seem the default.

#[func(gd_self)]
fn set_property(mut this: Gd<Self>, _property: StringName, value: Variant) -> bool {
this.bind_mut().some_val = value.to();
!this.bind().some_val == 4
Copy link
Member

@Bromeon Bromeon Aug 25, 2025

Choose a reason for hiding this comment

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

Do you mean != ?

This is bitwise negation...


#[func(gd_self)]
fn property_get_revert(this: Gd<Self>, property: StringName) -> Option<Variant> {
Some(Self::get_property(this, property)?.to_variant())
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid ? inside expressions, they're hard to see.

Instead, declare a variable, so ? is at the end of line.

let mut obj = VirtualGdSelfTest::new_gd();
assert_eq!(GString::from("Gd<Self>"), obj.call("to_string", &[]).to());

let ret = obj.call("get", &["".to_variant()]).to();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let ret = obj.call("get", &["".to_variant()]).to();
let ret = obj.call("get", vslice[""]).to();

#[itest]
fn test_gd_self_virtual_methods() {
let mut obj = VirtualGdSelfTest::new_gd();
assert_eq!(GString::from("Gd<Self>"), obj.call("to_string", &[]).to());
Copy link
Member

Choose a reason for hiding this comment

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

Assertions should always have actual, expected order.

For complex expressions, feel free to store the actual part in a variable.

Also below.

Copy link
Contributor Author

@Yarwin Yarwin Aug 25, 2025

Choose a reason for hiding this comment

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

Oh, I see why I changed the order – for some reason type inference works only if we have assert_eq!(concrete, infered).

(I changed them anyway, I think actual, expected is a bit more readable)

Comment on lines 192 to 210
pub fn recv_gd(mut self) -> Gd<T> {
let mut out = VirtualMethodReceiverInner::Uninit;
std::mem::swap(&mut out, &mut self.inner);

let VirtualMethodReceiverInner::GdRecv(instance) = out else {
panic!("Tried to use Gd<T> receiver for method which doesn't accept it.")
};

instance
}

pub fn recv_self(mut self) -> impl Deref<Target = T> + use<'a, T> {
let mut out = VirtualMethodReceiverInner::Uninit;
std::mem::swap(&mut out, &mut self.inner);
let VirtualMethodReceiverInner::SelfRecv(instance) = out else {
panic!("Tried to use &self receiver for method which doesn't accept it.")
};

instance
}

pub fn recv_self_mut(mut self) -> impl DerefMut<Target = T> + use<'a, T> {
let mut out = VirtualMethodReceiverInner::Uninit;
std::mem::swap(&mut out, &mut self.inner);
let VirtualMethodReceiverInner::SelfMutRecv(instance) = out else {
panic!("Tried to use &mut self receiver for method which doesn't accept it.")
};

instance
}
Copy link
Member

Choose a reason for hiding this comment

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

You can probably use mem::replace here?

Also, I was thinking to outsource common code into function/macro, but there's barely any benefit.

There is however not much reason to do let else, since the "then" case is only instance -- so I'd use either regular if let or match.

Is the panic! actually an unreachable!, indicating an internal godot-rust bug? Or can users run into it?

Copy link
Contributor Author

@Yarwin Yarwin Aug 25, 2025

Choose a reason for hiding this comment

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

Is the panic! actually an unreachable!, indicating an internal godot-rust bug? Or can users run into it?

all traits and virtual functions – which are written via macros – can be implemented manually, so theoretically it is possible 😄, albeit very unlikely.

(in other words – it is private albeit user-facing API, so I assume that it can be misused. I'm not sure if we won't use(/expose) it with builder as well (if I'll ever get myself to actually sit down and implement it 😄 ))

@Yarwin Yarwin force-pushed the add-gd-self-receiver-to-virtual-methods branch from edd3d17 to 47b59cb Compare August 25, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants