Construct object from custom native class

[ghost]

This patch allows new objects with NativeClass script instances attached to be constructed and returned from Rust.

Depends on #187. Implements #154. Fixes #182.

This is a fairly large patch touching many parts of the package. Explanations for each commit are available in commit messages. Each commit should individually compile and pass tests when applied sequentially.

Motivation

When creating an game, it's often desired to return new objects with custom Rust behaviors attached. For example:

• Custom nodes.
• yield-able objects like GDScriptFunctionState, to avoid blocking the main thread for computationally intensive operations.
• Custom data structures, to avoid expensive wholesale serialization of state into GDScript types.
• Value types with operations defined in Rust.

This is not previously possible in godot-rust, because as it stands, ToVariant can't be implemented for NativeClass:

1. NativeClass instances no longer carry the native class header, and can be constructed freely without an underlying Godot object. There is nothing to put in the resulting Variant.
2. Further, there is no exposed way to construct an instance with an underlying Godot object, at all.
3. from_variant is required for ToVariant, and there is no sensible way to implement it for NativeClass instances, because user_data is just a Box<RefCell<T>> with no run-time type information.

This patch changes the API in a way so (1) and (2) can be resolved, and (3) can be worked around.

Explanation

This patch does 3 major API changes that are required for the feature to work:

1. Replaced the unused NativeRef type with Instance, that wraps around Godot objects with Rust script instances.
2. Introduced the trait Instanciable: GodotObject as a generic bound for type safety.
3. Made user-data wrapper customizable, and the default one safe.

Instance API

This is a safer wrapper (compared to a plain tuple or the original NativeRef) around a Godot object and a Rust NativeClass implementation. It can be used in the following ways:

struct Foo;
impl NativeClass for Foo {
    type Base = Reference;
    // - snip -
}
#[methods]
impl Foo {
    #[export]
    fn answer(&mut self, owner: Reference) -> i64 {
        // - intricate algorithm -
        42
    }

    /// Use as return value
    #[export]
    fn fooinate(&mut self, owner: Reference) -> Instance<Foo> {
        Instance::<Foo>::new() // Creates a Reference with Foo attached
    }
}

let foo = Instance::<Foo>::new(); 
assert_eq!(42, foo.apply(|foo, owner| foo.answer(owner))); // apply clones the reference,
assert_eq!(42, foo.apply(|foo, owner| foo.answer(owner))); // so it's safe to do it again

let foo = Instance::<Foo>::new();
assert_eq!(42, unsafe { foo.apply_aliased(|foo, owner| foo.answer(owner)) }); // non-reference-counted types can be unsafely aliased if you promise it's valid and you won't free it

let base: Reference = Instance::<Foo>::new().into_base(); // upcast to base type
let variant: Variant = Instance::<Foo>::new().to_variant(); // or convert to Variant

assert_eq!(Some(42), unsafe { base.call("answer".into(), &[])) }.try_to_i64()); // it still works

Instanciable

This trait is implemented by the bindings generator for all instanciable classes, and used on Instance::new as a generic bound on T::Base, making sure that only NativeClass instances that extend instanciable base classes can be used.

The trait has one function, construct, that takes no arguments and return a fresh object. It is safe, unlike its super trait GodotObject, because it's always valid to create a new instance.

Instance::new

As an implementation detail, the function works by:

• On gdnative_init, save a pointer to the GDNativeLibrary being initialized passed through godot_gdnative_init_options, as a static in gdnative-core.
• When Instance::new is called, construct a NativeScript instance using said GDNativeLibrary and NativeClass::class_name.
• Construct a T::Base with Instanciable::construct.
• Call Object::set_script on the base object, with the NativeScript instance as argument.
• Take the user data from T::Base with godot_nativescript_get_userdata, and clone into a NativeInstanceWrapper<T>.
• Put both the T::Base and the NativeInstanceWrapper<T> in an Instance struct.

The function is implemented using direct GodotApi calls, to keep it all in gdnative-core and avoid depending on gdnative-bindings.

Customizable user data storage

See conversation in #182.

Drawbacks

