godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.62k stars 210 forks source link

Implement trait-based entry point declaration #1026

Closed chitoyuu closed 1 year ago

chitoyuu commented 1 year ago

Adds the #[gdnative::init::callbacks] attribute macro and GDNativeCallbacks trait for entry point declaration, replacing the old system of init macros. godot_init! is kept as a shim and deprecated.

This also adds support for the following callbacks:

As a side effect, most of the init boilerplate are no longer emitted into the user crate, instead staying in gdnative-core. Some further internal refactoring might be necessary to improve code organization.

Close #985, Close #338

chitoyuu commented 1 year ago

Example code looks like this:

struct HelloWorldLibrary;

#[gdnative::init::callbacks]
unsafe impl GDNativeCallbacks for HelloWorldLibrary {
    fn nativescript_init(handle: InitHandle) {
        handle.add_class::<HelloWorld>();
    }
}

The safety section is still TODO. GDNativeCallbacks currently contains definitions for all known GDNative symbols, including many niche ones that aren't previously exposed (for many years!) and are of dubious usefulness. I wonder if it's a little bit too low-level for the average use case of simply registering a bunch of types, but still it should be a step up from the previous system.


I don't know why the TODO check is somehow passing on CI. It's failing on my local machine. Might need to look into this later.

chitoyuu commented 1 year ago

You marked this as breaking-change, but is it when the old macros are kept around? I would probably even keep them at least during the next minor version, to give people time to migrate. Keep in mind that removing the init macros breaks every single godot-rust application.

Yes, indeed I'm aware of that. It has to be a breaking change because the separate macros can't be implemented in terms of the new system, only godot_init! can (which is). I think it should be possible to keep the empty variants of the macros as no-ops, but anything outside than the godot_init! pattern will have to be broken. Perhaps we can do that.

bluenote10 commented 1 year ago

From a user perspective the most puzzling part about this change is that now an unsafe is needed, while the old pattern didn't. The notion of an unsafe trait is a bit niche, and probably not familiar to everyone. For instance, I was at first wondering if this means that all the init functions now run in an unsafe scope. After experimenting a bit, I don't think this is the case fortunately, right? But then I still don't fully understand what it means for me as a user having to provide an unsafe impl. Reading up on the topic I came across statements like

When a trait is marked as unsafe, it means the trait implementor needs to implement carefully.

but I wouldn't know what exactly I have to worry about implementing my init functions.

To avoid frequent questions, it would be great if the trait docstring could briefly explain what the unsafe is trying to communicate / accomplish.

chitoyuu commented 1 year ago

The question is, is this unsafe really needed at the moment? The GDNative Rust binding has a much more sophisticated type system than the GDExtension one and carefully makes sure that the user is made aware when they must uphold invariants (e.g. in assume_safe()). There may be edge cases, but does it warrant having both unsafes?

You're right that it isn't immediately necessary. Perhaps it makes sense to remove it for now.

chitoyuu commented 1 year ago

Applied suggestions in the thread.

bors try

bors[bot] commented 1 year ago

try

Build succeeded:

chitoyuu commented 1 year ago

bors r+

bors[bot] commented 1 year ago

Build succeeded: