From 661c80f734599190b6bc03ed64bc4fcd5a8b675f Mon Sep 17 00:00:00 2001 From: Yarvin Date: Sun, 16 Feb 2025 16:29:46 +0100 Subject: [PATCH 1/4] Add `OnEditor`, remove `Export` and `Var` implementations for `DynGd` and `Gd` - Add `OnEditor` - a wrapper that allows to export non-nullable types which must be nullable in the Editor. - Remove Export and Var implementations for `DynGd` and `Gd` - it should be provided by algebraic types instead (OnEditor and Option). - Add marker trait `BuiltinExport` which informs if given type can be safely and conveniently used in a `#[export]` directly. - Create implementations for `OnEditor>` and `OnEditor>`. - Create proper blanket implementation for Godot Types other than Gd - Inform user about available use cases for `OnEditor` in associated docs - Move Gd::export_info to PropertyHintInfo::export_gd. - Add before_ready check for OnEditor fields to panic and warn user in case if values haven't been set. - FIX: `Option>` and `OnEditor>` can no longer be used as an Export for Resource-based classes for `T` Inheriting Node. --- godot-core/src/builtin/collections/array.rs | 43 ++- godot-core/src/meta/property_info.rs | 43 ++- godot-core/src/meta/sealed.rs | 3 +- godot-core/src/obj/dyn_gd.rs | 116 ++++++- godot-core/src/obj/gd.rs | 87 +++-- godot-core/src/obj/mod.rs | 2 + godot-core/src/obj/oneditor.rs | 308 ++++++++++++++++++ godot-core/src/registry/class.rs | 3 +- godot-core/src/registry/property.rs | 23 ++ godot-macros/src/class/data_models/field.rs | 2 + godot-macros/src/class/derive_godot_class.rs | 137 ++++++-- godot/src/prelude.rs | 2 +- itest/godot/ManualFfiTests.gd | 5 +- itest/rust/src/object_tests/dyn_gd_test.rs | 23 +- itest/rust/src/object_tests/mod.rs | 1 + itest/rust/src/object_tests/oneditor_test.rs | 85 +++++ itest/rust/src/object_tests/property_test.rs | 6 +- .../src/register_tests/option_ffi_test.rs | 2 +- 18 files changed, 813 insertions(+), 78 deletions(-) create mode 100644 godot-core/src/obj/oneditor.rs create mode 100644 itest/rust/src/object_tests/oneditor_test.rs diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index 82399f145..c9ccbd8ee 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -12,11 +12,12 @@ use crate::builtin::*; use crate::meta; use crate::meta::error::{ConvertError, FromGodotError, FromVariantError}; use crate::meta::{ - element_godot_type_name, element_variant_type, ArrayElement, ArrayTypeInfo, AsArg, CowArg, - FromGodot, GodotConvert, GodotFfiVariant, GodotType, ParamType, PropertyHintInfo, RefArg, - ToGodot, + element_godot_type_name, element_variant_type, ArrayElement, ArrayTypeInfo, AsArg, ClassName, + CowArg, FromGodot, GodotConvert, GodotFfiVariant, GodotType, ParamType, PropertyHintInfo, + RefArg, ToGodot, }; -use crate::registry::property::{Export, Var}; +use crate::obj::{bounds, Bounds, DynGd, Gd, GodotClass}; +use crate::registry::property::{BuiltinExport, Export, Var}; use godot_ffi as sys; use sys::{ffi_methods, interface_fn, GodotFfi}; @@ -1099,6 +1100,40 @@ where } } +impl BuiltinExport for Array {} + +impl Export for Array> +where + T: Bounds, +{ + fn export_hint() -> PropertyHintInfo { + PropertyHintInfo::export_array_element::>() + } + + #[doc(hidden)] + fn as_node_class() -> Option { + PropertyHintInfo::object_as_node_class::() + } +} + +/// `#[export]` for `Array>` is available only for `T` being Engine class (such as Node or Resource). +/// +/// Consider exporting `Array>` instead of `Array>` for user-declared GDExtension classes. +impl Export for Array> +where + T: GodotClass + Bounds, + D: ?Sized + 'static, +{ + fn export_hint() -> PropertyHintInfo { + PropertyHintInfo::export_array_element::>() + } + + #[doc(hidden)] + fn as_node_class() -> Option { + PropertyHintInfo::object_as_node_class::() + } +} + impl Default for Array { #[inline] fn default() -> Self { diff --git a/godot-core/src/meta/property_info.rs b/godot-core/src/meta/property_info.rs index 51a96f058..8c4f0c8b9 100644 --- a/godot-core/src/meta/property_info.rs +++ b/godot-core/src/meta/property_info.rs @@ -10,9 +10,10 @@ use crate::global::{PropertyHint, PropertyUsageFlags}; use crate::meta::{ element_godot_type_name, ArrayElement, ClassName, GodotType, PackedArrayElement, }; -use crate::obj::{EngineBitfield, EngineEnum}; +use crate::obj::{bounds, Bounds, EngineBitfield, EngineEnum, GodotClass}; +use crate::registry::class::get_dyn_property_hint_string; use crate::registry::property::{Export, Var}; -use crate::sys; +use crate::{classes, sys}; use godot_ffi::VariantType; /// Describes a property in Godot. @@ -302,4 +303,42 @@ impl PropertyHintInfo { hint_string: GString::from(T::element_type_string()), } } + + pub fn export_gd() -> Self + where + T: GodotClass + Bounds, + { + let hint = if T::inherits::() { + PropertyHint::RESOURCE_TYPE + } else if T::inherits::() { + PropertyHint::NODE_TYPE + } else { + unreachable!("classes not inheriting from Resource or Node should not be exportable") + }; + + // Godot does this by default too; the hint is needed when the class is a resource/node, + // but doesn't seem to make a difference otherwise. + let hint_string = T::class_name().to_gstring(); + + Self { hint, hint_string } + } + + pub fn export_dyn_gd() -> Self + where + T: GodotClass + Bounds, + D: ?Sized + 'static, + { + PropertyHintInfo { + hint_string: GString::from(get_dyn_property_hint_string::()), + ..PropertyHintInfo::export_gd::() + } + } + + #[doc(hidden)] + pub fn object_as_node_class() -> Option + where + T: GodotClass + Bounds, + { + T::inherits::().then(|| T::class_name()) + } } diff --git a/godot-core/src/meta/sealed.rs b/godot-core/src/meta/sealed.rs index 37ab1b08b..d57b141c8 100644 --- a/godot-core/src/meta/sealed.rs +++ b/godot-core/src/meta/sealed.rs @@ -11,7 +11,7 @@ use crate::builtin::*; use crate::meta; use crate::meta::traits::{ArrayElement, GodotNullableFfi, GodotType}; -use crate::obj::{DynGd, Gd, GodotClass, RawGd}; +use crate::obj::{DynGd, Gd, GodotClass, OnEditor, RawGd}; pub trait Sealed {} impl Sealed for Aabb {} @@ -72,3 +72,4 @@ where T::Ffi: GodotNullableFfi, { } +impl Sealed for OnEditor where T: GodotType {} diff --git a/godot-core/src/obj/dyn_gd.rs b/godot-core/src/obj/dyn_gd.rs index 1e97a1b92..535c6112e 100644 --- a/godot-core/src/obj/dyn_gd.rs +++ b/godot-core/src/obj/dyn_gd.rs @@ -5,11 +5,11 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use crate::builtin::{GString, Variant}; +use crate::builtin::Variant; use crate::meta::error::ConvertError; use crate::meta::{ClassName, FromGodot, GodotConvert, PropertyHintInfo, ToGodot}; use crate::obj::guards::DynGdRef; -use crate::obj::{bounds, AsDyn, Bounds, DynGdMut, Gd, GodotClass, Inherits}; +use crate::obj::{bounds, AsDyn, Bounds, DynGdMut, Gd, GodotClass, Inherits, OnEditor}; use crate::registry::class::{get_dyn_property_hint_string, try_dynify_object}; use crate::registry::property::{object_export_element_type_string, Export, Var}; use crate::{meta, sys}; @@ -136,6 +136,25 @@ use std::{fmt, ops}; /// godot-rust achieves this thanks to the registration done by `#[godot_dyn]`: the library knows for which classes `Health` is implemented, /// and it can query the dynamic type of the object. Based on that type, it can find the `impl Health` implementation matching the correct class. /// Behind the scenes, everything is wired up correctly so that you can restore the original `DynGd` even after it has passed through Godot. +/// +/// # `#[export]` for `DynGd` +/// +/// Exporting `DynGd` is possible only via algebraic types such as [`OnEditor`] or [`Option`]. +/// `DynGd` can also be exported directly as an element of an array such as `Array>`. +/// +/// Since `DynGd` expresses shared functionality `D` among classes inheriting `T`, +/// `#[export]` for `DynGd` where `T` is a concrete Rust class is not allowed. +/// Consider using `Gd` instead in such cases. +/// +/// ## Node based classes +/// +/// `#[export]` for a `DynGd` works identically to `#[export]` `Gd` for `T` inheriting Node classes. +/// Godot will report an error if the conversion fails, but it will only do so when accessing the given value. +/// +/// ## Resource based classes +/// +/// `#[export]` for a `DynGd` allows you to limit the available choices to implementors of a given trait `D` whose base inherits the specified `T` +/// (for example, `#[export] DynGd` won't include Rust classes with an Object base, even if they implement `MyTrait`). pub struct DynGd where // T does _not_ require AsDyn here. Otherwise, it's impossible to upcast (without implementing the relation for all base classes). @@ -273,6 +292,22 @@ where pub fn into_gd(self) -> Gd { self.obj } + + /// Implementation shared between `OnEditor>` and `Option>`. + #[doc(hidden)] + fn set_property(&mut self, value: ::Via) + where + D: 'static, + { + // `set_property` can't be delegated to Gd, since we have to set `erased_obj` as well. + *self = ::from_godot(value); + } + + /// Implementation shared between `OnEditor>` and `Option>`. + #[doc(hidden)] + fn get_property(&self) -> ::Via { + self.obj.to_godot() + } } impl DynGd @@ -484,33 +519,90 @@ where } } -impl Var for DynGd +impl Var for Option> +where + T: GodotClass, + D: ?Sized + 'static, +{ + fn get_property(&self) -> Self::Via { + self.as_ref().map(|this| this.get_property()) + } + + fn set_property(&mut self, value: Self::Via) { + match (value, self.as_mut()) { + (Some(new_value), Some(current_value)) => current_value.set_property(new_value), + (Some(new_value), _) => *self = Some( as FromGodot>::from_godot(new_value)), + (None, _) => *self = None, + } + } +} + +/// `#[export]` for `Option>` is available only for `T` being Engine class (such as Node or Resource). +/// +/// Consider exporting `Option>` instead of `Option>` for user-declared GDExtension classes. +impl Export for Option> +where + T: GodotClass + Bounds, + D: ?Sized + 'static, +{ + fn export_hint() -> PropertyHintInfo { + PropertyHintInfo::export_dyn_gd::() + } + + #[doc(hidden)] + fn as_node_class() -> Option { + PropertyHintInfo::object_as_node_class::() + } +} + +impl Default for OnEditor> +where + T: GodotClass, + D: ?Sized + 'static, +{ + fn default() -> Self { + OnEditor::gd_invalid() + } +} + +impl GodotConvert for OnEditor> +where + T: GodotClass, + D: ?Sized, +{ + type Via = Option< as GodotConvert>::Via>; +} + +impl Var for OnEditor> where T: GodotClass, D: ?Sized + 'static, { fn get_property(&self) -> Self::Via { - self.obj.get_property() + OnEditor::>::get_property_inner(self, >::get_property) } fn set_property(&mut self, value: Self::Via) { // `set_property` can't be delegated to Gd, since we have to set `erased_obj` as well. - *self = ::from_godot(value); + OnEditor::>::set_property_inner(self, value, >::set_property) } } -impl Export for DynGd +/// `#[export]` for `OnEditor>` is available only for `T` being Engine class (such as Node or Resource). +/// +/// Consider exporting `OnEditor>` instead of `OnEditor>` for user-declared GDExtension classes. +impl Export for OnEditor> where - T: GodotClass + Bounds, + OnEditor>: Var, + T: GodotClass + Bounds, D: ?Sized + 'static, { fn export_hint() -> PropertyHintInfo { - PropertyHintInfo { - hint_string: GString::from(get_dyn_property_hint_string::()), - .. as Export>::export_hint() - } + PropertyHintInfo::export_dyn_gd::() } + + #[doc(hidden)] fn as_node_class() -> Option { - as Export>::as_node_class() + PropertyHintInfo::object_as_node_class::() } } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index c7bff68e3..7669e6bb2 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -12,7 +12,6 @@ use godot_ffi as sys; use sys::{static_assert_eq_size_align, SysPtr as _}; use crate::builtin::{Callable, NodePath, StringName, Variant}; -use crate::global::PropertyHint; use crate::meta::error::{ConvertError, FromFfiError}; use crate::meta::{ ArrayElement, AsArg, CallContext, ClassName, CowArg, FromGodot, GodotConvert, GodotType, @@ -20,7 +19,7 @@ use crate::meta::{ }; use crate::obj::{ bounds, cap, Bounds, DynGd, GdDerefTarget, GdMut, GdRef, GodotClass, Inherits, InstanceId, - RawGd, WithSignals, + OnEditor, RawGd, WithSignals, }; use crate::private::callbacks; use crate::registry::property::{object_export_element_type_string, Export, Var}; @@ -550,6 +549,18 @@ impl Gd { // Do not increment ref-count; assumed to be return value from FFI. sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys_weak(ptr)) } + + /// Implementation shared between `OnEditor>` and `Option`. + #[doc(hidden)] + fn set_property(&mut self, new_value: ::Via) { + *self = FromGodot::from_godot(new_value); + } + + /// Implementation shared between `OnEditor>` and `Option`. + #[doc(hidden)] + fn get_property(&self) -> ::Via { + self.to_godot() + } } /// _The methods in this impl block are only available for objects `T` that are manually managed, @@ -896,46 +907,78 @@ impl Clone for Gd { } } -// TODO: Do we even want to implement `Var` and `Export` for `Gd`? You basically always want to use `Option>` because the editor -// may otherwise try to set the object to a null value. -impl Var for Gd { +impl Var for Option> { fn get_property(&self) -> Self::Via { - self.to_godot() + self.as_ref().map(>::get_property) } fn set_property(&mut self, value: Self::Via) { - *self = FromGodot::from_godot(value) + match (value, self.as_mut()) { + (Some(new_value), Some(current_value)) => current_value.set_property(new_value), + (Some(new_value), _) => *self = Some( as FromGodot>::from_godot(new_value)), + (None, _) => *self = None, + } } } -impl Export for Gd +impl Export for Option> where T: GodotClass + Bounds, + Option>: Var, { fn export_hint() -> PropertyHintInfo { - let hint = if T::inherits::() { - PropertyHint::RESOURCE_TYPE - } else if T::inherits::() { - PropertyHint::NODE_TYPE - } else { - unreachable!("classes not inheriting from Resource or Node should not be exportable") - }; + PropertyHintInfo::export_gd::() + } + + #[doc(hidden)] + fn as_node_class() -> Option { + PropertyHintInfo::object_as_node_class::() + } +} + +impl Default for OnEditor> { + fn default() -> Self { + OnEditor::gd_invalid() + } +} - // Godot does this by default too; the hint is needed when the class is a resource/node, - // but doesn't seem to make a difference otherwise. - let hint_string = T::class_name().to_gstring(); +impl GodotConvert for OnEditor> +where + T: GodotClass, + Option< as GodotConvert>::Via>: GodotType, +{ + type Via = Option< as GodotConvert>::Via>; +} - PropertyHintInfo { hint, hint_string } +impl Var for OnEditor> +where + T: GodotClass, + OnEditor>: GodotConvert as GodotConvert>::Via>>, +{ + fn get_property(&self) -> Self::Via { + OnEditor::>::get_property_inner(self, >::get_property) + } + + fn set_property(&mut self, value: Self::Via) { + OnEditor::>::set_property_inner(self, value, >::set_property) + } +} + +impl Export for OnEditor> +where + T: GodotClass + Bounds, + OnEditor>: Var, +{ + fn export_hint() -> PropertyHintInfo { + PropertyHintInfo::export_gd::() } #[doc(hidden)] fn as_node_class() -> Option { - T::inherits::().then(|| T::class_name()) + PropertyHintInfo::object_as_node_class::() } } -// Trait impls Property, Export and TypeStringHint for Option> are covered by blanket impl for Option - impl PartialEq for Gd { /// ⚠️ Returns whether two `Gd` pointers point to the same object. /// diff --git a/godot-core/src/obj/mod.rs b/godot-core/src/obj/mod.rs index 5659ca7cb..fab6cc3aa 100644 --- a/godot-core/src/obj/mod.rs +++ b/godot-core/src/obj/mod.rs @@ -16,6 +16,7 @@ mod dyn_gd; mod gd; mod guards; mod instance_id; +mod oneditor; mod onready; mod raw_gd; mod traits; @@ -27,6 +28,7 @@ pub use dyn_gd::DynGd; pub use gd::*; pub use guards::{BaseMut, BaseRef, DynGdMut, DynGdRef, GdMut, GdRef}; pub use instance_id::*; +pub use oneditor::*; pub use onready::*; pub use raw_gd::*; pub use traits::*; diff --git a/godot-core/src/obj/oneditor.rs b/godot-core/src/obj/oneditor.rs new file mode 100644 index 000000000..2f2d20c4d --- /dev/null +++ b/godot-core/src/obj/oneditor.rs @@ -0,0 +1,308 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +use crate::meta::{FromGodot, GodotConvert, GodotType, PropertyHintInfo}; +use crate::registry::property::{BuiltinExport, Export, Var}; + +/// Exported property that must be initialized in the editor (or associated code) before use. +/// +/// Allows to use `Gd`, which by itself never holds null objects, as an `#[export]` that should not be null during runtime. +/// As such, it can be used as a more ergonomic way of `Option>` which _assumes_ initialization. +/// +/// Panics during access if uninitialized. +/// When used inside a node class, `OnEditor` checks if a value has been set before `ready()` is run, and panics otherwise. +/// Once initialized, it can be used almost as if it was a `T` value itself, due to `Deref`/`DerefMut` impls. +/// +/// `OnEditor` should always be used as a property, preferably in tandem with an `#[export]` or `#[var]`. +/// Initializing `OnEditor` values via code before the first use is supported but should be limited to use cases involving builder or factory patterns. +/// +/// [`Option>`](std::option) and [`OnReady>`](crate::obj::onready::OnReady) should be used for any other late initialization logic. +/// +/// # Using `OnEditor` with `Gd` and `DynGd` +/// +/// ## Example - auto-generated init +/// +/// ``` +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct MyClass { +/// #[export] +/// editor_property: OnEditor>, +/// } +/// +/// #[godot_api] +/// impl INode for MyClass { +/// fn ready(&mut self) { +/// // Will always be valid and **must** be set via editor. +/// // Additional check is being run before ready() +/// // to ensure that given value can't be null. +/// let some_variant = self.editor_property.get_meta("SomeName"); +/// } +/// } +/// +/// ``` +/// +/// ## Example - user-generated init +/// +/// Uninitialized `OnEditor>` and `OnEditor>` can be created with `OnEditor<...>::default()`. +/// +/// ``` +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(base = Node)] +/// struct MyClass { +/// #[export] +/// required_node: OnEditor>, +/// +/// base: Base +/// } +/// +/// #[godot_api] +/// impl INode for MyClass { +/// fn init(base: Base) -> Self { +/// Self { +/// base, +/// required_node: OnEditor::default(), +/// } +/// } +/// } +///``` +/// +/// ## Example - factory pattern +/// +/// ``` +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct SomeClass { +/// #[export] +/// required_node: OnEditor>, +/// } +/// +/// fn create_and_add( +/// mut this: Gd, +/// some_class_scene: Gd, +/// some_node: Gd, +/// ) -> Gd { +/// let mut my_node = some_class_scene.instantiate_as::(); +/// +/// // Would cause the panic: +/// // this.add_child(&my_node); +/// +/// // Note: Remember that nodes are manually managed. +/// // They will leak memory if not added to tree and/or pruned. +/// my_node.bind_mut().required_node.init(some_node); +/// +/// // Will not cause the panic. +/// this.add_child(&my_node); +/// +/// my_node +/// } +/// ``` +/// +/// # Using `OnEditor` with other GodotTypes +/// +/// `OnEditor` can be used with other built-ins to provide extra validation logic and making sure that given properties has been set. +/// Example usage might be checking if entities has been granted properly generated id. +/// +/// In such cases the value which will be deemed invalid **must** be specified with `#[init(uninit = val)]`. +/// Given `val` will be used to represent uninitialized `OnEditor` in the Godot editor. +/// Accessing uninitialized value will cause the panic. +/// +/// ## Example - using `OnEditor` with primitives +/// +/// ``` +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct SomeClassThatCanBeInstantiatedInCode { +/// // Uninitialized value will be represented by `42` in the editor. +/// // Will cause panic if not set via the editor or code before use. +/// #[export] +/// #[init(invalid = 42)] +/// some_primitive: OnEditor, +/// } +/// +/// fn create_and_add(mut this: Gd, val: i64) -> Gd { +/// let mut my_node = SomeClassThatCanBeInstantiatedInCode::new_alloc(); +/// +/// // Would cause the panic: +/// // this.add_child(&my_node); +/// +/// my_node.bind_mut().some_primitive.init(val); +/// +/// // Will not cause the panic. +/// this.add_child(&my_node); +/// +/// my_node +/// } +/// ``` +/// +pub struct OnEditor { + inner: OnEditorState, +} + +pub(crate) enum OnEditorState { + /// Uninitialized null value. + Null, + /// Uninitialized state, but with a value marked as invalid (required to represent non-nullable type in the editor). + Uninitialized(T), + /// Initialized with a value. + Initialized(T), +} + +impl OnEditor { + /// Initializes invalid `OnEditor` with given value. + /// + /// # Panics + /// If `init()` was called before. + pub fn init(&mut self, val: T) { + match self.inner { + OnEditorState::Null | OnEditorState::Uninitialized(_) => { + *self = OnEditor { + inner: OnEditorState::Initialized(val), + }; + } + OnEditorState::Initialized(_) => { + panic!("Given OnEditor value has been already initialized; did you call init() more than once?") + } + } + } + + /// Creates new `OnEditor` with a value that is considered invalid. + /// + /// If this value is not changed in the editor, accessing it from Rust will cause a panic. + pub fn new_invalid(val: T) -> Self + where + T: BuiltinExport, + { + OnEditor { + inner: OnEditorState::Uninitialized(val), + } + } + + /// Creates new uninitialized `OnEditor` value for nullable GodotTypes. + /// + /// Not a part of public API – available only via `Default` implementation on `OnEditor>` and `OnEditor>`. + pub(crate) fn gd_invalid() -> Self { + OnEditor { + inner: OnEditorState::Null, + } + } + + #[doc(hidden)] + pub fn is_invalid(&self) -> bool { + match self.inner { + OnEditorState::Null | OnEditorState::Uninitialized(_) => true, + OnEditorState::Initialized(_) => false, + } + } + + /// `Var::get_property` implementation that works both for nullable and non-nullable types. + pub(crate) fn get_property_inner( + &self, + get_property_fn: impl FnOnce(&T) -> T::Via, + ) -> Option { + match &self.inner { + OnEditorState::Null => None, + OnEditorState::Uninitialized(val) | OnEditorState::Initialized(val) => { + Some(get_property_fn(val)) + } + } + } + + /// [`Var::set_property`] implementation that works both for nullable and non-nullable types. + /// + /// All the state transitions are valid, since it is being run only in the editor. + /// See also [`Option::set_property()`]. + pub(crate) fn set_property_inner( + &mut self, + value: Option, + set_property_fn: impl FnOnce(&mut T, T::Via), + ) where + T::Via: PartialEq, + { + match (value, &mut self.inner) { + (None, _) => self.inner = OnEditorState::Null, + (Some(value), OnEditorState::Initialized(current_value)) => { + set_property_fn(current_value, value) + } + (Some(value), OnEditorState::Null) => { + self.inner = OnEditorState::Initialized(FromGodot::from_godot(value)) + } + (Some(value), OnEditorState::Uninitialized(current_value)) => { + let value = FromGodot::from_godot(value); + if value != *current_value { + self.inner = OnEditorState::Initialized(value) + } + } + } + } +} + +impl std::ops::Deref for OnEditor { + type Target = T; + fn deref(&self) -> &Self::Target { + match &self.inner { + OnEditorState::Null | OnEditorState::Uninitialized(_) => { + panic!("OnEditor field hasn't been initialized.") + } + OnEditorState::Initialized(v) => v, + } + } +} + +impl std::ops::DerefMut for OnEditor { + fn deref_mut(&mut self) -> &mut Self::Target { + match &mut self.inner { + OnEditorState::Null | OnEditorState::Uninitialized(_) => { + panic!("OnEditor field hasn't been initialized.") + } + OnEditorState::Initialized(v) => v, + } + } +} + +impl GodotConvert for OnEditor +where + T: GodotType + GodotConvert + BuiltinExport, +{ + type Via = T::Via; +} + +impl Var for OnEditor +where + OnEditor: GodotConvert, + T: GodotConvert + BuiltinExport + Var + FromGodot + PartialEq, +{ + fn get_property(&self) -> Self::Via { + // Will never fail – `PrimitiveGodotType` can not be represented by the `OnEditorState::Null`. + OnEditor::::get_property_inner(self, T::get_property) + .expect("DirectExport is not nullable.") + } + + fn set_property(&mut self, value: T) { + OnEditor::::set_property_inner(self, Some(value), T::set_property); + } +} + +impl Export for OnEditor +where + OnEditor: Var, + T: GodotConvert + BuiltinExport + Export, +{ + fn export_hint() -> PropertyHintInfo { + T::export_hint() + } +} + +impl BuiltinExport for OnEditor {} diff --git a/godot-core/src/registry/class.rs b/godot-core/src/registry/class.rs index 65f1b7fcc..a0b02cc80 100644 --- a/godot-core/src/registry/class.rs +++ b/godot-core/src/registry/class.rs @@ -365,11 +365,10 @@ where **this is a bug, please report it**", trait_name = sys::short_type_name::() ); - let relations_iter = relations.iter(); // Include only implementors inheriting given T. // For example – don't include Nodes or Objects while creating hint_string for Resource. - let relations_iter = relations_iter.filter_map(|implementor| { + let relations_iter = relations.iter().filter_map(|implementor| { // TODO – check if caching it (using is_derived_base_cached) yields any benefits. if ClassDb::singleton().is_parent_class( &implementor.parent_class_name?.to_string_name(), diff --git a/godot-core/src/registry/property.rs b/godot-core/src/registry/property.rs index ec2da87c6..e7eb85e02 100644 --- a/godot-core/src/registry/property.rs +++ b/godot-core/src/registry/property.rs @@ -82,6 +82,22 @@ pub trait Export: Var { } } +/// Marker trait to identify `GodotType`s that can be directly used with an `#[export]`. +/// +/// Implemented pretty much for all the [`GodotTypes`][GodotType] that are not [`GodotClass`]. +/// Provides a few blanket implementations and, by itself, has no implications +/// for the [`Var`] or [`Export`] traits. +/// +/// Some Godot Types which are inherently non-nullable (e.g., `Gd`), +/// might have their value set to null by the editor. Additionally, Godot must generate +/// initial, default value for such properties, causing memory leaks. +/// +/// Non-algebraic types that don't implement `BuiltinExport` shouldn't be used directly as an `#[export]` +/// and must be handled using associated algebraic types, such as: +/// * [`Option`], which represents optional value that can be null when used. +/// * [`OnEditor`][crate::obj::OnEditor], which represents value that must not be null when used. +pub trait BuiltinExport {} + /// This function only exists as a place to add doc-tests for the `Export` trait. /// /// Test with export of exportable type should succeed: @@ -159,6 +175,8 @@ where } } +impl BuiltinExport for Option {} + // ---------------------------------------------------------------------------------------------------------------------------------------------- // Export machinery @@ -432,6 +450,7 @@ mod export_impls { ($Ty:ty) => { impl_property_by_godot_convert!(@property $Ty); impl_property_by_godot_convert!(@export $Ty); + impl_property_by_godot_convert!(@builtin $Ty); }; (@property $Ty:ty) => { @@ -453,6 +472,10 @@ mod export_impls { } } }; + + (@builtin $Ty:ty) => { + impl BuiltinExport for $Ty {} + } } // Bounding Boxes diff --git a/godot-macros/src/class/data_models/field.rs b/godot-macros/src/class/data_models/field.rs index a96ec8454..e05eabc70 100644 --- a/godot-macros/src/class/data_models/field.rs +++ b/godot-macros/src/class/data_models/field.rs @@ -16,6 +16,7 @@ pub struct Field { pub var: Option, pub export: Option, pub is_onready: bool, + pub is_oneditor: bool, #[cfg(feature = "register-docs")] pub attributes: Vec, pub span: Span, @@ -30,6 +31,7 @@ impl Field { var: None, export: None, is_onready: false, + is_oneditor: false, #[cfg(feature = "register-docs")] attributes: field.attributes.clone(), span: field.span(), diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs index 17775d6f0..b20d53a15 100644 --- a/godot-macros/src/class/derive_godot_class.rs +++ b/godot-macros/src/class/derive_godot_class.rs @@ -253,6 +253,78 @@ fn make_godot_init_impl(class_name: &Ident, fields: &Fields) -> TokenStream { } } +fn make_onready_init(all_fields: &[Field]) -> TokenStream { + let onready_fields = all_fields + .iter() + .filter(|&field| field.is_onready) + .map(|field| { + let field = &field.name; + quote! { + ::godot::private::auto_init(&mut self.#field, &base); + } + }) + .collect::>(); + + if !onready_fields.is_empty() { + quote! { + { + let base = ::to_gd(self).upcast(); + #( #onready_fields )* + } + } + } else { + TokenStream::new() + } +} + +fn make_oneditor_panic_inits(class_name: &Ident, all_fields: &[Field]) -> TokenStream { + // Despite its name OnEditor shouldn't panic in the editor for tool classes. + let is_in_editor = quote! { ::godot::classes::Engine::singleton().is_editor_hint() }; + + let are_all_oneditor_fields_valid = quote! { are_all_oneditor_fields_valid }; + + // Informs the user which fields haven't been set, instead of panicking on the very first one. Useful for debugging. + let on_editor_fields_checks = all_fields + .iter() + .filter(|&field| field.is_oneditor) + .map(|field| { + let field = &field.name; + let warning_message = + format! { "godot-rust: OnEditor field {field} hasn't been initialized."}; + + quote! { + if this.#field.is_invalid() { + ::godot::global::godot_warn!(#warning_message); + #are_all_oneditor_fields_valid = false; + } + } + }) + .collect::>(); + + if !on_editor_fields_checks.is_empty() { + quote! { + fn __are_oneditor_fields_initalized(this: &#class_name) -> bool { + // Early return for `#[class(tool)]`. + if #is_in_editor { + return true; + } + + let mut #are_all_oneditor_fields_valid: bool = true; + + #( #on_editor_fields_checks )* + + #are_all_oneditor_fields_valid + } + + if !__are_oneditor_fields_initalized(&self) { + panic!("OnEditor fields must be properly initialized before ready.") + } + } + } else { + TokenStream::new() + } +} + fn make_user_class_impl( class_name: &Ident, is_tool: bool, @@ -264,31 +336,13 @@ fn make_user_class_impl( #[cfg(not(feature = "codegen-full"))] let rpc_registrations = TokenStream::new(); - let onready_inits = { - let mut onready_fields = all_fields - .iter() - .filter(|&field| field.is_onready) - .map(|field| { - let field = &field.name; - quote! { - ::godot::private::auto_init(&mut self.#field, &base); - } - }); + let onready_inits = make_onready_init(all_fields); - if let Some(first) = onready_fields.next() { - quote! { - { - let base = ::to_gd(self).upcast(); - #first - #( #onready_fields )* - } - } - } else { - TokenStream::new() - } - }; + let oneditor_panic_inits = make_oneditor_panic_inits(class_name, all_fields); - let default_virtual_fn = if all_fields.iter().any(|field| field.is_onready) { + let run_before_ready = !onready_inits.is_empty() || !oneditor_panic_inits.is_empty(); + + let default_virtual_fn = if run_before_ready { let tool_check = util::make_virtual_tool_check(); let signature_info = SignatureInfo::fn_ready(); @@ -338,6 +392,7 @@ fn make_user_class_impl( #[doc(hidden)] fn __before_ready(&mut self) { + #oneditor_panic_inits #rpc_registrations #onready_inits } @@ -463,6 +518,11 @@ fn parse_fields( field.is_onready = true; } + // OnEditor type inference + if path_ends_with_complex(&field.ty, "OnEditor") { + field.is_oneditor = true; + } + // #[init] if let Some(mut parser) = KvParser::parse(&named_field.attributes, "init")? { // #[init] on fields is useless if there is no generated constructor. @@ -535,6 +595,37 @@ fn parse_fields( }); } + // #[init(invalid = val)] + if let Some(invalid_representation) = parser.handle_expr("invalid")? { + let mut is_well_formed = true; + if !field.is_oneditor { + is_well_formed = false; + errors.push(error!( + parser.span(), + "The key `invalid` in attribute #[init] requires field of type `OnEditor`" + )); + } + + if field.default_val.is_some() { + is_well_formed = false; + errors.push(error!( + parser.span(), + "The key `invalid` in attribute #[init] is mutually exclusive with the keys `default` and `val`" + )); + } + + let default_val = if is_well_formed { + quote! { OnEditor::new_invalid( #invalid_representation ) } + } else { + quote! { todo!() } + }; + + field.default_val = Some(FieldDefault { + default_val, + span: parser.span(), + }); + } + parser.finish()?; } diff --git a/godot/src/prelude.rs b/godot/src/prelude.rs index 0f3bb81e1..4445a3847 100644 --- a/godot/src/prelude.rs +++ b/godot/src/prelude.rs @@ -28,7 +28,7 @@ pub use super::tools::{load, save, try_load, try_save, GFile}; pub use super::init::{gdextension, ExtensionLibrary, InitLevel}; pub use super::obj::{ AsDyn, Base, DynGd, DynGdMut, DynGdRef, Gd, GdMut, GdRef, GodotClass, Inherits, InstanceId, - OnReady, + OnEditor, OnReady, }; // Make trait methods available. diff --git a/itest/godot/ManualFfiTests.gd b/itest/godot/ManualFfiTests.gd index 92e5a7044..cbc384da1 100644 --- a/itest/godot/ManualFfiTests.gd +++ b/itest/godot/ManualFfiTests.gd @@ -69,7 +69,7 @@ func test_export(): node.free() func test_export_dyn_gd(): - var dyn_gd_exporter = RefcDynGdExporter.new() + var dyn_gd_exporter = RefcDynGdVarDeclarer.new() # NodeHealth is valid candidate both for `empty` and `second` fields. var node = NodeHealth.new() @@ -89,7 +89,7 @@ func test_export_dyn_gd_should_fail_for_wrong_type(): if runs_release(): return - var dyn_gd_exporter = RefcDynGdExporter.new() + var dyn_gd_exporter = RefcDynGdVarDeclarer.new() var refc = RefcHealth.new() disable_error_messages() @@ -321,6 +321,7 @@ func test_option_export(): assert_eq(obj.optional_export, null) test_node.free() + obj.free() func test_func_rename(): var func_rename := FuncObj.new() diff --git a/itest/rust/src/object_tests/dyn_gd_test.rs b/itest/rust/src/object_tests/dyn_gd_test.rs index 937b69271..6e1ea571e 100644 --- a/itest/rust/src/object_tests/dyn_gd_test.rs +++ b/itest/rust/src/object_tests/dyn_gd_test.rs @@ -507,24 +507,37 @@ impl InstanceIdProvider for foreign::NodeHealth { } // ---------------------------------------------------------------------------------------------------------------------------------------------- -// Check if DynGd can be properly exported +// Checks if DynGd can be properly used as a `#[var]`. +// All classes can be used as a `#[var]` for `DynGd`. #[derive(GodotClass)] #[class(init)] -struct RefcDynGdExporter { +struct RefcDynGdVarDeclarer { #[var] first: Option>, - // Using DynGd with concrete type foreign::NodeHealth doesn't give benefits over Gd, but is allowed in godot-rust 0.2.x. - #[export] + #[var] second: Option>>, } // Implementation created only to register the DynGd `HealthWithAssociatedType` trait. // Pointless trait, but tests proper conversion. #[godot_dyn] -impl InstanceIdProvider for RefcDynGdExporter { +impl InstanceIdProvider for RefcDynGdVarDeclarer { type Id = f32; fn get_id_dynamic(&self) -> Self::Id { 42.0 } } + +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Checks if `#[export]`s for DynGd can be properly auto-generated. +// Only built-in classes can be used as an `#[export]` for `DynGd`. + +#[derive(GodotClass)] +#[class(init, base=Node)] +struct DynGdExporter { + #[export] + first: Option>, + #[export] + second: OnEditor>, +} diff --git a/itest/rust/src/object_tests/mod.rs b/itest/rust/src/object_tests/mod.rs index a5adf046b..dc987d078 100644 --- a/itest/rust/src/object_tests/mod.rs +++ b/itest/rust/src/object_tests/mod.rs @@ -18,6 +18,7 @@ mod init_level_test; mod object_arg_test; mod object_swap_test; mod object_test; +mod oneditor_test; mod onready_test; mod property_template_test; mod property_test; diff --git a/itest/rust/src/object_tests/oneditor_test.rs b/itest/rust/src/object_tests/oneditor_test.rs new file mode 100644 index 000000000..df7e27d58 --- /dev/null +++ b/itest/rust/src/object_tests/oneditor_test.rs @@ -0,0 +1,85 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +use crate::framework::{expect_panic, itest}; +use godot::classes::notify::NodeNotification; +use godot::classes::{INode, Node, RefCounted}; +use godot::register::{godot_api, GodotClass}; + +use godot::obj::{Gd, NewAlloc, OnEditor}; + +#[itest] +fn oneditor_deref() { + let mut on_editor = OnEditor::new_invalid(0); + on_editor.init(42); + assert_eq!(*on_editor, 42); + + *on_editor = 44; + assert_eq!(*on_editor, 44); +} + +#[itest] +fn oneditor_no_value_panic_on_deref_primitive() { + expect_panic("Deref on null fails for primitive", || { + let on_editor_panic: OnEditor = OnEditor::new_invalid(0); + let _ref: &i64 = &on_editor_panic; + }); + expect_panic("Deref on null fails for Gd class", || { + let on_editor_panic: OnEditor> = OnEditor::default(); + let _ref: &Gd = &on_editor_panic; + }); + + expect_panic("DerefMut on null fails for primitive", || { + let mut on_editor_panic: OnEditor = OnEditor::new_invalid(0); + let _ref: &mut i64 = &mut on_editor_panic; + }); + expect_panic("DerefMut on null fails for Gd class", || { + let mut on_editor_panic: OnEditor> = OnEditor::default(); + let _ref: &mut Gd = &mut on_editor_panic; + }); +} + +#[itest] +fn oneditor_panic_on_ready() { + let mut obj = OnEditorNoDefault::new_alloc(); + + // causes the panic which is NOT propagated to godot-rust but prevents `ready` from being run. + obj.notify(NodeNotification::READY); + assert!(!obj.bind().was_ready_run); + obj.free(); +} + +#[itest] +fn oneditor_no_panic_on_ready() { + let mut obj = OnEditorNoDefault::new_alloc(); + obj.bind_mut().node_field.init(Node::new_alloc()); + obj.bind_mut().some_primitive.init(64); + obj.notify(NodeNotification::READY); + assert!(obj.bind().was_ready_run); + obj.bind_mut().node_field.clone().free(); + obj.free(); +} + +#[derive(GodotClass)] +#[class(init, base=Node)] +struct OnEditorNoDefault { + #[export] + #[init(invalid = 0)] + some_primitive: OnEditor, + #[export] + node_field: OnEditor>, + + /// Informs whether `ready()` has been run (false if a panic occurred). + was_ready_run: bool, +} + +#[godot_api] +impl INode for OnEditorNoDefault { + fn ready(&mut self) { + self.was_ready_run = true; + } +} diff --git a/itest/rust/src/object_tests/property_test.rs b/itest/rust/src/object_tests/property_test.rs index 1b878607b..d6643f9a2 100644 --- a/itest/rust/src/object_tests/property_test.rs +++ b/itest/rust/src/object_tests/property_test.rs @@ -9,7 +9,7 @@ use godot::builtin::{dict, Color, Dictionary, GString, Variant, VariantType}; use godot::classes::{INode, IRefCounted, Node, Object, RefCounted, Resource, Texture}; use godot::global::{PropertyHint, PropertyUsageFlags}; use godot::meta::{GodotConvert, PropertyHintInfo, ToGodot}; -use godot::obj::{Base, EngineBitfield, EngineEnum, Gd, NewAlloc, NewGd}; +use godot::obj::{Base, EngineBitfield, EngineEnum, Gd, NewAlloc, NewGd, OnEditor}; use godot::register::property::{Export, Var}; use godot::register::{godot_api, Export, GodotClass, GodotConvert, Var}; use godot::test::itest; @@ -44,7 +44,7 @@ struct HasProperty { object_val: Option>, #[var] - texture_val: Gd, + texture_val: OnEditor>, #[var(get = get_texture_val, set = set_texture_val, hint = RESOURCE_TYPE, hint_string = "Texture")] texture_val_rw: Option>, @@ -139,7 +139,7 @@ impl INode for HasProperty { int_val_setter: 0, object_val: None, string_val: GString::new(), - texture_val: Texture::new_gd(), + texture_val: OnEditor::default(), texture_val_rw: None, } } diff --git a/itest/rust/src/register_tests/option_ffi_test.rs b/itest/rust/src/register_tests/option_ffi_test.rs index 17fc07d9b..879d195de 100644 --- a/itest/rust/src/register_tests/option_ffi_test.rs +++ b/itest/rust/src/register_tests/option_ffi_test.rs @@ -97,7 +97,7 @@ impl OptionFfiTest { } #[derive(GodotClass)] -#[class(init)] +#[class(init, base=Node)] struct OptionExportFfiTest { #[var] optional: Option>, From 49c27dec6ab0529fc66f9b42959de610603837cb Mon Sep 17 00:00:00 2001 From: Yarvin Date: Fri, 7 Mar 2025 13:28:16 +0100 Subject: [PATCH 2/4] Add `OnEditor`, remove `Export` implementations for `DynGd` and `Gd` - Restore Var implementation. - Allow to use `OnEditor` for types that are represented by `BuiltinExport` (like converted enums represented in godot as i64 and whatnot) - Add proper bounds for `impl BuiltinExport for Option` - Remove `Sealed` for `OnEditor` since it uses Option implementation - Extend & tweak docs according to CR comments - Add information about skipping before-ready checks for `#[class(tool)]` - Include `compile_fail` doctests and examples. --- godot-core/src/meta/sealed.rs | 3 +- godot-core/src/obj/dyn_gd.rs | 76 ++++++++++++++++++----------- godot-core/src/obj/gd.rs | 26 ++-------- godot-core/src/obj/oneditor.rs | 46 +++++++++-------- godot-core/src/registry/property.rs | 51 ++++++++++++++++--- 5 files changed, 120 insertions(+), 82 deletions(-) diff --git a/godot-core/src/meta/sealed.rs b/godot-core/src/meta/sealed.rs index d57b141c8..37ab1b08b 100644 --- a/godot-core/src/meta/sealed.rs +++ b/godot-core/src/meta/sealed.rs @@ -11,7 +11,7 @@ use crate::builtin::*; use crate::meta; use crate::meta::traits::{ArrayElement, GodotNullableFfi, GodotType}; -use crate::obj::{DynGd, Gd, GodotClass, OnEditor, RawGd}; +use crate::obj::{DynGd, Gd, GodotClass, RawGd}; pub trait Sealed {} impl Sealed for Aabb {} @@ -72,4 +72,3 @@ where T::Ffi: GodotNullableFfi, { } -impl Sealed for OnEditor where T: GodotType {} diff --git a/godot-core/src/obj/dyn_gd.rs b/godot-core/src/obj/dyn_gd.rs index 535c6112e..8a08983e1 100644 --- a/godot-core/src/obj/dyn_gd.rs +++ b/godot-core/src/obj/dyn_gd.rs @@ -139,12 +139,51 @@ use std::{fmt, ops}; /// /// # `#[export]` for `DynGd` /// -/// Exporting `DynGd` is possible only via algebraic types such as [`OnEditor`] or [`Option`]. +/// Exporting `DynGd` is possible only via [`OnEditor`] or [`Option`]. /// `DynGd` can also be exported directly as an element of an array such as `Array>`. /// /// Since `DynGd` expresses shared functionality `D` among classes inheriting `T`, /// `#[export]` for `DynGd` where `T` is a concrete Rust class is not allowed. -/// Consider using `Gd` instead in such cases. +/// +/// ```compile_fail +/// +/// use godot::prelude::*; +/// +/// trait Health { /* ... */ } +/// +/// #[derive(GodotClass)] +/// # #[class(init)] +/// struct Monster { /* ... */ } +/// +/// #[godot_dyn] +/// impl Health for Monster { /* ... */ } +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct MyClass { +/// #[export] +/// dyn_concrete: Option>, +/// } +/// ``` +/// +/// Consider using `Gd` instead in such cases: +/// +/// ``` +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct Monster { /* ... */ } +/// +/// /* ... */ +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct MyClass { +/// #[export] +/// dyn_concrete: Option>, +/// } +/// ``` /// /// ## Node based classes /// @@ -154,7 +193,7 @@ use std::{fmt, ops}; /// ## Resource based classes /// /// `#[export]` for a `DynGd` allows you to limit the available choices to implementors of a given trait `D` whose base inherits the specified `T` -/// (for example, `#[export] DynGd` won't include Rust classes with an Object base, even if they implement `MyTrait`). +/// (for example, `#[export] Option>` won't include Rust classes with an Object base, even if they implement `MyTrait`). pub struct DynGd where // T does _not_ require AsDyn here. Otherwise, it's impossible to upcast (without implementing the relation for all base classes). @@ -292,22 +331,6 @@ where pub fn into_gd(self) -> Gd { self.obj } - - /// Implementation shared between `OnEditor>` and `Option>`. - #[doc(hidden)] - fn set_property(&mut self, value: ::Via) - where - D: 'static, - { - // `set_property` can't be delegated to Gd, since we have to set `erased_obj` as well. - *self = ::from_godot(value); - } - - /// Implementation shared between `OnEditor>` and `Option>`. - #[doc(hidden)] - fn get_property(&self) -> ::Via { - self.obj.to_godot() - } } impl DynGd @@ -519,21 +542,18 @@ where } } -impl Var for Option> +impl Var for DynGd where T: GodotClass, D: ?Sized + 'static, { fn get_property(&self) -> Self::Via { - self.as_ref().map(|this| this.get_property()) + self.obj.get_property() } fn set_property(&mut self, value: Self::Via) { - match (value, self.as_mut()) { - (Some(new_value), Some(current_value)) => current_value.set_property(new_value), - (Some(new_value), _) => *self = Some( as FromGodot>::from_godot(new_value)), - (None, _) => *self = None, - } + // `set_property` can't be delegated to Gd, since we have to set `erased_obj` as well. + *self = ::from_godot(value); } } @@ -579,12 +599,12 @@ where D: ?Sized + 'static, { fn get_property(&self) -> Self::Via { - OnEditor::>::get_property_inner(self, >::get_property) + OnEditor::>::get_property_inner(self) } fn set_property(&mut self, value: Self::Via) { // `set_property` can't be delegated to Gd, since we have to set `erased_obj` as well. - OnEditor::>::set_property_inner(self, value, >::set_property) + OnEditor::>::set_property_inner(self, value) } } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 7669e6bb2..c0c2ac892 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -549,18 +549,6 @@ impl Gd { // Do not increment ref-count; assumed to be return value from FFI. sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys_weak(ptr)) } - - /// Implementation shared between `OnEditor>` and `Option`. - #[doc(hidden)] - fn set_property(&mut self, new_value: ::Via) { - *self = FromGodot::from_godot(new_value); - } - - /// Implementation shared between `OnEditor>` and `Option`. - #[doc(hidden)] - fn get_property(&self) -> ::Via { - self.to_godot() - } } /// _The methods in this impl block are only available for objects `T` that are manually managed, @@ -907,17 +895,13 @@ impl Clone for Gd { } } -impl Var for Option> { +impl Var for Gd { fn get_property(&self) -> Self::Via { - self.as_ref().map(>::get_property) + self.to_godot() } fn set_property(&mut self, value: Self::Via) { - match (value, self.as_mut()) { - (Some(new_value), Some(current_value)) => current_value.set_property(new_value), - (Some(new_value), _) => *self = Some( as FromGodot>::from_godot(new_value)), - (None, _) => *self = None, - } + *self = FromGodot::from_godot(value) } } @@ -956,11 +940,11 @@ where OnEditor>: GodotConvert as GodotConvert>::Via>>, { fn get_property(&self) -> Self::Via { - OnEditor::>::get_property_inner(self, >::get_property) + OnEditor::>::get_property_inner(self) } fn set_property(&mut self, value: Self::Via) { - OnEditor::>::set_property_inner(self, value, >::set_property) + OnEditor::>::set_property_inner(self, value) } } diff --git a/godot-core/src/obj/oneditor.rs b/godot-core/src/obj/oneditor.rs index 2f2d20c4d..c400ffdf6 100644 --- a/godot-core/src/obj/oneditor.rs +++ b/godot-core/src/obj/oneditor.rs @@ -11,7 +11,7 @@ use crate::registry::property::{BuiltinExport, Export, Var}; /// Exported property that must be initialized in the editor (or associated code) before use. /// /// Allows to use `Gd`, which by itself never holds null objects, as an `#[export]` that should not be null during runtime. -/// As such, it can be used as a more ergonomic way of `Option>` which _assumes_ initialization. +/// As such, it can be used as a more ergonomic version of `Option>` which _assumes_ initialization. /// /// Panics during access if uninitialized. /// When used inside a node class, `OnEditor` checks if a value has been set before `ready()` is run, and panics otherwise. @@ -147,6 +147,11 @@ use crate::registry::property::{BuiltinExport, Export, Var}; /// } /// ``` /// +/// # Using `OnEditor` with `#[class(tool)]` +/// +/// When used with `#[class(tool)]`, the before-ready checks are omitted. +/// Otherwise, `OnEditor` behaves the same — accessing an uninitialized value +/// will cause a panic. pub struct OnEditor { inner: OnEditorState, } @@ -160,7 +165,7 @@ pub(crate) enum OnEditorState { Initialized(T), } -impl OnEditor { +impl OnEditor { /// Initializes invalid `OnEditor` with given value. /// /// # Panics @@ -183,7 +188,7 @@ impl OnEditor { /// If this value is not changed in the editor, accessing it from Rust will cause a panic. pub fn new_invalid(val: T) -> Self where - T: BuiltinExport, + T::Via: BuiltinExport, { OnEditor { inner: OnEditorState::Uninitialized(val), @@ -208,14 +213,11 @@ impl OnEditor { } /// `Var::get_property` implementation that works both for nullable and non-nullable types. - pub(crate) fn get_property_inner( - &self, - get_property_fn: impl FnOnce(&T) -> T::Via, - ) -> Option { + pub(crate) fn get_property_inner(&self) -> Option { match &self.inner { OnEditorState::Null => None, OnEditorState::Uninitialized(val) | OnEditorState::Initialized(val) => { - Some(get_property_fn(val)) + Some(val.get_property()) } } } @@ -224,17 +226,11 @@ impl OnEditor { /// /// All the state transitions are valid, since it is being run only in the editor. /// See also [`Option::set_property()`]. - pub(crate) fn set_property_inner( - &mut self, - value: Option, - set_property_fn: impl FnOnce(&mut T, T::Via), - ) where - T::Via: PartialEq, - { + pub(crate) fn set_property_inner(&mut self, value: Option) { match (value, &mut self.inner) { (None, _) => self.inner = OnEditorState::Null, (Some(value), OnEditorState::Initialized(current_value)) => { - set_property_fn(current_value, value) + current_value.set_property(value); } (Some(value), OnEditorState::Null) => { self.inner = OnEditorState::Initialized(FromGodot::from_godot(value)) @@ -274,31 +270,33 @@ impl std::ops::DerefMut for OnEditor { impl GodotConvert for OnEditor where - T: GodotType + GodotConvert + BuiltinExport, + T: GodotConvert, + T::Via: GodotType + BuiltinExport, { type Via = T::Via; } impl Var for OnEditor where - OnEditor: GodotConvert, - T: GodotConvert + BuiltinExport + Var + FromGodot + PartialEq, + OnEditor: GodotConvert, + T: Var + FromGodot + PartialEq, + T::Via: BuiltinExport, { fn get_property(&self) -> Self::Via { // Will never fail – `PrimitiveGodotType` can not be represented by the `OnEditorState::Null`. - OnEditor::::get_property_inner(self, T::get_property) - .expect("DirectExport is not nullable.") + OnEditor::::get_property_inner(self).expect("DirectExport is not nullable.") } - fn set_property(&mut self, value: T) { - OnEditor::::set_property_inner(self, Some(value), T::set_property); + fn set_property(&mut self, value: T::Via) { + OnEditor::::set_property_inner(self, Some(value)); } } impl Export for OnEditor where OnEditor: Var, - T: GodotConvert + BuiltinExport + Export, + T: GodotConvert + Export, + T::Via: BuiltinExport, { fn export_hint() -> PropertyHintInfo { T::export_hint() diff --git a/godot-core/src/registry/property.rs b/godot-core/src/registry/property.rs index e7eb85e02..ac2f28367 100644 --- a/godot-core/src/registry/property.rs +++ b/godot-core/src/registry/property.rs @@ -10,7 +10,7 @@ use crate::classes; use crate::global::PropertyHint; use godot_ffi as sys; -use godot_ffi::VariantType; +use godot_ffi::{GodotNullableFfi, VariantType}; use std::fmt::Display; use crate::meta::{ClassName, FromGodot, GodotConvert, GodotType, PropertyHintInfo, ToGodot}; @@ -88,14 +88,22 @@ pub trait Export: Var { /// Provides a few blanket implementations and, by itself, has no implications /// for the [`Var`] or [`Export`] traits. /// -/// Some Godot Types which are inherently non-nullable (e.g., `Gd`), -/// might have their value set to null by the editor. Additionally, Godot must generate -/// initial, default value for such properties, causing memory leaks. -/// -/// Non-algebraic types that don't implement `BuiltinExport` shouldn't be used directly as an `#[export]` +/// Types which don't implement the `BuiltinExport` trait can't be used directly as an `#[export]` /// and must be handled using associated algebraic types, such as: /// * [`Option`], which represents optional value that can be null when used. /// * [`OnEditor`][crate::obj::OnEditor], which represents value that must not be null when used. +// Some Godot Types which are inherently non-nullable (e.g., `Gd`), +// might have their value set to null by the editor. Additionally, Godot must generate +// initial, default value for such properties, causing memory leaks. +// Such `GodotType`s don't implement `BuiltinExport`. +// +// Note: This marker trait is required to create a blanket implementation +// for `OnEditor` where `T` is anything other than `GodotClass`. +// An alternative approach would involve introducing an extra associated type +// to `GodotType` trait. However, this would not be ideal — `GodotType` is used +// in contexts unrelated to `#[export]`, and adding unnecessary complexity +// should be avoided. Since Rust does not yet support specialization (i.e. negative trait bounds), +// this `MarkerTrait` serves as the intended solution to recognize aforementioned types. pub trait BuiltinExport {} /// This function only exists as a place to add doc-tests for the `Export` trait. @@ -126,6 +134,30 @@ pub trait BuiltinExport {} /// } /// ``` /// +/// Neither `Gd` nor `DynGd` can be used with an `#[export]` directly: +/// +/// ```compile_fail +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct MyClass { +/// #[export] +/// editor_property: Gd, +/// } +/// ``` +/// +/// ```compile_fail +/// use godot::prelude::*; +/// +/// #[derive(GodotClass)] +/// #[class(init, base = Node)] +/// struct MyClass { +/// #[export] +/// editor_property: DynGd, +/// } +/// ``` +/// /// ```compile_fail /// use godot::prelude::*; /// @@ -175,7 +207,12 @@ where } } -impl BuiltinExport for Option {} +impl BuiltinExport for Option +where + T: GodotType, + T::Ffi: GodotNullableFfi, +{ +} // ---------------------------------------------------------------------------------------------------------------------------------------------- // Export machinery From cdcdadc4f90b82052fe419cd216b35c55ff3e967 Mon Sep 17 00:00:00 2001 From: Yarvin Date: Fri, 7 Mar 2025 14:00:42 +0100 Subject: [PATCH 3/4] Add `OnEditor`, remove `Export` implementations for `DynGd` and `Gd` - Make clear that OnEditor works with `#[var]` properties and as a rust field set on struct as well --- godot-core/src/obj/oneditor.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/godot-core/src/obj/oneditor.rs b/godot-core/src/obj/oneditor.rs index c400ffdf6..3149783c4 100644 --- a/godot-core/src/obj/oneditor.rs +++ b/godot-core/src/obj/oneditor.rs @@ -15,6 +15,7 @@ use crate::registry::property::{BuiltinExport, Export, Var}; /// /// Panics during access if uninitialized. /// When used inside a node class, `OnEditor` checks if a value has been set before `ready()` is run, and panics otherwise. +/// This validation is performed for all `OnEditor` fields declared in a given `GodotClass`, regardless of whether they are `#[var]`, `#[export]`, or neither. /// Once initialized, it can be used almost as if it was a `T` value itself, due to `Deref`/`DerefMut` impls. /// /// `OnEditor` should always be used as a property, preferably in tandem with an `#[export]` or `#[var]`. From f33932379a132a4ea61297a4f23c86b4c60b5925 Mon Sep 17 00:00:00 2001 From: Yarvin Date: Sat, 8 Mar 2025 07:49:34 +0100 Subject: [PATCH 4/4] Add `OnEditor`, remove `Export` implementations for `DynGd` and `Gd` - Clean up `OnEditor<...>` bounds. - Make `DynGd, D>` exportable with `Option` & `DynGd` - Fix export for `DynGd + Bounds, D>` - Code tweaks according to CR --- godot-core/src/builtin/collections/array.rs | 6 +- godot-core/src/obj/dyn_gd.rs | 55 +++---------------- godot-core/src/obj/gd.rs | 7 +-- godot-core/src/obj/oneditor.rs | 3 + godot-core/src/registry/class.rs | 10 ++-- itest/godot/ManualFfiTests.gd | 6 +- .../src/register_tests/option_ffi_test.rs | 6 +- 7 files changed, 29 insertions(+), 64 deletions(-) diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index c9ccbd8ee..542c28b77 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -1102,9 +1102,9 @@ where impl BuiltinExport for Array {} -impl Export for Array> +impl Export for Array> where - T: Bounds, + T: GodotClass + Bounds, { fn export_hint() -> PropertyHintInfo { PropertyHintInfo::export_array_element::>() @@ -1121,7 +1121,7 @@ where /// Consider exporting `Array>` instead of `Array>` for user-declared GDExtension classes. impl Export for Array> where - T: GodotClass + Bounds, + T: GodotClass + Bounds, D: ?Sized + 'static, { fn export_hint() -> PropertyHintInfo { diff --git a/godot-core/src/obj/dyn_gd.rs b/godot-core/src/obj/dyn_gd.rs index 8a08983e1..a910930fd 100644 --- a/godot-core/src/obj/dyn_gd.rs +++ b/godot-core/src/obj/dyn_gd.rs @@ -142,48 +142,9 @@ use std::{fmt, ops}; /// Exporting `DynGd` is possible only via [`OnEditor`] or [`Option`]. /// `DynGd` can also be exported directly as an element of an array such as `Array>`. /// -/// Since `DynGd` expresses shared functionality `D` among classes inheriting `T`, -/// `#[export]` for `DynGd` where `T` is a concrete Rust class is not allowed. -/// -/// ```compile_fail -/// -/// use godot::prelude::*; -/// -/// trait Health { /* ... */ } -/// -/// #[derive(GodotClass)] -/// # #[class(init)] -/// struct Monster { /* ... */ } -/// -/// #[godot_dyn] -/// impl Health for Monster { /* ... */ } -/// -/// #[derive(GodotClass)] -/// #[class(init, base = Node)] -/// struct MyClass { -/// #[export] -/// dyn_concrete: Option>, -/// } -/// ``` -/// -/// Consider using `Gd` instead in such cases: -/// -/// ``` -/// use godot::prelude::*; -/// -/// #[derive(GodotClass)] -/// #[class(init, base = Node)] -/// struct Monster { /* ... */ } -/// -/// /* ... */ -/// -/// #[derive(GodotClass)] -/// #[class(init, base = Node)] -/// struct MyClass { -/// #[export] -/// dyn_concrete: Option>, -/// } -/// ``` +/// Since `DynGd` represents shared functionality `D` across classes inheriting from `T`, +/// consider using `#[export] Gd` instead of `#[export] DynGd` +/// in cases when `T` is a concrete Rust `GodotClass`. /// /// ## Node based classes /// @@ -562,7 +523,7 @@ where /// Consider exporting `Option>` instead of `Option>` for user-declared GDExtension classes. impl Export for Option> where - T: GodotClass + Bounds, + T: GodotClass + Bounds, D: ?Sized + 'static, { fn export_hint() -> PropertyHintInfo { @@ -599,12 +560,12 @@ where D: ?Sized + 'static, { fn get_property(&self) -> Self::Via { - OnEditor::>::get_property_inner(self) + Self::get_property_inner(self) } fn set_property(&mut self, value: Self::Via) { // `set_property` can't be delegated to Gd, since we have to set `erased_obj` as well. - OnEditor::>::set_property_inner(self, value) + Self::set_property_inner(self, value) } } @@ -613,8 +574,8 @@ where /// Consider exporting `OnEditor>` instead of `OnEditor>` for user-declared GDExtension classes. impl Export for OnEditor> where - OnEditor>: Var, - T: GodotClass + Bounds, + Self: Var, + T: GodotClass + Bounds, D: ?Sized + 'static, { fn export_hint() -> PropertyHintInfo { diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index c0c2ac892..7ea0a488f 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -937,21 +937,20 @@ where impl Var for OnEditor> where T: GodotClass, - OnEditor>: GodotConvert as GodotConvert>::Via>>, { fn get_property(&self) -> Self::Via { - OnEditor::>::get_property_inner(self) + Self::get_property_inner(self) } fn set_property(&mut self, value: Self::Via) { - OnEditor::>::set_property_inner(self, value) + Self::set_property_inner(self, value) } } impl Export for OnEditor> where + Self: Var, T: GodotClass + Bounds, - OnEditor>: Var, { fn export_hint() -> PropertyHintInfo { PropertyHintInfo::export_gd::() diff --git a/godot-core/src/obj/oneditor.rs b/godot-core/src/obj/oneditor.rs index 3149783c4..ca5291d94 100644 --- a/godot-core/src/obj/oneditor.rs +++ b/godot-core/src/obj/oneditor.rs @@ -166,6 +166,9 @@ pub(crate) enum OnEditorState { Initialized(T), } +/// `OnEditor` is usable only for properties – which is enforced via `Var` and `FromGodot` bounds. +/// +/// Furthermore, `PartialEq` is needed to compare against uninitialized sentinel values. impl OnEditor { /// Initializes invalid `OnEditor` with given value. /// diff --git a/godot-core/src/registry/class.rs b/godot-core/src/registry/class.rs index a0b02cc80..8c65bcc7a 100644 --- a/godot-core/src/registry/class.rs +++ b/godot-core/src/registry/class.rs @@ -370,10 +370,12 @@ where // For example – don't include Nodes or Objects while creating hint_string for Resource. let relations_iter = relations.iter().filter_map(|implementor| { // TODO – check if caching it (using is_derived_base_cached) yields any benefits. - if ClassDb::singleton().is_parent_class( - &implementor.parent_class_name?.to_string_name(), - &T::class_name().to_string_name(), - ) { + if implementor.parent_class_name? == T::class_name() + || ClassDb::singleton().is_parent_class( + &implementor.parent_class_name?.to_string_name(), + &T::class_name().to_string_name(), + ) + { Some(implementor) } else { None diff --git a/itest/godot/ManualFfiTests.gd b/itest/godot/ManualFfiTests.gd index cbc384da1..d991da221 100644 --- a/itest/godot/ManualFfiTests.gd +++ b/itest/godot/ManualFfiTests.gd @@ -309,11 +309,12 @@ func test_option_export(): assert_eq(obj.optional_export, null) var test_node := Node.new() + var test_resource := Resource.new() obj.optional = test_node - obj.optional_export = test_node + obj.optional_export = test_resource assert_eq(obj.optional, test_node) - assert_eq(obj.optional_export, test_node) + assert_eq(obj.optional_export, test_resource) obj.optional = null obj.optional_export = null @@ -321,7 +322,6 @@ func test_option_export(): assert_eq(obj.optional_export, null) test_node.free() - obj.free() func test_func_rename(): var func_rename := FuncObj.new() diff --git a/itest/rust/src/register_tests/option_ffi_test.rs b/itest/rust/src/register_tests/option_ffi_test.rs index 879d195de..9b2359ac5 100644 --- a/itest/rust/src/register_tests/option_ffi_test.rs +++ b/itest/rust/src/register_tests/option_ffi_test.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use godot::classes::{Node, Object, RefCounted}; +use godot::classes::{Node, Object, RefCounted, Resource}; use godot::meta::GodotType; use godot::obj::{Gd, NewAlloc, NewGd, RawGd}; use godot::register::{godot_api, GodotClass}; @@ -97,11 +97,11 @@ impl OptionFfiTest { } #[derive(GodotClass)] -#[class(init, base=Node)] +#[class(init)] struct OptionExportFfiTest { #[var] optional: Option>, #[export] - optional_export: Option>, + optional_export: Option>, }