• Cognitive complexity: there is a distinction between a T: NativeClass and an Instance<T>. The former is just an inert Rust value, and only the latter actually works in the engine. Users need to be taught about this.
• Ugly raw API code in gdnative-core depending on NativeScript classes. Not aware of a better way to do this.
• Performance: Calls to NativeClass functions from Godot has Rc overhead, but is necessary to uphold safe invariants.

Rationale and alternatives

I consider this design better than the alternatives below because:

• Usable with only dependency on gdnative-core.
• Possible to replace some raw API code in macros with Instance calls.
• Relatively small change on API and internal workings.

Putting Instance::new outside gdnative-core

It's possible to put the function into a new crate, e.g. gdnative-utils, which will depend on both gdnative-core and gdnative-bindings. However, this has multiple problems:

• One more crate to publish and keep track of.
• Less consistent API: users would expect something like new to be available under the type it constructs.
• In case a future, more ergonomic way to create instances is made available in Godot, the new crate would be useless.

Re-introducing manually managed headers

• The ergonomic loss is too big, and it would require big (backwards) changes in inner workings.

Having only GDNativeLibrary::this_library

With only GDNativeLibrary::this_library, construction is possible. However, without the API Instance<T> provides, the code to construct and use custom instances would be very clumsy, involve unsafe, and have little type safety. It would look something like:

// T: NativeClass
let library = GDNativeLibrary::this_library();
let mut script = NativeScript::new();
script.set_class_name(T::class_name().into());
script.set_library(Some(library));

let variant = script::_new(&[]);
let base = variant.try_to_object::<T::Base>().unwrap();

let answer_variant = unsafe { base.call("method_name".into(), &[])) };
let answer = answer_variant.try_to_i64().unwrap();
// actually use answer

Versus:

let instance = Instance::<T>::new();
let answer: i64 = instance.apply(|t, owner| t.method_name(owner));
// actually use answer

Not doing this at all

• Won't be able to construct object from custom native class.

Unresolved questions

Better way to put a "one-off" function into bindings?

Currently GDNativeLibrary::this_library is "generated" by just writing a string into the generated file. This feels rather contrived. Is there a better way to do this (or not do this at all)?

Creating an manually managed Instance and not passing it to the engine / freeing it leaks memory.

There is probably no way to handle them properly without some form of ownership model, as non-Reference GodotObjects drop by forgetting. Refer to #70.

Should we implement Send (or even Sync) for Godot types?

Raw pointers are neither Send or Sync in Rust. This means that for all NativeClass that holds some reference to a Godot object, Send or Sync need to be manually implemented for any of the safe wrappers to be used.

Godot's C++ code contain plenty of locks, but there is no comprehensive information available about the thread safety for all the types.

I think Sync is certainly being too confident. Is it reasonably safe to just assume that everything is at least Send, since there're nothing we can do about it anyway?

Future possibilities

Should we implement FromVariant for Instance, and how?

Currently, it's not possible to implement FromVariant because the user data is just a Box<RefCell<T>> with no type information. This is acceptable for methods, where we trust NativeScript to call the correct functions, but unacceptable for FromVariant.

Further complicating the problem, is the situation that the current implementation of godot_nativescript_get_userdata in Godot only checks that the script is any NativeScript: that is, not necessarily from this library or even language. Thus, trying to read a 8-byte TypeId, or rather, anything at all from it may lead to a swift segfault.

It may be feasible to track all user data pointers and their TypeIds in a static HashMap, but that may incur extra cost for types that don't need to be FromVariant. Maybe we could introduce a separate marker trait users can explicitly opt into, and implement FromVariant for those types through a wrapper newtype?

This also prevents safe downcasting from T::Base to Instance<T>.

Can we add a more ergonomic constructor to the NativeScript API itself?

The current Instance::new implementation is ugly, and Godot is open source. Maybe this could be changed somehow in the future.

[ghost]

Force-push(es):

• Updated implementation of Instance::new so instances with non-Reference base classes can be constructed.
• Added fix to Object::set_script on owner is UB #182.
• Derive traits and implement Clone for Instance.
• Removed Instance::once because it's unsound: it's possible to store an Instance somewhere and end up with an invalid pointer later.

[karroffel]

Thanks a lot for this PR! This is quite some code to review and I already started, but I will probably need some time to go through all of it.

