Proof of concept: Register wrapped functions

It's been a while. Last time, tired of the language barrier, I disappeared. But I'll try again.

This is a proof of concept of the wrapper code used to register classes and methods with Godot.

Design

Update: Design intent was added.

Can register directly any type

Any type, such as Rc<T> or rand::rngs::ThreadRng (maybe even Box<dyn T>) can be register as class. Types are not required to implement the NativeClass trait.

Allowing Rc<T> to register directly will remove the boilerplate when shared values between Rust and Godot. Another goal is to remove restrictions on extension from external libraries.

Boilerplate example: https://gist.github.com/B-head/21c659978ec55b6ae1b128db59f0d8e7

Flexible `self` and `#[base]` parameter type

If the registered class implements AsRef<T> trait, type T can be used for the self parameter of methods. &Rc<Self> or &Arc<Self> can also be used.

Types that implement FromInstancePtr<'a> trait and FromBasePtr<'a> trait can be used for #[base] parameter type. Smart pointers from external libraries can also be used if traits are implemented.

Mutable references cannot be used for `self` parameter

Due to technical constraints because of use of AsRef<T> trait, mutable references cannot be used for self parameter. interior mutability must be introduced on the user side using Cell<T> or RefCell<T>.

Nevertheless, the code is better if interior mutability is introduced on the user side. Because:

Cell<T>, AtomicI32, or other types can be used freely.
By drop the reference guard before calling Godot method or before emit a signal, so that Rust method can be called again.

Cannot be called from other than main thread

Instance creation and method calls are not allowed from threads created in Godot. Instead, create threads in Rust.

This is because they would not benefit from Rust static analysis. Node in Godot is not Send by Rust criteria because it is referenced by another Node without guard. However, Godot does not care about it but sends to other threads, so non Send values shared among threads are taken into Rust. And there is no way for Rust to detect that values are being shared among threads.

The same applies to calls to Rust methods after sharing values only between Godot threads, so do not create threads in Godot.

Incidentally, call_deferred("emit_signal", ...) can be used to safely send values from the thread created by Rust. (although marked as unsafe in the current API)

zero overhead (Maybe)

Since Rust generates a type that uniquely identifies the function, so using generic parameters should result in zero overhead.

Reference: https://doc.rust-lang.org/std/primitive.fn.html#creating-function-pointers

Additional suggestion

This could be a separate crate to support both gdnative and gdextension.

@chitoyuu

Would it be possible to support this using existing high-level manual registration APIs, like this?

That might be possible, but I don't think the code would be simple, given that NativeClass is renaming the class and specifying UserData wrapper.

That might be possible, but I don't think the code would be simple, given that NativeClass is renaming the class and specifying UserData wrapper.

Are you by any chance working off a very old version? Since #847 (just a little over a year ago), the class name no longer has to be defined statically in NativeClass, but can be provided dynamically through the aforementioned InitHandle::add_class_as_with method.

The UserData wrapper type should be possible to parameterize with some refactoring, now that associated type constructors have been stablized.

The big issue with all this brand new unsafe code here is that, again, while it looks simple on the surface for now, there are too many unknowns:

Does it cover all the features of the old code? How much demand is there for the new features that it adds?
Does it handle errors as well, is it as safe?
Does it play as well with the macro system? How big a change would have to be made?
If it doesn't, is it easy to extend it later so that it does? Would it still be simple after that?

Please understand that throwing away years of design and opting into this whole new untested system, is not a decision that can be lightly made. To be clear, we need very positive answers for all the questions above for this to be considered.

I urge you to fully explore the possibilities within our current framework first, before diving deeper into further implementation. Our project depends fully on volunteer contributors. It would be very sad to see a major effort end up unmerged due to direction problems.

Are you by any chance working off a very old version? Since https://github.com/godot-rust/gdnative/pull/847 (just a little over a year ago), the class name no longer has to be defined statically in NativeClass, but can be provided dynamically through the aforementioned InitHandle::add_class_as_with method.

The UserData wrapper type should be possible to parameterize with some refactoring, now that associated type constructors have been stablized.

However, if we write a wrapper and expose all of its contents outside of the module, then the wrapper introduces needless complexity. It is simpler for the user to create the new type directly.

To begin with, the possibility of registering foreign types is an added bonus. The design goals are to make ‘Rc＜T>‘ type easy to use and to improve extensibility. Do you think you can achieve that goal with a wrapper?

(Had InitHandle::add_class_as_with been placed in the export module I would have noticed it)

Does it cover all the features of the old code?

The emplace construct feature has not been written yet, but since this PoC is generalization of an old code, there are no features to be removed. (Automatic interior mutability can be implemented externally)

How much demand is there for the new features that it adds?

This is a proof of concept to measure that.

Does it handle errors as well, is it as safe?

this (godot instance) pointer is null: Trust Godot and will not check. (Changed from old code)
this (godot instance) pointer is invalid type: Trust Godot and will not check. (Same as old code)
user_data pointer is null: Check and not accept. (Same as old code)
user_data pointer is invalid type: Statically constrained. (Same as old code)
method_data pointer is null: If the creation of method_data failed, then the method will not be registered. (Same as old code)
method_data pointer is invalid type: Statically constrained. (Same as old code)
args pointer is null: Trust Godot and will not check. (Same as old code)
args pointer and num_args is invalid data: Trust Godot and will not check. (Same as old code)

