From c61a1baa43bd56d5b004f2fd635acce2e4e6b2e7 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 26 Nov 2023 22:36:16 +0100 Subject: [PATCH 1/2] Empty #[godot_api] workaround is no longer needed --- godot-core/src/obj/traits.rs | 2 +- godot-core/src/registry.rs | 66 +++++++++++++------ godot-macros/src/class/derive_godot_class.rs | 3 + godot-macros/src/class/godot_api.rs | 14 ++-- .../rust/src/register_tests/constant_test.rs | 4 +- 5 files changed, 59 insertions(+), 30 deletions(-) diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index dcdbc51d3..45dc53a4d 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -38,7 +38,7 @@ where /// During which initialization level this class is available/should be initialized with Godot. /// /// Is `None` if the class has complicated initialization requirements, and generally cannot be inherited - /// from. + /// from (currently only for `()`, the "base" of `Object`). const INIT_LEVEL: Option; /// The name of the class, under which it is registered in Godot. diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs index aed07c039..35b35b970 100644 --- a/godot-core/src/registry.rs +++ b/godot-core/src/registry.rs @@ -39,6 +39,9 @@ static LOADED_CLASSES: Mutex>>> = Mutex pub struct ClassPlugin { pub class_name: ClassName, pub component: PluginComponent, + + // Init-level is per ClassPlugin and not per PluginComponent, because all components of all classes are mixed together in one + // huge linker list. There is no per-class aggregation going on, so this allows to easily filter relevant classes. pub init_level: Option, } @@ -60,11 +63,11 @@ impl fmt::Debug for ErasedRegisterFn { /// Represents the data part of a [`ClassPlugin`] instance. #[derive(Clone, Debug)] pub enum PluginComponent { - /// Class definition itself, must always be available + /// Class definition itself, must always be available. ClassDef { base_class_name: ClassName, - /// Godot low-level`create` function, wired up to library-generated `init` + /// Godot low-level `create` function, wired up to library-generated `init`. generated_create_fn: Option< unsafe extern "C" fn( _class_userdata: *mut std::ffi::c_void, // @@ -78,6 +81,9 @@ pub enum PluginComponent { ) -> sys::GDExtensionClassInstancePtr, >, + /// Callback to library-generated function which registers properties in the `struct` definition. + register_properties_fn: ErasedRegisterFn, + free_fn: unsafe extern "C" fn( _class_user_data: *mut std::ffi::c_void, instance: sys::GDExtensionClassInstancePtr, @@ -86,18 +92,18 @@ pub enum PluginComponent { /// Collected from `#[godot_api] impl MyClass` UserMethodBinds { - /// Callback to library-generated function which registers functions in the `impl` + /// Callback to library-generated function which registers functions and constants in the `impl` block. /// /// Always present since that's the entire point of this `impl` block. - generated_register_fn: ErasedRegisterFn, + register_methods_constants_fn: ErasedRegisterFn, }, /// Collected from `#[godot_api] impl GodotExt for MyClass` UserVirtuals { - /// Callback to user-defined `register_class` function + /// Callback to user-defined `register_class` function. user_register_fn: Option, - /// Godot low-level`create` function, wired up to the user's `init` + /// Godot low-level `create` function, wired up to the user's `init`. user_create_fn: Option< unsafe extern "C" fn( _class_userdata: *mut std::ffi::c_void, // @@ -111,7 +117,7 @@ pub enum PluginComponent { ) -> sys::GDExtensionClassInstancePtr, >, - /// User-defined `to_string` function + /// User-defined `to_string` function. user_to_string_fn: Option< unsafe extern "C" fn( p_instance: sys::GDExtensionClassInstancePtr, @@ -120,7 +126,7 @@ pub enum PluginComponent { ), >, - /// User-defined `on_notification` function + /// User-defined `on_notification` function. #[cfg(before_api = "4.2")] user_on_notification_fn: Option< unsafe extern "C" fn( @@ -137,7 +143,7 @@ pub enum PluginComponent { ), >, - /// Callback for other virtuals + /// Callback for other virtuals. get_virtual_fn: unsafe extern "C" fn( p_userdata: *mut std::os::raw::c_void, p_name: sys::GDExtensionConstStringNamePtr, @@ -154,8 +160,12 @@ pub enum PluginComponent { struct ClassRegistrationInfo { class_name: ClassName, parent_class_name: Option, - generated_register_fn: Option, + // Following functions are stored separately, since their order matters. + register_methods_constants_fn: Option, + register_properties_fn: Option, user_register_fn: Option, + + /// Godot low-level class creation parameters. #[cfg(before_api = "4.2")] godot_params: sys::GDExtensionClassCreationInfo, #[cfg(since_api = "4.2")] @@ -208,7 +218,8 @@ pub fn register_class< register_class_raw(ClassRegistrationInfo { class_name: T::class_name(), parent_class_name: Some(T::Base::class_name()), - generated_register_fn: None, + register_methods_constants_fn: None, + register_properties_fn: None, user_register_fn: Some(ErasedRegisterFn { raw: callbacks::register_class_by_builder::, }), @@ -232,13 +243,15 @@ pub fn auto_register_classes(init_level: InitLevel) { crate::private::iterate_plugins(|elem: &ClassPlugin| { //out!("* Plugin: {elem:#?}"); + + // Filter per ClassPlugin and not PluginComponent, because all components of all classes are mixed together in one huge list. match elem.init_level { None => { log::godot_error!("Unknown initialization level for class {}", elem.class_name); return; } Some(elem_init_level) if elem_init_level != init_level => return, - _ => (), + _ => { /* Nothing */ } } let name = elem.class_name; @@ -304,6 +317,7 @@ fn fill_class_info(component: PluginComponent, c: &mut ClassRegistrationInfo) { base_class_name, generated_create_fn, generated_recreate_fn, + register_properties_fn, free_fn, } => { c.parent_class_name = Some(base_class_name); @@ -335,12 +349,13 @@ fn fill_class_info(component: PluginComponent, c: &mut ClassRegistrationInfo) { assert!(generated_recreate_fn.is_none()); // not used c.godot_params.free_instance_func = Some(free_fn); + c.register_properties_fn = Some(register_properties_fn); } PluginComponent::UserMethodBinds { - generated_register_fn, + register_methods_constants_fn, } => { - c.generated_register_fn = Some(generated_register_fn); + c.register_methods_constants_fn = Some(register_methods_constants_fn); } PluginComponent::UserVirtuals { @@ -433,11 +448,18 @@ fn register_class_raw(info: ClassRegistrationInfo) { //let mut class_builder = crate::builder::ClassBuilder::::new(); let mut class_builder = 0; // TODO dummy argument; see callbacks - // First call generated (proc-macro) registration function, then user-defined one. - // This mimics the intuition that proc-macros are running "before" normal runtime code. - if let Some(register_fn) = info.generated_register_fn { + // Order of the following registrations is crucial: + // 1. Methods and constants. + // 2. Properties (they may depend on get/set methods). + // 3. User-defined registration function (intuitively, user expects their own code to run after proc-macro generated code). + if let Some(register_fn) = info.register_methods_constants_fn { + (register_fn.raw)(&mut class_builder); + } + + if let Some(register_fn) = info.register_properties_fn { (register_fn.raw)(&mut class_builder); } + if let Some(register_fn) = info.user_register_fn { (register_fn.raw)(&mut class_builder); } @@ -639,7 +661,11 @@ pub mod callbacks { T::__godot_register_class(&mut class_builder); } - pub fn register_user_binds( + pub fn register_user_properties(_class_builder: &mut dyn Any) { + T::__register_exports(); + } + + pub fn register_user_methods_constants( _class_builder: &mut dyn Any, ) { // let class_builder = class_builder @@ -649,7 +675,6 @@ pub mod callbacks { //T::register_methods(class_builder); T::__register_methods(); T::__register_constants(); - T::__register_exports(); } } @@ -662,7 +687,8 @@ fn default_registration_info(class_name: ClassName) -> ClassRegistrationInfo { ClassRegistrationInfo { class_name, parent_class_name: None, - generated_register_fn: None, + register_methods_constants_fn: None, + register_properties_fn: None, user_register_fn: None, godot_params: default_creation_info(), init_level: InitLevel::Scene, diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs index e0fe49a78..7852f0a19 100644 --- a/godot-macros/src/class/derive_godot_class.rs +++ b/godot-macros/src/class/derive_godot_class.rs @@ -103,6 +103,9 @@ pub fn derive_godot_class(decl: Declaration) -> ParseResult { base_class_name: #base_class_name_obj, generated_create_fn: #create_fn, generated_recreate_fn: #recreate_fn, + register_properties_fn: #prv::ErasedRegisterFn { + raw: #prv::callbacks::register_user_properties::<#class_name>, + }, free_fn: #prv::callbacks::free::<#class_name>, }, init_level: <#class_name as ::godot::obj::GodotClass>::INIT_LEVEL, diff --git a/godot-macros/src/class/godot_api.rs b/godot-macros/src/class/godot_api.rs index 6df74fea4..f06f319e1 100644 --- a/godot-macros/src/class/godot_api.rs +++ b/godot-macros/src/class/godot_api.rs @@ -77,10 +77,10 @@ struct SignalDefinition { } /// Codegen for `#[godot_api] impl MyType` -fn transform_inherent_impl(mut decl: Impl) -> Result { - let class_name = util::validate_impl(&decl, None, "godot_api")?; +fn transform_inherent_impl(mut original_impl: Impl) -> Result { + let class_name = util::validate_impl(&original_impl, None, "godot_api")?; let class_name_obj = util::class_name_obj(&class_name); - let (funcs, signals) = process_godot_fns(&mut decl)?; + let (funcs, signals) = process_godot_fns(&mut original_impl)?; let mut signal_cfg_attrs: Vec> = Vec::new(); let mut signal_name_strs: Vec = Vec::new(); @@ -135,7 +135,7 @@ fn transform_inherent_impl(mut decl: Impl) -> Result { .into_iter() .map(|func_def| make_method_registration(&class_name, func_def)); - let consts = process_godot_constants(&mut decl)?; + let consts = process_godot_constants(&mut original_impl)?; let mut integer_constant_cfg_attrs = Vec::new(); let mut integer_constant_names = Vec::new(); let mut integer_constant_values = Vec::new(); @@ -184,7 +184,7 @@ fn transform_inherent_impl(mut decl: Impl) -> Result { }; let result = quote! { - #decl + #original_impl impl ::godot::obj::cap::ImplementsGodotApi for #class_name { fn __register_methods() { @@ -227,8 +227,8 @@ fn transform_inherent_impl(mut decl: Impl) -> Result { ::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin { class_name: #class_name_obj, component: #prv::PluginComponent::UserMethodBinds { - generated_register_fn: #prv::ErasedRegisterFn { - raw: #prv::callbacks::register_user_binds::<#class_name>, + register_methods_constants_fn: #prv::ErasedRegisterFn { + raw: #prv::callbacks::register_user_methods_constants::<#class_name>, }, }, init_level: <#class_name as ::godot::obj::GodotClass>::INIT_LEVEL, diff --git a/itest/rust/src/register_tests/constant_test.rs b/itest/rust/src/register_tests/constant_test.rs index 5f40e8524..28b784625 100644 --- a/itest/rust/src/register_tests/constant_test.rs +++ b/itest/rust/src/register_tests/constant_test.rs @@ -168,8 +168,8 @@ godot::sys::plugin_add!( ::godot::private::ClassPlugin { class_name: HasOtherConstants::class_name(), component: ::godot::private::PluginComponent::UserMethodBinds { - generated_register_fn: ::godot::private::ErasedRegisterFn { - raw: ::godot::private::callbacks::register_user_binds::, + register_methods_constants_fn: ::godot::private::ErasedRegisterFn { + raw: ::godot::private::callbacks::register_user_methods_constants::, }, }, init_level: HasOtherConstants::INIT_LEVEL, From 560cdeef93d211d21d83bd695bd839386eb1e95a Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 26 Nov 2023 22:57:33 +0100 Subject: [PATCH 2/2] Remove all places where empty #[godot_api] workaround was in use --- godot-core/src/lib.rs | 9 +++---- godot-core/src/property.rs | 17 ------------ .../src/class/data_models/property.rs | 10 ------- godot-macros/src/class/godot_api.rs | 2 -- godot-macros/src/lib.rs | 22 --------------- itest/rust/build.rs | 3 --- itest/rust/src/object_tests/object_test.rs | 6 ----- itest/rust/src/object_tests/property_test.rs | 27 ------------------- .../src/object_tests/virtual_methods_test.rs | 3 --- .../src/register_tests/option_ffi_test.rs | 3 --- itest/rust/src/register_tests/var_test.rs | 4 --- 11 files changed, 4 insertions(+), 102 deletions(-) diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index 4a4735c4f..aa4802725 100644 --- a/godot-core/src/lib.rs +++ b/godot-core/src/lib.rs @@ -45,11 +45,6 @@ mod gen; #[doc(hidden)] pub mod private { - // If someone forgets #[godot_api], this causes a compile error, rather than virtual functions not being called at runtime. - #[allow(non_camel_case_types)] - pub trait You_forgot_the_attribute__godot_api {} - pub use crate::property::Cannot_export_without_godot_api_impl; - use std::sync::{Arc, Mutex}; pub use crate::gen::classes::class_macros; @@ -59,6 +54,10 @@ pub mod private { use crate::{log, sys}; + // If someone forgets #[godot_api], this causes a compile error, rather than virtual functions not being called at runtime. + #[allow(non_camel_case_types)] + pub trait You_forgot_the_attribute__godot_api {} + sys::plugin_registry!(pub __GODOT_PLUGIN_REGISTRY: ClassPlugin); pub(crate) fn iterate_plugins(mut visitor: impl FnMut(&ClassPlugin)) { diff --git a/godot-core/src/property.rs b/godot-core/src/property.rs index 65a6bae1c..83aece929 100644 --- a/godot-core/src/property.rs +++ b/godot-core/src/property.rs @@ -106,23 +106,6 @@ impl PropertyHintInfo { } } -/// To export properties to Godot, you must have an impl-block with the `#[godot_api]` attribute, even if -/// it is empty. -/// -/// This trait is automatically implemented when such an impl-block is present. If Rust complains that it is -/// not implemented, then you can usually fix this by adding: -/// -/// ```ignore -/// #[godot_api] -/// impl MyClass {} -/// ``` -/// -/// Where you replace `MyClass` with the name of your class. -#[allow(non_camel_case_types)] -pub trait Cannot_export_without_godot_api_impl { - const EXISTS: () = (); -} - /// Functions used to translate user-provided arguments into export hints. pub mod export_info_functions { use crate::builtin::GString; diff --git a/godot-macros/src/class/data_models/property.rs b/godot-macros/src/class/data_models/property.rs index 6fdccd3db..5ea135da1 100644 --- a/godot-macros/src/class/data_models/property.rs +++ b/godot-macros/src/class/data_models/property.rs @@ -203,18 +203,8 @@ pub fn make_property_impl(class_name: &Ident, fields: &Fields) -> TokenStream { }); } - let enforce_godot_api_impl = if !export_tokens.is_empty() { - quote! { - const MUST_HAVE_GODOT_API_IMPL: () = <#class_name as ::godot::private::Cannot_export_without_godot_api_impl>::EXISTS; - } - } else { - TokenStream::new() - }; - quote! { impl #class_name { - #enforce_godot_api_impl - #(#getter_setter_impls)* } diff --git a/godot-macros/src/class/godot_api.rs b/godot-macros/src/class/godot_api.rs index f06f319e1..a658a2774 100644 --- a/godot-macros/src/class/godot_api.rs +++ b/godot-macros/src/class/godot_api.rs @@ -222,8 +222,6 @@ fn transform_inherent_impl(mut original_impl: Impl) -> Result TokenStream { /// foo: TestEnum /// } /// -/// # //TODO: remove this when https://github.com/godot-rust/gdext/issues/187 is truly addressed -/// # #[godot_api] -/// # impl TestClass {} -/// /// # fn main() { /// let mut class = TestClass {foo: TestEnum::B}; /// assert_eq!(class.get_foo(), TestEnum::B as i32); diff --git a/itest/rust/build.rs b/itest/rust/build.rs index cb2dc05f5..11bcb1d1b 100644 --- a/itest/rust/build.rs +++ b/itest/rust/build.rs @@ -457,9 +457,6 @@ fn generate_property_template(inputs: &[Input]) -> PropertyTests { #[export(enum = (Rebecca, Mary, Leah))] export_enum_string_rebecca_mary_leah: GString, } - - #[godot_api] - impl PropertyTestsRust {} }; let gdscript = format!( diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index 3de509fe7..60f86e4ca 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -844,9 +844,6 @@ pub mod object_test_gd { i: i64, } - #[godot_api] - impl MockObjRust {} - #[derive(GodotClass)] #[class(init, base=RefCounted)] struct MockRefCountedRust { @@ -854,9 +851,6 @@ pub mod object_test_gd { i: i64, } - #[godot_api] - impl MockRefCountedRust {} - #[derive(GodotClass, Debug)] #[class(init, base=RefCounted)] struct ObjectTest; diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index 483e65c35..8fa0269b8 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -298,9 +298,6 @@ struct CheckAllExports { color_no_alpha: Color, } -#[godot_api] -impl CheckAllExports {} - #[repr(i64)] #[derive(Property, Export, Eq, PartialEq, Debug)] pub enum TestEnum { @@ -315,9 +312,6 @@ pub struct DeriveProperty { pub foo: TestEnum, } -#[godot_api] -impl DeriveProperty {} - #[itest] fn derive_property() { let mut class = DeriveProperty { foo: TestEnum::B }; @@ -335,9 +329,6 @@ pub struct DeriveExport { pub base: Base, } -#[godot_api] -impl DeriveExport {} - #[godot_api] impl IRefCounted for DeriveExport { fn init(base: godot::obj::Base) -> Self { @@ -373,22 +364,10 @@ fn derive_export() { #[class(init, base=Resource)] pub struct CustomResource {} -#[godot_api] -impl CustomResource {} - -#[godot_api] -impl IResource for CustomResource {} - #[derive(GodotClass)] #[class(init, base=Resource, rename=NewNameCustomResource)] pub struct RenamedCustomResource {} -#[godot_api] -impl RenamedCustomResource {} - -#[godot_api] -impl IResource for RenamedCustomResource {} - #[derive(GodotClass)] #[class(init, base=Node)] pub struct ExportResource { @@ -400,12 +379,6 @@ pub struct ExportResource { pub bar: Option>, } -#[godot_api] -impl ExportResource {} - -#[godot_api] -impl INode for ExportResource {} - #[itest] fn export_resource() { let class = ExportResource::alloc_gd(); diff --git a/itest/rust/src/object_tests/virtual_methods_test.rs b/itest/rust/src/object_tests/virtual_methods_test.rs index 21005f8c5..07e5f8b32 100644 --- a/itest/rust/src/object_tests/virtual_methods_test.rs +++ b/itest/rust/src/object_tests/virtual_methods_test.rs @@ -44,9 +44,6 @@ struct VirtualMethodTest { integer: i32, } -#[godot_api] -impl VirtualMethodTest {} - #[godot_api] impl IRefCounted for VirtualMethodTest { fn to_string(&self) -> GString { diff --git a/itest/rust/src/register_tests/option_ffi_test.rs b/itest/rust/src/register_tests/option_ffi_test.rs index a7d4351ec..e12780826 100644 --- a/itest/rust/src/register_tests/option_ffi_test.rs +++ b/itest/rust/src/register_tests/option_ffi_test.rs @@ -103,6 +103,3 @@ struct OptionExportFfiTest { #[export] optional_export: Option>, } - -#[godot_api] -impl OptionExportFfiTest {} diff --git a/itest/rust/src/register_tests/var_test.rs b/itest/rust/src/register_tests/var_test.rs index ef4406d93..cf5437f3a 100644 --- a/itest/rust/src/register_tests/var_test.rs +++ b/itest/rust/src/register_tests/var_test.rs @@ -20,7 +20,3 @@ struct WithInitDefaults { #[init(default = -42)] expr_int: i64, } - -// TODO Remove once https://github.com/godot-rust/gdext/issues/187 is fixed -#[godot_api] -impl WithInitDefaults {}