Simplify often-used and currently verbose APIs

[Bromeon]

Some operations that are simple in GDScript are quite verbose in godot-rust. This has mostly to do with static typing, safety, and the need to emulate inheritance, so it's partly justified. Still, I think we don't gain much if we force users to abstract these often-used chores on their own; having them in one central place would allow for code reuse and more robust code, as many contributors use it and give their feedback.

A discussion that needs to take place, is whether macros should be able to implicitly use unsafe code, which somewhat defeats the "user code must explicitly opt-in to unsafe behavior". Interesting resources in this regard:

• https://steveklabnik.com/writing/the-cxx-debate
• https://www.abubalay.com/blog/2020/08/22/safe-bindings-in-rust (elaboration on cxx)
• https://internals.rust-lang.org/t/explicitly-marking-unsafe-macro-expressions/9425
• https://www.reddit.com/r/rust/comments/ltnpr1/totallysafetransmute/

Please respond in the comments with functionality that you use on a regular basis, but is cumbersome or overly verbose with current godot-rust syntax.

[fnky]

A think I do often is store references to nodes inside my structs. Currently this requires a lot of boilerplate to setup.

A derive macro could be helpful for the most common cases of getting nodes, similar to onready in GDScript.

struct MyScript {
    #[get_node("Camera", Unique)]
    camera: Option<Camera>
}

which would expand to something like

struct MyScript {
    camera: Option<Ref<Camera, Unique>>
}

impl MyScript {
    pub fn new(_owner: &Node) -> Self {
        Self {
            camera: None
        }
    }

    // generated:
    fn _ready() {
        self.camera = unsafe { Some(get_node_as::<Camera>("Camera").unwrap()) };
    }
}

Another possibility could be to introduce a similar onready proc macro for properties:

struct MyScript {
    #[onready]
    camera: Option<Ref<Camera, Unique>>
}

Of course this introduces the issue that you mentioned regarding generating unsafe code from a macro, rather than explicitly. But I think it makes sense for godot-rust to provide macros that may generate unsafe code for convenience sake.

Macros could be prefixed or suffixed with the word unsafe to be more explicit to the user.

[Demindiro]

I'd prefix macros with unsafe_ if they use unsafe code without being able to verify whether it's actually safe.

As for verbosity: something that annoys me is the need to add owner: TRef<...> for every exported function even if I don't need it. It would be nice if I could omit it.

AFAICT it's also not possible to return Result<(), GodotError> directly and you instead need to convert it to an integer yourself.

It would also be nice if I could use into() instead of owned_to_variant(). It's a little shorter and I wouldn't have to remember one more function.

Not directly related to verbosity, but I wonder if it would make sense to implicitly convert between integers and floats? Godot implicitly converts between them when using static typing in debug/editor builds but in release builds it strips that info and no longers casts them, which can lead to some unexpected issues ("expected I64, got F64" kinda deal). Working around it would require using Variants instead (which is more verbose) or ensuring any GDScript uses int(...) whenever needed, but isn't always obvious when.

[setzer22]

Here's a few things that I found too boilerplate-y and ended up building macros / helper traits for, in no particular order:

Calling GDScript methods on Nodes

The current syntax for call is a bit too cumbersome. Due to requirements in the type system, the function has to take a slice of variants. This often requires writing .to_variant() for each of the arguments, generating unnecessary line noise. This is why I wrote a gdcall! macro.

I ended up not using this one a lot, not because it's not useful, but rather because I try to avoid using call as much as possible. However, it makes for a nice convenient alternative when I need to.

Note that it uses unsafe internally. This makes sense to me because I know when I see gdcall! it's the same as seeing unsafe { node.call(...) }, only far shorter. Lets me avoid unnecessary noise when focusing on adding a new feature. However, I'm not really sure hiding unsafe like this would be a good idea in general :thinking:

macro_rules! gdcall {
    ( $node:expr, $method:ident ) => {
        #[allow(unused_unsafe)] // Necessary because sometimes we can't check if we're already on an unsafe block.
        unsafe { $node.call(stringify!($method), &[]) } };
    ( $node:expr, $method:ident $(, $arg:expr )+) => {
        #[allow(unused_unsafe)]
        unsafe { $node.call(stringify!($method), &[ $( $arg.to_variant() , )+ ]) }
    };
}

