Skip to content

Commit 3b0f1b7

Browse files
committed
Prevent Gd::default_instance bypassing placeholder instances.
---- - Forward object creation through ClassDB to handle placeholder instances logic. - Remove blanket implementation of `__godot_default` – requiring repeatedly implementing `__godot_default()` for all user classes.
1 parent ee07cea commit 3b0f1b7

File tree

7 files changed

+75
-55
lines changed

7 files changed

+75
-55
lines changed

godot-core/src/obj/base.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ pub struct Base<T: GodotClass> {
8080
// 2.
8181
obj: ManuallyDrop<Gd<T>>,
8282

83-
/// Tracks the initialization state of this `Base<T>` in Debug mode.
83+
/// Tracks the initialization state of this `Base<T>` if safeguards are enabled.
8484
///
8585
/// Rc allows to "copy-construct" the base from an existing one, while still affecting the user-instance through the original `Base<T>`.
8686
#[cfg(safeguards_balanced)]

godot-core/src/obj/bounds.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ impl Declarer for DeclUser {
420420
where
421421
T: GodotDefault + Bounds<Declarer = Self>,
422422
{
423-
Gd::default_instance()
423+
Gd::default_user_instance()
424424
}
425425
}
426426

godot-core/src/obj/gd.rs

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,14 @@ use crate::meta::{
1919
ToGodot,
2020
};
2121
use crate::obj::{
22-
bounds, cap, Bounds, DynGd, GdDerefTarget, GdMut, GdRef, GodotClass, Inherits, InstanceId,
23-
OnEditor, RawGd, WithBaseField, WithSignals,
22+
bounds, cap, Base, Bounds, DynGd, GdDerefTarget, GdMut, GdRef, GodotClass, Inherits,
23+
InstanceId, OnEditor, RawGd, WithBaseField, WithSignals,
2424
};
25+
use crate::private::callbacks::postinit;
2526
use crate::private::{callbacks, PanicPayload};
2627
use crate::registry::class::try_dynify_object;
2728
use crate::registry::property::{object_export_element_type_string, Export, Var};
29+
use crate::storage::Storage;
2830
use crate::{classes, meta, out};
2931

3032
/// Smart pointer to objects owned by the Godot engine.
@@ -204,6 +206,39 @@ where
204206
pub fn bind_mut(&mut self) -> GdMut<'_, T> {
205207
self.raw.bind_mut()
206208
}
209+
210+
/// Creates a new initialized instance.
211+
///
212+
/// Creation goes through ClassDb, which handles creation of placeholders and other specific logic.
213+
///
214+
/// Since Godot 4.4 the caller (in this case godot-rust) is responsible for emitting the `POSTINITIALIZE`
215+
/// notification, which should be handled by a not fully-initialized object. This is done so to maintain the consistency –
216+
/// the notification must be handled the same way regardless of how and where the instance was created (in other words,
217+
/// `T::new_gd()` and `T::default()` must be equivalent to the GDScript expression `T.new()`).
218+
///
219+
/// Before Godot 4.4, Godot itself was responsible for emitting the `POSTINITIALIZE` notification, so we mark the instance as initialized
220+
/// during creation.
221+
///
222+
/// See also: [`Base`].
223+
#[doc(hidden)]
224+
pub fn default_user_instance() -> Gd<T>
225+
where
226+
T: cap::GodotDefault,
227+
{
228+
// SAFETY: All safety requirements of classdb_construct_object/from_obj_sys are being held.
229+
let obj = unsafe {
230+
let object_ptr = sys::classdb_construct_object(T::class_id().string_sys());
231+
postinit(object_ptr);
232+
Gd::<T>::from_obj_sys(object_ptr)
233+
};
234+
235+
// Mark base as initialized after postinit.
236+
#[cfg(since_api = "4.4")]
237+
if let Some(storage) = obj.raw.storage() {
238+
unsafe { Base::from_base(storage.base()).mark_initialized() };
239+
}
240+
obj
241+
}
207242
}
208243

209244
/// _The methods in this impl block are available for any `T`._ <br><br>
@@ -524,24 +559,6 @@ impl<T: GodotClass> Gd<T> {
524559
.map_err(Self::from_ffi)
525560
}
526561