[ghost]

Updated with implementation of customizable user-data as per discussion in #182.

[karroffel]

Wew tons of code! 😅 Really good job so far, I like it!

[karroffel]

Is this notice still true when considering that this allows multiple mutable aliases?

[ghost]

You can't make multiple mutable aliases of the NativeClass because the reference is only valid for the closure. If you call map_mut again inside, the user data wrapper should prevent it.

The _aliased part refers to the Godot object, and for Reference types from_sys(to_sys()) is indeed (currently) equivalent to a clone.

[karroffel]

This is just personal preference, but I would probably prefer to have the default be the single-threaded version (so RefCell based). Most scripts will not be used in multiple threads so this might be more costly than it is useful in the general case.

[karroffel]

(After finishing reviewing I realized single-threaded versions don't exist, so feel free to ignore this)

[ghost]

It is true that _physics_process and _process happen on the same thread, but signals connections seem to get called immediately unless they are CONNECT_DEFERRED. That's quite some chance to get exposed to threads, imo.

parking_lot mutexes need only one atomic operation for uncontended locks, so it may not be as scary as it sounds, but a feature flag should be useful, as said in replies to other comments on this topic.

[karroffel]

This is a really nice summary, nice job!

We should probably add single-threaded versions of this later on, that way you don't have any overhead due to locks or atomics when using a NO_THREADS build of Godot or know in advance that no access via threads will be done.

[ghost]

I think that is a good idea, combined with the default wrapper type alias. Though wrapper types that are not thread safe should be put behind a no_threads feature flag, imo, as it isn't unsafe to just slap an unsafe wrapper type on something that could be accessed by threads.

---

Actually, maybe under a sub module called something like hazmat, and re-export at user_data level only with a feature flag. So users could still reach for them if they really wanted to in a non-no_threads build, but can be informed of the unsafeness during the process through the type path.

[karroffel]

This is looking pretty good now! I only suggested fixing a typo in a doc comment and that's it. Great work :)

[karroffel]

It's a lot to ask, but I feel like I would be more comfortable merging this after having some benchmarks that show how the default behavior changes with this PR applied. I could try to set up a benchmark if you don't want to put even more work on this, but I think this would be really useful to see what impact this has.

[ghost]

having some benchmarks that show how the default behavior changes with this PR applied.

I think I'll try setting one up. This should mainly impact frequent calls from Godot, so maybe just a tight loop calling methods?

[karroffel]

Yes, the entry and exit of method calls are the most interesting part in that regard, so having a loop with calls seems like a good idea.

To get a more real example maybe we can make one version with very simple function and another where the functions do some sort of work to make it more realistic.

[ghost]

Got some results using the following benchmark functions:

• echo_add: returns counter contained inside Rust type and adds by 1, 1000000 times
• naive_factor: runs a naive factor algorithm on counter and returns the result, 500000 times

Results for *const RefCell<T>:

_bench_echo_add: 1000000 times in 977 msecs, 1023541.453429 ops/sec
_bench_naive_factor: 500000 times in 2435 msecs, 205.338809 ops/sec

Results for MutexData<T>:

_bench_echo_add: 1000000 times in 1001 msecs, 999000.999001 ops/sec
_bench_naive_factor: 500000 times in 2408 msecs, 207.641196 ops/sec

... I think the takeaway here is that GDScript is so slow it doesn't really matter a lot.

We could try setting up a benchmark in C++, if you think that would make a difference.

Benchmark code

On Rust side:

use std::cell::RefCell;

#[derive(Clone, Debug)]
struct PtrUserData(*const RefCell<CallBenchmark>);

unsafe impl user_data::UserData for PtrUserData {
    type Target = CallBenchmark;

    fn new(val: Self::Target) -> Self {
        // don't care about leaks in a benchmark
        PtrUserData(Box::into_raw(Box::new(RefCell::new(val))))
    }

    unsafe fn into_user_data(self) -> *const libc::c_void {
        self.0 as *const libc::c_void
    }

    unsafe fn consume_user_data_unchecked(ptr: *const libc::c_void) -> Self {
        PtrUserData(ptr as *const RefCell<CallBenchmark>)
    }

    unsafe fn clone_from_user_data_unchecked(ptr: *const libc::c_void) -> Self {
        PtrUserData(ptr as *const RefCell<CallBenchmark>)
    }
}

impl user_data::Map for PtrUserData {
    type Err = ();
    fn map<F, U>(&self, op: F) -> Result<U, Self::Err>
    where
        F: FnOnce(&Self::Target) -> U,
    {
        unsafe {
            Ok(op(&*(&*self.0).borrow()))
        }
    }
}

impl user_data::MapMut for PtrUserData {
    type Err = ();
    fn map_mut<F, U>(&self, op: F) -> Result<U, Self::Err>
    where
        F: FnOnce(&mut Self::Target) -> U,
    {
        unsafe {
            Ok(op(&mut *(&*self.0).borrow_mut()))
        }
    }
}

#[derive(Debug)]
struct CallBenchmark(i32);

impl NativeClass for CallBenchmark {
    type Base = Reference;
    //type UserData = user_data::MutexData<CallBenchmark>;
    type UserData = PtrUserData;
    fn class_name() -> &'static str {
        "CallBenchmark"
    }
    fn init(_owner: Reference) -> CallBenchmark {
        CallBenchmark(1)
    }
    fn register_properties(_builder: &init::ClassBuilder<Self>) {
    }
}