Does it play as well with the macro system? How big a change would have to be made? If it doesn't, is it easy to extend it later so that it does? Would it still be simple after that?

Since this PoC is only generalization of the interface, all that needs to be changes in the macro is the part that generates the code that uses the interface.

I urge you to fully explore the possibilities within our current framework first,

Because I considered it, I know that implementing the feature would involve major breaking change.

I'll be completely blunt. Up till now, everything you have said about the motivation of this rewrite is purely abstract. No tests, no code examples, no before-afters -- nothing. This is not enough to justify such a big change, by a long shot. You have to be more concrete here.

Allowing Rc<T> to register directly will remove the boilerplate when shared values between Rust and Godot. Another goal is to remove restrictions on extension from external libraries.

Immediately after this section you suggested yourself that further interior mutability should be used within user types, due to a limitation that your new system brings by. How is that not boilerplate? How does that not introduce complexity?

To begin with, the possibility of registering foreign types is an added bonus. The design goals are to make ‘Rc＜T>‘ type easy to use and to improve extensibility. Do you think you can achieve that goal with a wrapper?

What extensibility are you talking about here? What are you trying to extend? How do you want to extend it? Why is it not possible without rewriting everything? Show that not with words, but with code examples and tests that demonstrate how your approach is (supposedly) superior.

How much demand is there for the new features that it adds?

This is a proof of concept to measure that.

If you don't even know what the use case is, and need to measure it, then it's too early to make a PR. You should have done that with a GitHub issue or a Discord discussion.

To be fair, there are elements in this PR that resemble things we're already trying to do (e.g. #974), which might have been valuable if you took the time to research, and communicate in advance. It hurts me to say this, but sadly I don't see any way to move this PR forward as it stands. While we're grateful for your effort, I'm labeling this as wontfix, and will proceed to close in a few days, unless major changes in direction are made.

Please consider discussing your ideas with other developers first before making big changes like this. This applies not only to godot-rust, but generally to other open source projects as well. It might feel slower at first, but overall it leads to less wasted efforts and better use of time for everyone.

No tests, no code examples, no before-afters -- nothing.

This seems I have been terribly lazy. I must work harder.

Immediately after this section you suggested yourself that further interior mutability should be used within user types, due to a limitation that your new system brings by. How is that not boilerplate? How does that not introduce complexity?

This is true and I wanted feedback on whether I need to address this issue. However, it seems I was wrong to suggest more than one thing at a time.

What extensibility are you talking about here? What are you trying to extend? How do you want to extend it? Why is it not possible without rewriting everything? Show that not with words, but with code examples and tests that demonstrate how your approach is (supposedly) superior.

Since NativeClass cannot be implemented separately for Rc<LocalType> due to orphan rule limitation, NativeClass must be removed. But that would be major breaking change, so I want to rewrite the code as independent of the export module. (I should have explained this first)

I don't have an idea for an extension with "register directly any type" feature yet, but the orphan rule tends to be a limitation on extensions. Removing it and that should be useful in the future.

One idea for extending smart pointers is to separate pointers for "direct call" and pointers for "deferred call" to improve thread safety. However, this would require changing most of the code generated by the bindgen crate, so merging is impossible. So I want an extension point that can be developed as an external library.

I want to be able to write code like this.

#[method]
pub fn foo(self: &Rc<Self>, #[base] base: Gdcd<Self>) {}

it's too early to make a PR. You should have done that with a GitHub issue or a Discord discussion.

Since this PR description did not convey the intent to you, I think the intent was more difficult to convey on Discord... 😢 And this PoC is intended to start the discussion.

(e.g. https://github.com/godot-rust/gdnative/issues/974)

I don't think that would conflict with this PoC, since that can be done just by changing the macro.

This seems I have been terribly lazy. I must work harder.

Nobody is saying that you're lazy. You obviously aren't. You're just putting your effort in the wrong order. This could have been avoided through communication.

Since NativeClass cannot be implemented separately for Rc<LocalType> due to orphan rule limitation, NativeClass must be removed. But that would be major breaking change, so I want to rewrite the code as independent of the export module. (I should have explained this first)

Yes. You should have explained this first. Also, this is simply incorrect. It's fully possible to register generic NativeClasses, and allow arbitrary types to be registered through that. I literally gave an example right above in this comment: https://github.com/godot-rust/gdnative/pull/1033#pullrequestreview-1294425359

You are locked into this mode of thinking because you didn't communicate first, and continue to refuse to do it even after.

I want to be able to write code like this.
#[method]
pub fn foo(self: &Rc<Self>, #[base] base: Gdcd<Self>) {}

Now this, this is the thing you should have posted, as an issue or a question, before making any decisions, before writing any code, before spending any time chasing this whole XY problem.

The answer would be simple. This would've been covered by #974 with some macro changes. None of this complexity is necessary if all you want is to be able to write this.

I'll start working on #974 soon, since there's apparently some demand for that feature. We can keep this PR open until it's implemented, in case there's more discussion to do.

Since the main issue motivating this PR has now been resolved by #1034, are there any further reasons to pursue this? Otherwise, we can close this as superseded.

Superseded by #1034.

godot-rust / gdnative