527-
/// Create default instance for all types that have `GodotDefault`.
528-
///
529-
/// Deliberately more loose than `Gd::default()`, does not require ref-counted memory strategy for user types.
530-
pub(crate) fn default_instance() -> Self
531-
where
532-
T: cap::GodotDefault,
533-
{
534-
unsafe {
535-
// Default value (and compat one) for `p_notify_postinitialize` is true in Godot.
536-
#[cfg(since_api = "4.4")]
537-
let object_ptr = callbacks::create::<T>(std::ptr::null_mut(), sys::conv::SYS_TRUE);
538-
#[cfg(before_api = "4.4")]
539-
let object_ptr = callbacks::create::<T>(std::ptr::null_mut());
540-
541-
Gd::from_obj_sys(object_ptr)
542-
}
543-
}
544-
545562
/// Upgrades to a `DynGd<T, D>` pointer, enabling the `D` abstraction.
546563
///
547564
/// The `D` parameter can typically be inferred when there is a single `AsDyn<...>` implementation for `T`. \

godot-core/src/obj/traits.rs

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -773,7 +773,7 @@ pub mod cap {
773773
use super::*;
774774
use crate::builtin::{StringName, Variant};
775775
use crate::meta::PropertyInfo;
776-
use crate::obj::{Base, Bounds, Gd};
776+
use crate::obj::{Base, Gd};
777777
use crate::storage::{IntoVirtualMethodReceiver, VirtualMethodReceiver};
778778

779779
/// Trait for all classes that are default-constructible from the Godot engine.
@@ -804,19 +804,7 @@ pub mod cap {
804804
/// - For user-defined classes, this calls `T::init()` or the generated init-constructor.
805805
/// - For engine classes, this calls `T::new()`.
806806
#[doc(hidden)]
807-
fn __godot_default() -> Gd<Self> {
808-
// This is a bit hackish, but the alternatives are:
809-
// 1. Separate trait `GodotUserDefault` for user classes, which then proliferates through all APIs and makes abstraction harder.
810-
// 2. Repeatedly implementing __godot_default() that forwards to something like Gd::default_user_instance(). Possible, but this
811-
// will make the step toward builder APIs more difficult, as users would need to re-implement this as well.
812-
sys::strict_assert_eq!(
813-
std::any::TypeId::of::<<Self as Bounds>::Declarer>(),
814-
std::any::TypeId::of::<bounds::DeclUser>(),
815-
"__godot_default() called on engine class; must be overridden for engine classes"
816-
);
817-
818-
Gd::default_instance()
819-
}
807+
fn __godot_default() -> Gd<Self>;
820808

821809
/// Only provided for user classes.
822810
#[doc(hidden)]

godot-core/src/registry/callbacks.rs

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use std::any::Any;
1414

1515
use godot_ffi as sys;
16+
use godot_ffi::GDExtensionObjectPtr;
1617
use sys::conv::u32_to_usize;
1718
use sys::interface_fn;
1819

@@ -75,7 +76,7 @@ pub unsafe extern "C" fn recreate<T: cap::GodotDefault>(
7576
_class_userdata: *mut std::ffi::c_void,
7677
object: sys::GDExtensionObjectPtr,
7778
) -> sys::GDExtensionClassInstancePtr {
78-
create_rust_part_for_existing_godot_part(T::__godot_user_init, object, |_| {})
79+
create_rust_part_for_existing_godot_part(T::__godot_user_init, object, true)
7980
.unwrap_or(std::ptr::null_mut())
8081
}
8182

@@ -90,6 +91,18 @@ pub unsafe extern "C" fn recreate_null<T>(
9091
std::ptr::null_mut()
9192
}
9293

94+
#[cfg(since_api = "4.4")]
95+
pub(crate) fn postinit(base_ptr: GDExtensionObjectPtr) {
96+
// Should notify it with a weak pointer, during `NOTIFICATION_POSTINITIALIZE`, ref-counted object is not yet fully-initialized.
97+
// SAFETY: `Gd::from_obj_sys_weak` will raise panic if base_ptr is null or invalid.
98+
let mut obj = unsafe { Gd::<Object>::from_obj_sys_weak(base_ptr) };
99+
obj.notify(crate::classes::notify::ObjectNotification::POSTINITIALIZE);
100+
obj.drop_weak();
101+
}
102+
103+
#[cfg(before_api = "4.4")]
104+
pub(crate) fn postinit(_base_ptr: GDExtensionObjectPtr) {}
105+
93106
pub(crate) fn create_custom<T, F>(
94107
make_user_instance: F,
95108
notify_postinitialize: bool,
@@ -101,17 +114,11 @@ where
101114
let base_class_name = T::Base::class_id();
102115
let base_ptr = unsafe { sys::classdb_construct_object(base_class_name.string_sys()) };
103116

104-
let postinit = |base_ptr| {
105-
#[cfg(since_api = "4.4")]
106-
if notify_postinitialize {
107-
// Should notify it with a weak pointer, during `NOTIFICATION_POSTINITIALIZE`, ref-counted object is not yet fully-initialized.
108-
let mut obj = unsafe { Gd::<Object>::from_obj_sys_weak(base_ptr) };
109-
obj.notify(crate::classes::notify::ObjectNotification::POSTINITIALIZE);
110-
obj.drop_weak();
111-
}
112-
};
113-
114-
match create_rust_part_for_existing_godot_part(make_user_instance, base_ptr, postinit) {
117+
match create_rust_part_for_existing_godot_part(
118+
make_user_instance,
119+
base_ptr,
120+
notify_postinitialize,
121+
) {
115122
Ok(_extension_ptr) => Ok(base_ptr),
116123
Err(payload) => {
117124
// Creation of extension object failed; we must now also destroy the base object to avoid leak.
@@ -130,15 +137,14 @@ where
130137
/// With godot-rust, custom objects consist of two parts: the Godot object and the Rust object. This method takes the Godot part by pointer,
131138
/// creates the Rust part with the supplied state, and links them together. This is used for both brand-new object creation and hot reload.
132139
/// During hot reload, Rust objects are disposed of and then created again with updated code, so it's necessary to re-link them to Godot objects.
133-
fn create_rust_part_for_existing_godot_part<T, F, P>(
140+
fn create_rust_part_for_existing_godot_part<T, F>(
134141
make_user_instance: F,
135142
base_ptr: sys::GDExtensionObjectPtr,
136-
postinit: P,
143+
notify_postinitialize: bool,
137144
) -> Result<sys::GDExtensionClassInstancePtr, PanicPayload>
138145
where
139146
T: GodotClass,
140147
F: FnOnce(Base<T::Base>) -> T,
141-
P: Fn(sys::GDExtensionObjectPtr),
142148
{
143149
let class_name = T::class_id();
144150
//out!("create callback: {}", class_name.backing);
@@ -170,10 +176,11 @@ where
170176
);
171177
}
172178

173-
postinit(base_ptr);
174-
175-
// Mark initialization as complete, now that user constructor has finished.
176-
base_copy.mark_initialized();
179+
if notify_postinitialize {
180+
postinit(base_ptr);
181+
// Mark initialization as complete, now that user constructor has finished.
182+
base_copy.mark_initialized();
183+
}
177184

178185
// No std::mem::forget(base_copy) here, since Base may stores other fields that need deallocation.
179186
Ok(instance_ptr)

godot-macros/src/class/data_models/interface_trait_impl.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,10 @@ fn handle_init<'a>(
289289

290290
#(#cfg_attrs)*
291291
impl ::godot::obj::cap::GodotDefault for #class_name {
292+
fn __godot_default() -> ::godot::obj::Gd<Self> {
293+
::godot::obj::Gd::default_user_instance()
294+
}
295+
292296
fn __godot_user_init(base: ::godot::obj::Base<Self::Base>) -> Self {
293297
<Self as #trait_path>::init(base)
294298
}

godot-macros/src/class/derive_godot_class.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,10 @@ fn make_godot_init_impl(class_name: &Ident, fields: &Fields) -> TokenStream {
362362

363363
quote! {
364364
impl ::godot::obj::cap::GodotDefault for #class_name {
365+
fn __godot_default() -> ::godot::obj::Gd<Self> {
366+
::godot::obj::Gd::default_user_instance()
367+
}
368+
365369
fn __godot_user_init(base: ::godot::obj::Base<<#class_name as ::godot::obj::GodotClass>::Base>) -> Self {
366370
Self {
367371
#( #rest_init )*

0 commit comments

Comments
 (0)