The safe trait (Don't hate me for this, please :rofl:)

Many API methods typically return Ref<T> or even more often Option<Ref<T>>, so I'm sure everyone using godot-rust has written code like this

unsafe { some_method().expect("Something something").assume_safe() }

This is one of the most annoying parts of godot-rust IMO. I know why it's there, and I don't think there are better ways to handle this if we want to be very explicit about safety. But it's undeniable that it adds quite a bit of noise when compared to its GDScript counterpart (which in this case would simply be some_method()!). So to make the exact same operation more succint, I did this:

pub trait SafetyHelpers<N: GodotObject> {
    fn safe(&self) -> TRef<N, Shared>;
}

impl<N> SafetyHelpers<N> for Ref<N, Shared> where N: GodotObject
{
    fn safe(&self) -> TRef<N, Shared> { unsafe { self.assume_safe_unchecked() } }
}

impl<N> SafetyHelpers<N> for Option<Ref<N, Shared>> where N: GodotObject
{
    fn safe(&self) -> TRef<N, Shared> { self.as_ref().expect("Ref is some").safe() }
}

So now I can write instead:

some_method().safe()

Note that my use of assume_safe_unchecked is only because my newbie Rust skills can't help me find the right trait bounds for this to work with assume_safe :sweat_smile:

Anyway, I know this is going to be the most controversial one, but I think having to write long chains of unsafe .expect().assume_safe() adds nothing of value and is not making my codebase any safer. Godot-Rust is in a very particular situation in that it wraps a C++ codebase, so full safety is impossible, and you will always need to hold invariants (i.e. don't free stuff when Rust is not looking, ...). Anyway, If I'm going to be doing this all the time, I'd rather make it convenient and short.

Yeah, I know, I'm going straight to hell for this :fire: :fire: :fire:

Find children with type / Find descendants with type

I have functions in my codebase to iterate over all children of a node with a specific type, and a recursive version to iterate over all descendants. I think these would make for good additions in the unsafe fns in gdnative::api::helpers.

This shouldn't be used in tight loops, but is particularly helpful for caching refs at _ready time like @fnky suggested above, only by type instead of path / name.

Here's the code, as you can see it allocates and returns a Vec. That's because I didn't even know about impl Iterator<> when I wrote this, I'm pretty sure it can be made to return an iterator instead :thinking:

pub unsafe fn get_node_descendants<T>(n: TRef<Node>) -> Vec<TRef<T>>
    where T: gdnative::GodotObject + SubClass<Node> {
    let mut res = vec![];
    if let Some(n) = n.cast::<T>() {
        res.push(n);
    }
    for ch in n.get_children().iter().map(|x| x.try_to_object::<Node>().unwrap()) {
        res.append(&mut get_node_descendants(ch.assume_safe()));
    }
    return res;
}

Vector (and other elements) literal macros:

I have this in my code, and I use it a lot:

macro_rules! vec3 {
    ($x:expr, $y:expr, $z:expr) => {
        Vector3::new(($x) as f32, ($y) as f32, ($z) as f32)
    };
}

With this, I can write vec3!(0,0,0) instead of Vector3::new(0.0, 0.0, 0.0). Saves quite a few keystrokes, and the conversion to f32 is only performed when strictly necessary (casting f32 to f32 is a no-op, and any constant expression will be optimized away).

This goes a against the rule of no implicit conversions in Rust, but I've found it to be very convenient when writing code and need the occasional vector literal, and is closer (or even more succint) than its GDScript counterpart (where the Vector3 constructor also takes integers).

Other types like Vector2, Color or Rect could benefit from similar helper macros too.

Variant <-> hashmap

Not 100% on-topic, but HashMap does not implement ToVariant/FromVariant. Due to the orphan rule, there's no good way to do this on my own, I can either wrap all my hashmaps in a newtype, or specify a "to_variant_with" attribute on every hashmap field. I opted for the latter, but having this ToVariant implementations come by default would be helpful :+1:

pub fn hashmap_to_variant_as_dict<A, B>(conns: &HashMap<A, B>) -> Variant
    where A: ToVariant,
          B: ToVariant
{
    let res = gdnative::core_types::Dictionary::new();
    for (k, v) in conns.iter() {
        res.insert(&k.to_variant(), &v.to_variant());
    }
    res.into_shared().to_variant()
}

pub fn variant_to_hashmap<K, V>(variant: &Variant) -> Result<HashMap<K, V>, FromVariantError>
    where K: FromVariant + Eq + Hash,
          V: FromVariant
{
    let mut res = HashMap::new();

    let v = variant.try_to_dictionary().ok_or(FromVariantError::custom("Not a dictionary"))?;
    for k in v.keys().iter() {
        res.insert(K::from_variant(&k)?, V::from_variant(&v.get(k).to_variant())?);
    }

    return Ok(res);
}

[fnune]

In my case, I'm trying to simplify connecting a signal:

let signals = &mut owner.get_node("../Signals").unwrap();
let signals = unsafe { signals.assume_safe() };
signals
    .connect("move", owner, "_on_player_input_move", VariantArray::new_shared(), 0)
    .unwrap();

And emitting one (looks similar). The proposals here look promising.

[parasyte]

One of the papercuts I had today was trying to create a VariantArray with elements. It is easy to create an empty array, but really not obvious to populate it.

Since VariantArray::new_shared() shows up in examples in a few places, and I know I need shared references, this felt like the natural starting point. But it turns out you cannot push shared elements to a shared VariantArray. This took me embarrassingly long to come to terms with.

I ended up with something like this to fill out the exclude list on PhysicsShapeQueryParameters

let exclude = VariantArray::new();
exclude.push(&unsafe { owner.assume_shared() }.to_variant());
exclude.push(&cube.to_variant());

let query = PhysicsShapeQueryParameters::new();
query.set_exclude(exclude.into_shared());

The assume_shared() was also a surprising non-obvious requirement (and I'm still not sure if this is even correct) since owner is &KinematicBody.

I would love to see a From<&[T]> or FromIterator<T> for VariantArray<Shared>, if that's possible. Or even a macro like vec![] to help hide the boilerplate and provide an "obvious" way to use these interfaces together.

[parasyte]

This pattern happens a whole lot while creating GUI hierarchies:

let label = unsafe { Label::new().into_shared().assume_safe() };

Here it is in context:

#[methods]
impl MyPanel {
    #[export]
    fn _enter_tree(&self, parent: &Panel) {
        parent.set_custom_minimum_size(Vector2::new(256.0, 256.0));

        let tabs = unsafe { TabContainer::new().into_shared().assume_safe() };
        tabs.set_anchors_preset(Control::PRESET_WIDE, false);
        tabs.set_tab_align(TabContainer::ALIGN_LEFT);
        parent.add_child(tabs, false);

        let vbox = unsafe { VBoxContainer::new().into_shared().assume_safe() };
        tabs.add_child(vbox, false);
        tabs.set_tab_title(0, "Tab 1");

        let label = unsafe { Label::new().into_shared().assume_safe() };
        label.set_text("Contents 1");
        vbox.add_child(label, false);

        let vbox = unsafe { VBoxContainer::new().into_shared().assume_safe() };
        tabs.add_child(vbox, false);
        tabs.set_tab_title(1, "Tab 2");

        let label = unsafe { Label::new().into_shared().assume_safe() };
        label.set_text("Contents 2");
        vbox.add_child(label, false);
    }
}

Variant has a nice shortcut for this, which I mentioned above: new_shared(). I would like this method on all of the node types as well, but I'm not sure how this will play with suggestions in #808.

---

I have a macro that I am currently using for this. It's a macro because a method cannot return a reference to a temporary. It would be nicer with postfix position.

#[macro_export]
macro_rules! new_shared {
    ($object:ty) => {
        unsafe { <$object>::new().into_shared().assume_safe() }
    };
}

And its usage:

let label = new_shared!(Label);

[KennethWilke]

Chatted about this in discord a couple days ago and wanted to share some abstractions I've been tinkering with. If these would be useful to contribute I'll take any feedback and polish them up in a PR.

use gdnative::prelude::*;

#[macro_export]
macro_rules! get_tree {
    ($node:expr) => {
        unsafe { $node.get_tree().expect("could not retreive Scene Tree").assume_safe() }
    };
}

#[macro_export]
macro_rules! get_node {
    ($node:expr, $name:literal, $cast_type:ty) => {{
        let fetched: Option<TRef<$cast_type>> = unsafe { $node.get_node_as($name) };
        fetched.expect(format!("Failed to find node {:#?}", $node).as_str())
    }};
}

#[macro_export]
macro_rules! connect_signal {
    ($signal_node:expr, $signal:literal, $receiver:expr, $method:literal, $args:expr) => {
        $signal_node.connect(
            $signal,
            $receiver,
            $method,
            $args,
            0,
        )
        .expect(format!("Failed to connect '{}' signal to node {:#?}", $signal, $signal_node).as_str());
    }
}

pub fn load_scene(path: &str) -> Ref<PackedScene> {
    let loader = ResourceLoader::godot_singleton();
    loader.load(path, "PackedScene", false)
        .expect("Failed to load scene")
        .cast().expect("Failed to loaded resource to PackedScene")
}

pub fn instance_scene(path: &str) -> Ref<Node> {
    let scene = load_scene(path);
    let scene = unsafe { scene.assume_safe() };
    scene.instance(0).expect("Failed to instance scene")
}

Some example usages:

[derive(NativeClass, Debug)]
#[inherit(Node2D)]
pub struct Game {
    node: Ref<Node2D>
}

#[methods]
impl Game {
    fn new(node: TRef<Node2D>) -> Self {
        Self {
            node: node.claim()
        }
    }

    #[export]
    fn _ready(&mut self, _node: TRef<Node2D>) {
        self.load_connection_menu();
    }

    fn get_node(&mut self) -> TRef<Node2D> {
        unsafe { self.node.assume_safe() }
    }

    fn get_ui(&mut self) -> TRef<CenterContainer> {
        get_node!(self.get_node(), "canvas/ui", CenterContainer)
    }

    fn load_connection_menu(&mut self) {
        let ui = self.get_ui();

        let instance = instance_scene("res://ui/connection_menu.tscn");
        ui.add_child(instance, true);
    }
}

    #[export]
    fn _ready(&mut self, node: TRef<Node>) {
        node.set_name("Network");
        let tree = get_tree!(node);
        connect_signal!(tree, "network_peer_connected", node, "network_peer_connected", VariantArray::new_shared());

        let peer = NetworkedMultiplayerENet::new();
        peer.create_server(self.port, self.max_clients, 0, 0)
            .unwrap();

        tree.set_network_peer(peer);
        godot_print!("listening!");
    }

[Bromeon]

Thanks everyone for the great examples so far. Such condensed user feedback is rare and very helpful to identify pain points! 👍

---

I also stumbled upon a very simple, yet very interesting example: exiting.

GDScript:

get_tree().quit()

Rust:

let scene_tree = base.get_tree().unwrap();
unsafe { scene_tree.assume_safe() }.quit(0);

This is beautiful. 😬

And I don't mean the syntax, but it shows several dimensions of verbosity in an example that's just 2 identifiers in GDScript:

1. explicit access to base
2. Option::unwrap
3. unsafe + Ref::assume_safe
4. need for explicit default argument 0

Some comments on those:

1. Omitting base. won't be avoidable; at best it can be traded for self. -- I think this is not a big issue however.
2. This is a tricky one, because Node::get_tree() can indeed be null/None if the node is outside a tree. For often-used functions which return 95% of the time non-null, there could be a panicking overload, but this would need to be done on a case-by-case basis. I'm thinking of things like cast, get_node_as etc. where a null value is typically equivalent to a bug.
3. This could potentially become safe, see #808.
4. Here, it could theoretically also be possible to provide a basic method quit() and an extended quit_ext(exit_code). This might also be needed for forward compatibility. See #814 for discussion.