#[methods]
impl CallBenchmark {
    #[export]
    fn set_target(&mut self, _owner: Reference, target: i32) {
        self.0 = target;
    }

    #[export]
    fn echo_add(&mut self, _owner: Reference) -> i32 {
        debug_assert!(false, "build this with optimizations!");
        let num = self.0;
        self.0 += 1;
        num
    }

    #[export]
    fn naive_factor(&self, _owner: Reference) -> i32 {
        debug_assert!(false, "build this with optimizations!");
        if self.0 < 2 {
            godot_error!("call set_target with a value larger than 1");
            return -1
        }
        let max = f64::ceil(f64::sqrt(self.0 as f64)) as i32;
        for i in 2..=max {
            if self.0 % i == 0 {
                return i;
            }
        }
        -1
    }
}

GDScript:

extends Node

func _ready():
    print(" -- Rust gdnative test suite:")
    var gdn = GDNative.new()
    var status = false;

    gdn.library = load("res://gdnative.gdnlib")

    if gdn.initialize():
        status = gdn.call_native("standard_varcall", "run_tests", [])
        #gdn.terminate()
    else:
        print(" -- Could not load the gdnative library.")

    if status:
        print(" -- Test run completed successfully.")
    else:
        print(" -- Test run completed with errors.")
        OS.exit_code = 1

    if status:
        print(" -- Rust gdnative call benchmarks:")
        _bench_echo_add(gdn.library, 1000000)
        _bench_naive_factor(gdn.library, 500000)


    print(" -- exiting.")
    get_tree().quit()

func _bench_echo_add(library, times):
    var script = NativeScript.new()
    script.set_library(library)
    script.set_class_name("CallBenchmark")
    var bench = Reference.new()
    bench.set_script(script)
    bench.set_target(757)

    var start = OS.get_system_time_msecs()
    for i in range(0, times):
        assert((757 + i) == bench.echo_add())
    var end = OS.get_system_time_msecs()

    var elapsed = end - start
    var ops = float(times) / float(elapsed) * 1000

    print("_bench_echo_add: ", times, " times in ", elapsed, " msecs, ", ops, " ops/sec")

func _bench_naive_factor(library, times):
    var script = NativeScript.new()
    script.set_library(library)
    script.set_class_name("CallBenchmark")
    var bench = Reference.new()
    bench.set_script(script)
    bench.set_target(773 * 991)

    var start = OS.get_system_time_msecs()
    for _i in range(0, times):
        assert(773 == bench.naive_factor())
    var end = OS.get_system_time_msecs()

    var elapsed = end - start
    var ops = float(times) / float(elapsed)

    print("_bench_naive_factor: ", times, " times in ", elapsed, " msecs, ", ops, " ops/sec")

[karroffel]

Seems fine, yes GDScript is not the fastest and since inside of Rust you generally don't call methods through the scripting interface it should be fine :)