diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 6650769fe..2952a691c 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -14,7 +14,8 @@ on: - trying env: - GDEXT_FEATURES: '--features godot-core/convenience' + GDEXT_FEATURES: '' +# GDEXT_FEATURES: '--features crate/feature' # GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot' defaults: diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml index 4646248ce..4668382c7 100644 --- a/.github/workflows/minimal-ci.yml +++ b/.github/workflows/minimal-ci.yml @@ -14,7 +14,8 @@ on: - master env: - GDEXT_FEATURES: '--features godot-core/convenience' + GDEXT_FEATURES: '' +# GDEXT_FEATURES: '--features crate/feature' # GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot' defaults: diff --git a/check.sh b/check.sh index e7aed976b..fbf48981a 100755 --- a/check.sh +++ b/check.sh @@ -67,7 +67,8 @@ function findGodot() { fi } -features="--features godot-core/convenience" +#features="--features crate/feature" +features="" cmds=() for arg in "${args[@]}"; do diff --git a/examples/dodge-the-creeps/rust/Cargo.toml b/examples/dodge-the-creeps/rust/Cargo.toml index 4797baef7..f85abad06 100644 --- a/examples/dodge-the-creeps/rust/Cargo.toml +++ b/examples/dodge-the-creeps/rust/Cargo.toml @@ -8,5 +8,5 @@ publish = false crate-type = ["cdylib"] [dependencies] -godot = { path = "../../../godot", default-features = false, features = ["formatted", "convenience"] } +godot = { path = "../../../godot", default-features = false, features = ["formatted"] } rand = "0.8" diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index 056011b5d..0642f69da 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -12,7 +12,6 @@ default = [] trace = [] codegen-fmt = ["godot-ffi/codegen-fmt"] codegen-full = ["godot-codegen/codegen-full"] -convenience = [] [dependencies] godot-ffi = { path = "../godot-ffi" } diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 8207a4fc4..90ece46ca 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -40,13 +40,12 @@ impl Variant { value.to_variant() } - /// Convert to type `T`, panicking on failure. + /// ⚠️ Convert to type `T`, panicking on failure. /// /// Equivalent to `T::from_variant(&self)`. /// /// # Panics /// When this variant holds a different type. - #[cfg(feature = "convenience")] pub fn to(&self) -> T { T::from_variant(self) } diff --git a/godot-core/src/builtin/variant/variant_traits.rs b/godot-core/src/builtin/variant/variant_traits.rs index bb2f0d53e..9119727ed 100644 --- a/godot-core/src/builtin/variant/variant_traits.rs +++ b/godot-core/src/builtin/variant/variant_traits.rs @@ -6,10 +6,15 @@ use crate::builtin::Variant; +/// Trait to enable conversions of types _from_ the [`Variant`] type. pub trait FromVariant: Sized { + /// Tries to convert a `Variant` to `Self`, allowing to check the success or failure. fn try_from_variant(variant: &Variant) -> Result; - #[cfg(feature = "convenience")] + /// ⚠️ Converts from `Variant` to `Self`, panicking on error. + /// + /// This method should generally not be overridden by trait impls, even if conversions are infallible. + /// Implementing [`Self::try_from_variant`] suffices. fn from_variant(variant: &Variant) -> Self { Self::try_from_variant(variant).unwrap_or_else(|e| { panic!( @@ -22,6 +27,7 @@ pub trait FromVariant: Sized { } } +/// Trait to enable conversions of types _to_ the [`Variant`] type. pub trait ToVariant { /*fn try_to_variant(&self) -> Result; @@ -35,6 +41,9 @@ pub trait ToVariant { }) }*/ + /// Infallible conversion from `Self` type to `Variant`. + /// + /// This method must not panic. If your conversion is fallible, this trait should not be used. fn to_variant(&self) -> Variant; } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 700396cf0..70312cbe5 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -33,7 +33,7 @@ use crate::{callbacks, engine, out}; /// /// * Objects of type [`RefCounted`] or inherited from it are **reference-counted**. This means that every time a smart pointer is /// shared using [`Share::share()`], the reference counter is incremented, and every time one is dropped, it is decremented. -/// This ensures that the last reference (either in Rust or Godot) will deallocate the obj and call `T`'s destructor. +/// This ensures that the last reference (either in Rust or Godot) will deallocate the object and call `T`'s destructor. /// /// * Objects inheriting from [`Object`] which are not `RefCounted` (or inherited) are **manually-managed**. /// Their destructor is not automatically called (unless they are part of the scene tree). Creating a `Gd` means that @@ -49,7 +49,7 @@ pub struct Gd { // Note: `opaque` has the same layout as GDExtensionObjectPtr == Object* in C++, i.e. the bytes represent a pointer // To receive a GDExtensionTypePtr == GDExtensionObjectPtr* == Object**, we need to get the address of this // Hence separate sys() for GDExtensionTypePtr, and obj_sys() for GDExtensionObjectPtr. - // The former is the standard FFI type, while the latter is used in obj-specific GDExtension engines. + // The former is the standard FFI type, while the latter is used in object-specific GDExtension engines. // pub(crate) because accessed in obj::dom pub(crate) opaque: OpaqueObject, _marker: PhantomData<*const T>, @@ -63,24 +63,16 @@ static_assert_eq_size!( ); /// _The methods in this impl block are only available for user-declared `T`, that is, -/// structs with `#[derive(GodotClass)]` but not Godot classes like `Node` or `RefCounted`._ +/// structs with `#[derive(GodotClass)]` but not Godot classes like `Node` or `RefCounted`._

impl Gd where T: GodotClass, { - /// Moves a user-created obj into this smart pointer, submitting ownership to the Godot engine. + /// Moves a user-created object into this smart pointer, submitting ownership to the Godot engine. /// /// This is only useful for types `T` which do not store their base objects (if they have a base, /// you cannot construct them standalone). pub fn new(user_object: T) -> Self { - /*let result = unsafe { - //let ptr = interface_fn!(classdb_construct_object)(class_name.c_str()); - let ptr = callbacks::create::(ptr::null_mut()); - Obj::from_obj_sys(ptr) - }; - - result.storage().initialize(user_object);*/ - Self::with_base(move |_base| user_object) } @@ -91,23 +83,12 @@ where where T: cap::GodotInit, { - /*let class_name = ClassName::new::(); - let result = unsafe { - let ptr = interface_fn!(classdb_construct_object)(class_name.c_str()); - Obj::from_obj_sys(ptr) - }; - - result.storage().initialize_default(); - T::Mem::maybe_init_ref(&result); - result*/ - unsafe { let object_ptr = callbacks::create::(ptr::null_mut()); Gd::from_obj_sys(object_ptr) } } - // FIXME use ```no_run instead of ```ignore, as soon as unit test #[cfg] mess is cleaned up /// Creates a `Gd` using a function that constructs a `T` from a provided base. /// /// Imagine you have a type `T`, which has a `#[base]` field that you cannot default-initialize. @@ -115,7 +96,7 @@ where /// is then wrapped in a `Gd`. /// /// Example: - /// ```ignore + /// ```no_run /// # use godot::prelude::*; /// #[derive(GodotClass)] /// #[class(init, base=Node2D)] @@ -169,7 +150,7 @@ where GdMut::from_cell(self.storage().get_mut()) } - /// Storage obj associated with the extension instance + /// Storage object associated with the extension instance // FIXME proper + safe interior mutability, also that Clippy is happy #[allow(clippy::mut_from_ref)] pub(crate) fn storage(&self) -> &mut InstanceStorage { @@ -190,11 +171,11 @@ where } } -/// _The methods in this impl block are available for any `T`._ +/// _The methods in this impl block are available for any `T`._

impl Gd { /// Looks up the given instance ID and returns the associated obj, if possible. /// - /// If no such instance ID is registered, or if the dynamic type of the obj behind that instance ID + /// If no such instance ID is registered, or if the dynamic type of the object behind that instance ID /// is not compatible with `T`, then `None` is returned. pub fn try_from_instance_id(instance_id: InstanceId) -> Option { // SAFETY: Godot looks up ID in ObjectDB and returns null if not found @@ -209,16 +190,15 @@ impl Gd { } } - /// Looks up the given instance ID and returns the associated obj. + /// ⚠️ Looks up the given instance ID and returns the associated object. /// /// # Panics - /// If no such instance ID is registered, or if the dynamic type of the obj behind that instance ID + /// If no such instance ID is registered, or if the dynamic type of the object behind that instance ID /// is not compatible with `T`. - #[cfg(feature = "convenience")] pub fn from_instance_id(instance_id: InstanceId) -> Self { Self::try_from_instance_id(instance_id).unwrap_or_else(|| { panic!( - "Instance ID {} does not belong to a valid obj of class '{}'", + "Instance ID {} does not belong to a valid object of class '{}'", instance_id, T::CLASS_NAME ) @@ -232,7 +212,7 @@ impl Gd { } } - /// Returns the instance ID of this obj, or `None` if the obj is dead. + /// Returns the instance ID of this obj, or `None` if the object is dead. /// pub fn instance_id_or_none(&self) -> Option { // Note: bit 'id & (1 << 63)' determines if the instance is ref-counted @@ -240,11 +220,10 @@ impl Gd { InstanceId::try_from_u64(id) } - /// Returns the instance ID of this obj (panics when dead). + /// ⚠️ Returns the instance ID of this object (panics when dead). /// /// # Panics - /// If this obj is no longer alive (registered in Godot's obj database). - #[cfg(feature = "convenience")] + /// If this object is no longer alive (registered in Godot's object database). pub fn instance_id(&self) -> InstanceId { self.instance_id_or_none().unwrap_or_else(|| { panic!( @@ -254,12 +233,12 @@ impl Gd { }) } - /// Checks if this smart pointer points to a live obj (read description!). + /// Checks if this smart pointer points to a live object (read description!). /// - /// Using this method is often indicative of bad design -- you should dispose of your pointers once an obj is + /// Using this method is often indicative of bad design -- you should dispose of your pointers once an object is /// destroyed. However, this method exists because GDScript offers it and there may be **rare** use cases. /// - /// Do not use this method to check if you can safely access an obj. Accessing dead objects is generally safe + /// Do not use this method to check if you can safely access an object. Accessing dead objects is generally safe /// and will panic in a defined manner. Encountering such panics is almost always a bug you should fix, and not a /// runtime condition to check against. pub fn is_instance_valid(&self) -> bool { @@ -275,9 +254,15 @@ impl Gd { /// /// Moves out of this value. If you want to create _another_ smart pointer instance, /// use this idiom: - /// ```ignore - /// let obj: Gd = ...; - /// let base = obj.share().upcast::(); + /// ```no_run + /// # use godot::prelude::*; + /// + /// #[derive(GodotClass)] + /// #[class(init, base=Node2D)] + /// struct MyClass {} + /// + /// let obj: Gd = Gd::new_default(); + /// let base = obj.share().upcast::(); /// ``` pub fn upcast(self) -> Gd where @@ -293,7 +278,7 @@ impl Gd { /// If `T`'s dynamic type is not `Derived` or one of its subclasses, `None` is returned /// and the reference is dropped. Otherwise, `Some` is returned and the ownership is moved /// to the returned value. - // TODO consider Result, Self> so that user can still use original obj (e.g. to free if manual) + // TODO consider Result, Self> so that user can still use original object (e.g. to free if manual) pub fn try_cast(self) -> Option> where Derived: GodotClass + Inherits, @@ -301,11 +286,10 @@ impl Gd { self.owned_cast().ok() } - /// **Downcast:** convert into a smart pointer to a derived class. Panics on error. + /// ⚠️ **Downcast:** convert into a smart pointer to a derived class. Panics on error. /// /// # Panics /// If the class' dynamic type is not `Derived` or one of its subclasses. Use [`Self::try_cast()`] if you want to check the result. - #[cfg(feature = "convenience")] pub fn cast(self) -> Gd where Derived: GodotClass + Inherits, @@ -328,7 +312,7 @@ impl Gd { // to return the same pointer, however in theory those may yield a different pointer (VTable offset, // virtual inheritance etc.). It *seems* to work so far, but this is no indication it's not UB. // - // The Deref/DerefMut impls for T implement an "implicit upcast" on the obj (not Gd) level and + // The Deref/DerefMut impls for T implement an "implicit upcast" on the object (not Gd) level and // rely on this (e.g. &Node3D -> &Node). let result = unsafe { self.ffi_cast::() }; @@ -363,7 +347,7 @@ impl Gd { ); let tmp = unsafe { self.ffi_cast::() }; - let mut tmp = tmp.expect("obj expected to inherit RefCounted"); + let mut tmp = tmp.expect("object expected to inherit RefCounted"); let return_val = ::Declarer::scoped_mut(&mut tmp, |obj| apply(obj)); @@ -375,7 +359,7 @@ impl Gd { // Note: no validity check; this could be called by to_string(), which can be called on dead instances let tmp = unsafe { self.ffi_cast::() }; - let mut tmp = tmp.expect("obj expected to inherit Object; should never fail"); + let mut tmp = tmp.expect("object expected to inherit Object; should never fail"); // let return_val = apply(tmp.inner_mut()); let return_val = ::Declarer::scoped_mut(&mut tmp, |obj| apply(obj)); @@ -415,15 +399,15 @@ impl Gd { } /// _The methods in this impl block are only available for objects `T` that are manually managed, -/// i.e. anything that is not `RefCounted` or inherited from it._ +/// i.e. anything that is not `RefCounted` or inherited from it._

impl Gd where T: GodotClass, M: mem::PossiblyManual + mem::Memory, { - /// Destroy the manually-managed Godot obj. + /// Destroy the manually-managed Godot object. /// - /// Consumes this smart pointer and renders all other `Gd` smart pointers (as well as any GDScript references) to the same obj + /// Consumes this smart pointer and renders all other `Gd` smart pointers (as well as any GDScript references) to the same object /// immediately invalid. Using those `Gd` instances will lead to panics, but not undefined behavior. /// /// This operation is **safe** and effectively prevents double-free. @@ -432,7 +416,7 @@ where /// example to the node tree in case of nodes. /// /// # Panics - /// * When the referred-to obj has already been destroyed. + /// * When the referred-to object has already been destroyed. /// * When this is invoked on an upcast `Gd` that dynamically points to a reference-counted type (i.e. operation not supported). pub fn free(self) { // TODO disallow for singletons, either only at runtime or both at compile time (new memory policy) and runtime @@ -490,7 +474,7 @@ where // // The resulting &mut T is transmuted from &mut OpaqueObject, i.e. a *pointer* to the `opaque` field. // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous - // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot obj is the + // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the // same (i.e. `opaque` has the same value, but not address). unsafe { std::mem::transmute::<&mut OpaqueObject, &mut T>(&mut self.opaque) } } @@ -540,7 +524,7 @@ pub unsafe fn raw_object_init( /// * If this `Gd` smart pointer holds a reference-counted type, this will decrement the reference counter. /// If this was the last remaining reference, dropping it will invoke `T`'s destructor. /// -/// * If the held obj is manually-managed, **nothing happens**. +/// * If the held object is manually-managed, **nothing happens**. /// To destroy manually-managed `Gd` pointers, you need to call [`Self::free()`]. impl Drop for Gd { fn drop(&mut self) { diff --git a/godot-core/src/obj/instance_id.rs b/godot-core/src/obj/instance_id.rs index 20832a9e0..e7daa2c49 100644 --- a/godot-core/src/obj/instance_id.rs +++ b/godot-core/src/obj/instance_id.rs @@ -26,17 +26,18 @@ pub struct InstanceId { impl InstanceId { /// Constructs an instance ID from an integer, or `None` if the integer is zero. + /// + /// This does *not* check if the instance is valid. pub fn try_from_i64(id: i64) -> Option { Self::try_from_u64(id as u64) } - /// Constructs an instance ID from a non-zero integer, or panics. + /// ⚠️ Constructs an instance ID from a non-zero integer, or panics. /// /// This does *not* check if the instance is valid. /// /// # Panics /// If `id` is zero. - #[cfg(feature = "convenience")] pub fn from_nonzero(id: i64) -> Self { Self::try_from_i64(id).expect("expected non-zero instance ID") } diff --git a/godot/Cargo.toml b/godot/Cargo.toml index eebda8353..bf85f1147 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -8,8 +8,7 @@ keywords = ["gamedev", "godot", "engine", "2d", "3d"] # possibly: "ffi" categories = ["game-engines", "graphics"] [features] -default = ["convenience", "codegen-full"] -convenience = ["godot-core/convenience"] +default = ["codegen-full"] formatted = ["godot-core/codegen-fmt"] trace = ["godot-core/trace"] diff --git a/godot/src/lib.rs b/godot/src/lib.rs index 16207cf68..be4555353 100644 --- a/godot/src/lib.rs +++ b/godot/src/lib.rs @@ -57,6 +57,34 @@ //! you must either hand over ownership to Godot (e.g. by adding a node to the scene tree) or //! free them manually using [`Gd::free()`][crate::obj::Gd::free].

//! +//! # Ergonomics and panics +//! +//! The GDExtension Rust bindings are designed with usage ergonomics in mind, making them viable +//! for fast prototyping. Part of this design means that users should not constantly be forced +//! to write code such as `obj.cast::().unwrap()`. Instead, they can just write `obj.cast::()`, +//! which may panic at runtime. +//! +//! This approach has several advantages: +//! * The code is more concise and less cluttered. +//! * Methods like `cast()` provide very sophisticated panic messages when they fail (e.g. involved +//! classes), immediately giving you the necessary context for debugging. This is certainly +//! preferable over a generic `unwrap()`, and in most cases also over a `expect("literal")`. +//! * Usually, such methods panicking indicate bugs in the application. For example, you have a static +//! scene tree, and you _know_ that a node of certain type and name exists. `get_node_as::("name")` +//! thus _must_ succeed, or your mental concept is wrong. In other words, there is not much you can +//! do at runtime to recover from such errors anyway; the code needs to be fixed. +//! +//! Now, there are of course cases where you _do_ want to check certain assumptions dynamically. +//! Imagine a scene tree that is constructed at runtime, e.g. in a game editor. +//! This is why the library provides "overloads" for most of these methods that return `Option` or `Result`. +//! Such methods have more verbose names and highlight the attempt, e.g. `try_cast()`. +//! +//! To help you identify panicking methods, we use the symbol "⚠️" at the beginning of the documentation; +//! this should also appear immediately in the auto-completion of your IDE. Note that this warning sign is +//! not used as a general panic indicator, but particularly for methods which have a `Option`/`Result`-based +//! overload. If you want to know whether and how a method can panic, check if its documentation has a +//! _Panics_ section. +//! //! # Thread safety //! //! [Godot's own thread safety diff --git a/itest/rust/Cargo.toml b/itest/rust/Cargo.toml index f22fdfdb5..0e610b266 100644 --- a/itest/rust/Cargo.toml +++ b/itest/rust/Cargo.toml @@ -13,7 +13,7 @@ default = [] trace = ["godot/trace"] [dependencies] -godot = { path = "../../godot", default-features = false, features = ["formatted", "convenience"] } +godot = { path = "../../godot", default-features = false, features = ["formatted"] } [build-dependencies] quote = "1"