Remove feature convenience

This feature was originally designed as an opt-out from panicking methods, which have a Result/Option-based
overload, such as from_variant (try_from_variant), instance_id (instance_id_or_none), cast (try_cast), etc.

However, it does not really add much in practice besides maintenance overhead. To still warn people to not
accidentally use one of these methods, a warning sign is prepended to the brief doc. This should show up
first in IDEs.

Note that the warning sign is not added to every panicking method, but particularly those which have a "try"
overload. To know which methods panic, check out if it contains a `# Panics` section.

.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:

.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:

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

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"

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" }

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<T: FromVariant>(&self) -> T {
         T::from_variant(self)
     }

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<Self, VariantConversionError>;
 
-    #[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<Variant, VariantConversionError>;
 
@@ -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;
 }
 

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<T>` means that
@@ -49,7 +49,7 @@ pub struct Gd<T: GodotClass> {
     // 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`._ <br><br>
 impl<T> Gd<T>
 where
     T: GodotClass<Declarer = dom::UserDomain>,
 {
-    /// 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::<T>(ptr::null_mut());
-            Obj::from_obj_sys(ptr)
-        };
-
-        result.storage().initialize(user_object);*/
-
         Self::with_base(move |_base| user_object)
     }
 
@@ -91,31 +83,20 @@ where
     where
         T: cap::GodotInit,
     {
-        /*let class_name = ClassName::new::<T>();
-        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::<T>(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<T>` 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.
     /// The `init` function provides you with a `Base<T::Base>` object that you can use inside your `T`, which
     /// is then wrapped in a `Gd<T>`.
     ///
     /// 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<T> {
@@ -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`._ <br><br>
 impl<T: GodotClass> Gd<T> {
     /// 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<Self> {
         // SAFETY: Godot looks up ID in ObjectDB and returns null if not found
@@ -209,16 +190,15 @@ impl<T: GodotClass> Gd<T> {
         }
     }
 
-    /// 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,19 +212,18 @@ impl<T: GodotClass> Gd<T> {
         }
     }
 
-    /// 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<InstanceId> {
         // Note: bit 'id & (1 << 63)' determines if the instance is ref-counted
         let id = unsafe { interface_fn!(object_get_instance_id)(self.obj_sys()) };
         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<T: GodotClass> Gd<T> {
         })
     }
 
-    /// 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<T: GodotClass> Gd<T> {
     ///
     /// Moves out of this value. If you want to create _another_ smart pointer instance,
     /// use this idiom:
-    /// ```ignore
-    /// let obj: Gd<T> = ...;
-    /// let base = obj.share().upcast::<Base>();
+    /// ```no_run
+    /// # use godot::prelude::*;
+    ///
+    /// #[derive(GodotClass)]
+    /// #[class(init, base=Node2D)]
+    /// struct MyClass {}
+    ///
+    /// let obj: Gd<MyClass> = Gd::new_default();
+    /// let base = obj.share().upcast::<Node>();
     /// ```
     pub fn upcast<Base>(self) -> Gd<Base>
     where
@@ -293,19 +278,18 @@ impl<T: GodotClass> Gd<T> {
     /// 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<Gd<Derived>, Self> so that user can still use original obj (e.g. to free if manual)
+    // TODO consider Result<Gd<Derived>, Self> so that user can still use original object (e.g. to free if manual)
     pub fn try_cast<Derived>(self) -> Option<Gd<Derived>>
     where
         Derived: GodotClass + Inherits<T>,
     {
         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<Derived>(self) -> Gd<Derived>
     where
         Derived: GodotClass + Inherits<T>,
@@ -328,7 +312,7 @@ impl<T: GodotClass> Gd<T> {
         // 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::<U>() };
@@ -363,7 +347,7 @@ impl<T: GodotClass> Gd<T> {
         );
 
         let tmp = unsafe { self.ffi_cast::<engine::RefCounted>() };
-        let mut tmp = tmp.expect("obj expected to inherit RefCounted");
+        let mut tmp = tmp.expect("object expected to inherit RefCounted");
         let return_val =
             <engine::RefCounted as GodotClass>::Declarer::scoped_mut(&mut tmp, |obj| apply(obj));
 
@@ -375,7 +359,7 @@ impl<T: GodotClass> Gd<T> {
         // Note: no validity check; this could be called by to_string(), which can be called on dead instances
 
         let tmp = unsafe { self.ffi_cast::<engine::Object>() };
-        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 =
             <engine::Object as GodotClass>::Declarer::scoped_mut(&mut tmp, |obj| apply(obj));
@@ -415,15 +399,15 @@ impl<T: GodotClass> Gd<T> {
 }
 
 /// _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._ <br><br>
 impl<T, M> Gd<T>
 where
     T: GodotClass<Mem = M>,
     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<Object>` 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<T: GodotClass> Drop for Gd<T> {
     fn drop(&mut self) {

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> {
         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")
     }

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"]