Skip to content

Gd for user objects: should self have Deref/DerefMut? #131

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

Closed
Bromeon opened this issue Feb 22, 2023 · 6 comments · Fixed by #370
Closed

Gd for user objects: should self have Deref/DerefMut? #131

Bromeon opened this issue Feb 22, 2023 · 6 comments · Fixed by #370
Labels
c: register Register classes, functions and other symbols to GDScript quality-of-life No new functionality, but improves ergonomics/internals

Comments

@Bromeon
Copy link
Member

Bromeon commented Feb 22, 2023

At the moment, #[derive(GodotClass)] provides a Deref and DerefMut impl on the user-defined struct, with the target type being the immediate base class.

Inside inherent methods, this allows direct access to base functionality:

#[func]
fn my_function(&self) {
    self.method();
    // instead of 
    self.base.method();
}

and outside:

let obj = Gd::new(my_obj);
obj.bind().method();
// instead of
obj.upcast::<Base>().method();

So it makes access a bit shorter. However, it also has downsides:

  1. Users aren't sure if there is a difference between self.base.method() and self.method().
    (this happened already)
  2. There may be naming conflicts/confusion between inherent methods on the user object and inherited ones from Godot, especially since methods in Object and Node tend to have very generic names such as call, set, to_string, etc. But also things like set_position tend to be quite common in gamedev-related types.
  3. It encourages users to access Godot methods inside bind/bind_mut guards, even though this is should not be necessary.
    Or should it?
    • On one hand, it's possible to access to methods via base class through reflection (e.g. call("method", args)). This must however still go through the internal RefCell, as we need to get the receiver &self/&mut self from somewhere.
    • On the other hand, this is already possible anyway, by simple upcast().

If we remove impl Deref/DerefMut on self types, we could:

  • Inside: recommend calling self.base whenever a base method is needed.
  • Outside: provide an external method like Gd::base(), Gd::upcast_direct(), Gd::upcast_one() or so that provides the immediate base class.
    Then, people could simply replace obj.bind().method() with obj.base().method().

In addition to this, we might consider if Gd<T> should be retrievable from &self/&mut self. This is already possible via workarounds. For example, store an Option<Gd<T>> inside the user object as a self-referential, late-init pointer, which is written from the outside:

let mut obj = Gd::new(user_obj);
obj.self_ptr = Some(obj.share());

Or much simpler: self.base.cast::<Self>().

Ideally we have some concrete use cases. self.connect() was mentioned, but that takes a base object, not a user one.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels Feb 22, 2023
@jrockett6
Copy link
Contributor

jrockett6 commented Jun 5, 2023

Cool!

Also, as another motivation, I have at least one case where I want my own custom impl Deref/DerefMut but am disallowed by the duplicate implementation.

@Bromeon
Copy link
Member Author

Bromeon commented Jun 6, 2023

Also, as another motivation, I have at least one case where I want my own custom impl Deref/DerefMut but am disallowed by the duplicate implementation.

Can you tell us more about that use case? Would that even work with the orphan rule?

@jrockett6
Copy link
Contributor

Sorry, this is for my own custom type that has fields that I want a Deref/DerefMut for - not something inside gdext, so I haven't run into issues with the orphan rule. Just "duplicate implementations of Deref" errors when I am using a impl NodeVirtual.

@Bromeon
Copy link
Member Author

Bromeon commented Jun 6, 2023

this is for my own custom type that has fields that I want a Deref/DerefMut for

But that could fail if bind/bind_mut fails, no?
Because Deref/DerefMut should not panic according to Rust's docs.

@jrockett6
Copy link
Contributor

jrockett6 commented Jun 6, 2023

Hmm, I may be missing something here, but the Deref that I'm looking to use (but cannot, because it is a duplicate implementation) is just on the user struct - after it has been bind already.

Maybe it is indicative of poor design on the user side, but in my code it would turn this:

Gd<T>.bind().some_object.some_property

into this:

Gd<T>.bind().some_property

@Bromeon
Copy link
Member Author

Bromeon commented Jun 6, 2023

Ah makes sense -- you have impl Deref for T, where I was thinking you'd want it for Gd<T>.

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 quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants