Allow custom receivers in virtual methods

[lilizoey]

Currently most virtual methods take &mut self as the receiver, this can be problematic if you want to call multiple virtual methods at once from godot. We could loosen this restriction.

One way to do this would be to make all virtual methods take Gd<Self> instead as the first argument, and have the #[godot_api] macro rewrite the function slightly to take the appropriate reference.

This would also allow you to workaround #338. Especially if we allow the user to specify Gd<Self> as the receiver.

[gg-yb]

This could also mitigate #302 to an extent

[Bromeon]

One way to do this would be to make all virtual methods take Gd<Self> instead as the first argument, and have the #[godot_api] macro rewrite the function slightly to take the appropriate reference.

It can be argued that since bad runtime borrows are a common source of confusion, having an explicit

fn ready(this: Gd<Self>) {
   let this = this.bind_mut();
   ...
}

would be clearer. But I don't fully like this, as it makes code less ergonomic and punishes all the common cases like ready() and process(), which are usually perfectly fine.

I was also considering that &self or &mut self receivers could still be valid, but would be desugared by the proc-macro to this.bind() and this.bind_mut(). Problem is that it's not possible to use the self modifier, so even the function body would need to be rewritten, which would likely be very involved with weird edge cases... 🤔

[lilizoey]

I was also considering that &self or &mut self receivers could still be valid, but would be desugared by the proc-macro to this.bind() and this.bind_mut(). Problem is that it's not possible to use the self modifier, so even the function body would need to be rewritten, which would likely be very involved with weird edge cases... 🤔

We could just have it desugar to something like

impl NodeVirtual for MyClass {
  fn ready(this: Gd<Self>) {
    Self::ready(this.bind())
  }
}

impl MyClass {
  fn ready(&self) {
    // code
  }
}

Idk the exact syntax to make the name resolution work properly here but yeah.

[gg-yb]

Regarding #338, I'm not sure if this would allow you to workaround the situation described therein.

fn ready(this: Gd<Self>) {
  this.bind_mut().add_child(...); // Mutable borrow of Gd<Self>
}

fn on_notification(this: Gd<Self>, n: ...) {
  // do something with this, either bind / bind_mut -> panic due to aliasing
}

on_notification would still be required to not touch this, as far as I can see

[Ogeon]

Shouldn't it work if this is upcast to Node first? It's not pretty or obvious, though...

fn ready(this: Gd<Self>) {
  this.upcast::<Node>().add_child(...); // Uses DerefMut, since Node is an engine class
}

fn on_notification(this: Gd<Self>, n: ...) {
  // do something with this, either bind / bind_mut -> no panic?
}

[Bromeon]

Shouldn't it work if this is upcast to Node first? It's not pretty or obvious, though...

Great observation! In fact, that's one of the main motivations for #131. I'll probably implement that soon.

[LeaoLuciano]

Maybe the user can optionally choose to impl Gd<MyClass> instead of impl MyClass:

#[godot_api]
impl NodeVirtual for Gd<MyClass> {
  fn ready(self) {
    ...
  }
}

[Bromeon]

Maybe the user can optionally choose to impl Gd<MyClass> instead of impl MyClass:

Interesting idea, but that would mean:

• all functions need to take the same receiver type, instead of deciding it per-function
  • unless we allow multiple #[godot_api] blocks, but that comes with its own issues
• since the trait itself has methods defined on &self/&mut self (not the value self), this would need to be done here too, which is a bit weird 🤔

[mhoff12358]

It doesn't address the case of existing interfaces, but I was able to throw together #396 to allow user defined methods to take this: Gd<Self> instead of &self as the first argument to a non-static method.

[andreymal]

Just ran into this issue when I tried to test #883

#[derive(GodotClass)]
#[class(editor_plugin, tool, init, base=EditorPlugin)]
pub struct MySuperEditorPlugin {}

#[godot_api]
impl IEditorPlugin for MySuperEditorPlugin {
    fn enter_tree(&mut self) {
        let mut editor_button = Button::new_alloc();
        editor_button.connect(
            "pressed".into(),
            Callable::from_object_method(
                self, // ???????
                "editor_button_pressed_event",
            ),
        );
    }
}

So I have to make an (unnecessary?) "inner" object just to handle signals

[snakefangox]

Could we provide both methods in the trait and then have the macro handle the wiring? So for example:

trait INode {
    fn ready(&mut self,) { unimplemented !() }
    fn ready_gdself(this: Gd<Self>) { unimplemented !() }
}

And then have the macro generate the __virtual_call function to use the one the user implemented (or error if both were implemented).

The biggest downside I can see with that is it makes auto-complete a bit messy, we could get around that by adding a second interface trait with both methods, something like INodeWithGdself and keeping INode unchanged. Then we can auto-impl INodeWithGdself for all INode to make generics easier and so that implementing both is a compile error. In terms of issues there, I guess there is function name resolution when both traits are in scope but given the interface traits are really only for implementing that should be fine. Also code size, we could probably put this behind a feature gate fairly easily if that's a problem?

[Bromeon]

@snakefangox sorry, I missed your comment earlier.

While this is an option, it would generate double the number of functions across all interface traits. We already duplicate every base interface method in all its directly and indirectly derived interfaces, and this would further compound it -- with an effect on doc discoverability, compile times, IDE completion etc.

Two separate traits would be worse though: the user could no longer decide on a per-method level whether they'd want a reference or Gd to self, and adding one "gdself" method would require refactoring the whole impl.

Feature gates are an option, but they also contribute to combinatoric explosion of testing and things that can go wrong, so I'm very hesitant to make parts configurable that don't need to be.

---

That said, I don't have a great solution either. One option would be to make all trait methods take this: Gd<Self> and let the macro desugar &mut self into an initial let this = this.bind_mut() statement, similar to this but in the same body. However that's possibly also confusing for IDEs and would mean that IDE-generated methods would all have the Gd signature instead of the reference one. But it might still be worth trying.

Maybe worth noting that the double-borrow issue happens mostly in re-entrancy scenarios such as

• user_fn calls an engine API, which triggers a notification
• this notification calls on_notification, which attempts to borrow already-borrowed &mut self and fails

And in such cases, it's possible to migitate the problem on the call site with an explicit self.base_mut() lock held across the call -- even in situations where it's not necessary, see e.g. #1157 (comment).

---

Given that the issue has existed for quite a while, it would be interesting to hear how users have worked around this so far.

[Yarwin]

Honestly, the longer I think about it, the more it makes sense to use a Gd receiver for just one virtual method inherited by all of Godot's Objects – notification.

That said, this is entirely arbitrary.

---

Given that the issue has existed for quite a while, it would be interesting to hear how users have worked around this so far.

In some cases, the workaround is obvious. For example, we can await the ready signal, which is emitted after the ready method (when our instance is no longer bound):

    fn ready(&mut self) {
        let mut this = self.to_gd();

        godot::task::spawn(async move {
            let ready = this.signals().ready();
            let _ = ready.into_future().await;
            let mut state = Box::new(NewTurnState);
            state.enter(&mut this);
            this.bind_mut().state = Some(state);
        });
}

(we can also connect to it with one-shot flag)

However, this isn’t an option for all virtual methods.

Some cases can be handled with call_deferred, which postpones method execution until the end of the frame (e.g., self.base().call_deferred("the_real_physics_process", &[])).

If strict execution order is necessary processing can be moved to a Godot singleton (be it autoload singleton or engine singleton). For example, an AIServer might wait until the end of the frame and then trigger signals that are received by methods with a Gd receiver.