Skip to content

Commit 221c755

Browse files
committed
Add Base::to_init_gd() to hand out early pointers
For manually-managed objects, the implementation is simple: just create a copy of the internal Gd pointer. For ref-counted objects, we need to deal with the problem that `Base<T>` stores a weak Gd pointer, and thus dropping handed-out pointers will trigger object destruction. To deal with this, we keep an extra strong pointer around, and defer its destruction to the next frame. It would be even better if we could immediately destroy the strong ref after object construction, however we could only do so for Rust paths `new_gd()`, `Gd::from_init_fn()`, not for Godot-side construction. Unfortunately, Godot doesn't offer a hook to run code immediately after creation. The POSTINITIALIZE notification needs to be dispatched by godot-rust, and thus suffers from the same issue.
1 parent 870c725 commit 221c755

File tree

15 files changed

+507
-40
lines changed

15 files changed

+507
-40
lines changed

godot-core/src/obj/base.rs

Lines changed: 177 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,45 @@
55
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
66
*/
77

8+
#[cfg(debug_assertions)]
9+
use std::cell::Cell;
10+
use std::cell::RefCell;
811
use std::fmt::{Debug, Display, Formatter, Result as FmtResult};
912
use std::mem::ManuallyDrop;
13+
use std::rc::Rc;
1014

11-
use crate::obj::{Gd, GodotClass};
15+
use crate::builtin::{Callable, Variant};
16+
use crate::obj::{bounds, Gd, GodotClass};
1217
use crate::{classes, sys};
1318

19+
/// Represents the initialization state of a `Base<T>` object.
20+
#[cfg(debug_assertions)]
21+
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
22+
enum InitState {
23+
/// Object is being constructed (inside `I*::init()` or `Gd::from_init_fn()`).
24+
ObjectConstructing,
25+
/// Object construction is complete.
26+
ObjectInitialized,
27+
/// `ScriptInstance` context - always considered initialized (bypasses lifecycle checks).
28+
Script,
29+
}
30+
31+
#[cfg(debug_assertions)]
32+
macro_rules! base_from_obj {
33+
($obj:expr, $state:expr) => {
34+
Base::from_obj($obj, $state)
35+
};
36+
}
37+
38+
#[cfg(not(debug_assertions))]
39+
macro_rules! base_from_obj {
40+
($obj:expr, $state:expr) => {
41+
Base::from_obj($obj)
42+
};
43+
}
44+
45+
// ----------------------------------------------------------------------------------------------------------------------------------------------
46+
1447
/// Restricted version of `Gd`, to hold the base instance inside a user's `GodotClass`.
1548
///
1649
/// Behaves similarly to [`Gd`][crate::obj::Gd], but is more constrained. Cannot be constructed by the user.
@@ -35,6 +68,15 @@ pub struct Base<T: GodotClass> {
3568
// 1. Gd<T> -- triggers InstanceStorage destruction
3669
// 2.
3770
obj: ManuallyDrop<Gd<T>>,
71+
72+
/// Additional strong ref, needed to prevent destruction if [`Self::to_init_gd()`] is called on ref-counted objects.
73+
extra_strong_ref: Rc<RefCell<Option<Gd<T>>>>,
74+
75+
/// Tracks the initialization state of this `Base<T>` in Debug mode.
76+
///
77+
/// Rc allows to "copy-construct" the base from an existing one, while still affecting the user-instance through the original `Base<T>`.
78+
#[cfg(debug_assertions)]
79+
init_state: Rc<Cell<InitState>>,
3880
}
3981

4082
impl<T: GodotClass> Base<T> {
@@ -47,7 +89,15 @@ impl<T: GodotClass> Base<T> {
4789
/// If `base` is destroyed while the returned `Base<T>` is in use, that constitutes a logic error, not a safety issue.
4890
pub(crate) unsafe fn from_base(base: &Base<T>) -> Base<T> {
4991
debug_assert!(base.obj.is_instance_valid());
50-
Base::from_obj(Gd::from_obj_sys_weak(base.obj.obj_sys()))
92+
93+
let obj = Gd::from_obj_sys_weak(base.obj.obj_sys());
94+
95+
Self {
96+
obj: ManuallyDrop::new(obj),
97+
extra_strong_ref: Rc::clone(&base.extra_strong_ref), // Before user init(), no handing out of Gd pointers occurs.
98+
#[cfg(debug_assertions)]
99+
init_state: Rc::clone(&base.init_state),
100+
}
51101
}
52102

53103
/// Create base from existing object (used in script instances).
@@ -57,9 +107,11 @@ impl<T: GodotClass> Base<T> {
57107
/// # Safety
58108
/// `gd` must be alive at the time of invocation. If it is destroyed while the returned `Base<T>` is in use, that constitutes a logic
59109
/// error, not a safety issue.
60-
pub(crate) unsafe fn from_gd(gd: &Gd<T>) -> Self {
110+
pub(crate) unsafe fn from_script_gd(gd: &Gd<T>) -> Self {
61111
debug_assert!(gd.is_instance_valid());
62-
Base::from_obj(Gd::from_obj_sys_weak(gd.obj_sys()))
112+
113+
let obj = Gd::from_obj_sys_weak(gd.obj_sys());
114+
base_from_obj!(obj, InitState::Script)
63115
}
64116

65117
/// Create new base from raw Godot object.
@@ -72,20 +124,31 @@ impl<T: GodotClass> Base<T> {
72124
pub(crate) unsafe fn from_sys(base_ptr: sys::GDExtensionObjectPtr) -> Self {
73125
assert!(!base_ptr.is_null(), "instance base is null pointer");
74126

75-
// Initialize only as weak pointer (don't increment reference count)
127+
// Initialize only as weak pointer (don't increment reference count).
76128
let obj = Gd::from_obj_sys_weak(base_ptr);
77129

78130
// This obj does not contribute to the strong count, otherwise we create a reference cycle:
79131
// 1. RefCounted (dropped in GDScript)
80132
// 2. holds user T (via extension instance and storage)
81133
// 3. holds Base<T> RefCounted (last ref, dropped in T destructor, but T is never destroyed because this ref keeps storage alive)
82134
// Note that if late-init never happened on self, we have the same behavior (still a raw pointer instead of weak Gd)
83-
Base::from_obj(obj)
135+
base_from_obj!(obj, InitState::ObjectConstructing)
84136
}
85137

138+
#[cfg(debug_assertions)]
139+
fn from_obj(obj: Gd<T>, init_state: InitState) -> Self {
140+
Self {
141+
obj: ManuallyDrop::new(obj),
142+
extra_strong_ref: Rc::new(RefCell::new(None)),
143+
init_state: Rc::new(Cell::new(init_state)),
144+
}
145+
}
146+
147+
#[cfg(not(debug_assertions))]
86148
fn from_obj(obj: Gd<T>) -> Self {
87149
Self {
88150
obj: ManuallyDrop::new(obj),
151+
extra_strong_ref: Rc::new(RefCell::new(None)),
89152
}
90153
}
91154

@@ -94,10 +157,88 @@ impl<T: GodotClass> Base<T> {
94157
/// Using this method to call methods on the base field of a Rust object is discouraged, instead use the
95158
/// methods from [`WithBaseField`](super::WithBaseField) when possible.
96159
#[doc(hidden)]
160+
#[deprecated = "Private API. Use `Base::to_init_gd()` or `WithBaseField::to_gd()` instead."] // TODO(v0.4): remove.
97161
pub fn to_gd(&self) -> Gd<T> {
98162
(*self.obj).clone()
99163
}
100164

165+
#[cfg(since_api = "4.2")]
166+
pub fn to_init_gd(&self) -> Gd<T> {
167+
#[cfg(debug_assertions)]
168+
assert!(
169+
self.is_initializing(),
170+
"Base::to_init_gd() can only be called during object initialization, inside I*::init() or Gd::from_init_fn()"
171+
);
172+
173+
// For manually-managed objects, regular clone is fine.
174+
// Only static type matters, because this happens immediately after initialization, so T is both static and dynamic type.
175+
if !<T::Memory as bounds::Memory>::IS_REF_COUNTED {
176+
return Gd::clone(&self.obj);
177+
}
178+
179+
// First time handing out a Gd<T>, we need to take measures to temporarily upgrade the Base's weak pointer to a strong one.
180+
// During the initialization phase (derived object being constructed), increment refcount by 1.
181+
if self.extra_strong_ref.borrow().is_none() {
182+
let strong_ref = unsafe { Gd::from_obj_sys(self.obj.obj_sys()) };
183+
*self.extra_strong_ref.borrow_mut() = Some(strong_ref);
184+
}
185+
186+
// Can't use Gd::apply_deferred(), as that implicitly borrows &mut self, causing a "destroyed while bind was active" panic.
187+
let name = format!("Base<{}> deferred unref", T::class_name());
188+
let rc = Rc::clone(&self.extra_strong_ref);
189+
let callable = Callable::from_once_fn(&name, move |_args| {
190+
Self::drop_strong_ref(rc);
191+
Ok(Variant::nil())
192+
});
193+
callable.call_deferred(&[]);
194+
195+
(*self.obj).clone()
196+
}
197+
198+
/// Drops any extra strong references, possibly causing object destruction.
199+
fn drop_strong_ref(extra_strong_ref: Rc<RefCell<Option<Gd<T>>>>) {
200+
let mut r = extra_strong_ref.borrow_mut();
201+
assert!(r.is_some());
202+
203+
*r = None; // Triggers RawGd::drop() -> dec-ref -> possibly object destruction.
204+
}
205+
206+
/// Finalizes the initialization of this `Base<T>` and returns whether
207+
pub(crate) fn mark_initialized(&mut self) -> bool {
208+
#[cfg(debug_assertions)]
209+
{
210+
assert_eq!(
211+
self.init_state.get(),
212+
InitState::ObjectConstructing,
213+
"Base<T> is already initialized, or holds a script instance"
214+
);
215+
216+
self.init_state.set(InitState::ObjectInitialized);
217+
}
218+
219+
self.extra_strong_ref.borrow().is_some()
220+
}
221+
222+
/// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed.
223+
#[doc(hidden)]
224+
pub fn __fully_constructed_gd(&self) -> Gd<T> {
225+
#[cfg(debug_assertions)]
226+
assert!(
227+
!self.is_initializing(),
228+
"WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()"
229+
);
230+
231+
(*self.obj).clone()
232+
}
233+
234+
/// Returns a [`Gd`] referencing the base object, for use in script contexts only.
235+
#[doc(hidden)]
236+
pub fn __script_gd(&self) -> Gd<T> {
237+
// Used internally by `SiMut::base()` and `SiMut::base_mut()` for script re-entrancy.
238+
// Could maybe add debug validation to ensure script context in the future.
239+
(*self.obj).clone()
240+
}
241+
101242
// Currently only used in outbound virtual calls (for scripts); search for: base_field(self).obj_sys().
102243
#[doc(hidden)]
103244
pub fn obj_sys(&self) -> sys::GDExtensionObjectPtr {
@@ -109,6 +250,36 @@ impl<T: GodotClass> Base<T> {
109250
pub(crate) fn debug_instance_id(&self) -> crate::obj::InstanceId {
110251
self.obj.instance_id()
111252
}
253+
254+
/// Returns a [`Gd`] referencing the base object, for use in script contexts only.
255+
pub(crate) fn to_script_gd(&self) -> Gd<T> {
256+
#[cfg(debug_assertions)]
257+
assert_eq!(
258+
self.init_state.get(),
259+
InitState::Script,
260+
"to_script_gd() can only be called on script-context Base objects"
261+
);
262+
263+
(*self.obj).clone()
264+
}
265+
266+
/// Returns `true` if this `Base<T>` is currently in the initializing state.
267+
#[cfg(debug_assertions)]
268+
fn is_initializing(&self) -> bool {
269+
self.init_state.get() == InitState::ObjectConstructing
270+
}
271+
272+
/// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed.
273+
#[doc(hidden)]
274+
pub fn __constructed_gd(&self) -> Gd<T> {
275+
#[cfg(debug_assertions)]
276+
assert!(
277+
!self.is_initializing(),
278+
"WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()"
279+
);
280+
281+
(*self.obj).clone()
282+
}
112283
}
113284

114285
impl<T: GodotClass> Debug for Base<T> {

godot-core/src/obj/call_deferred.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ use crate::registry::signal::ToSignalObj;
2020
#[cfg(before_api = "4.2")]
2121
pub trait WithDeferredCall<T: GodotClass> {}
2222

23+
// TODO(v0.4): seal this and similar traits.
24+
2325
/// Enables `Gd::apply_deferred()` for type-safe deferred calls.
2426
///
2527
/// The trait is automatically available for all engine-defined Godot classes and user classes containing a `Base<T>` field.

godot-core/src/obj/casts.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ impl<T: GodotClass, U: GodotClass> CastSuccess<T, U> {
4848
}
4949

5050
/// Access shared reference to destination, without consuming object.
51+
#[cfg(debug_assertions)]
5152
pub fn as_dest_ref(&self) -> &RawGd<U> {
5253
self.check_validity();
5354
&self.dest

godot-core/src/obj/script.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ pub unsafe fn create_script_instance<T: ScriptInstance>(
319319
method_lists: BoundedPtrList::new(),
320320
// SAFETY: The script instance is always freed before the base object is destroyed. The weak reference should therefore never be
321321
// accessed after it has been freed.
322-
base: unsafe { Base::from_gd(&for_object) },
322+
base: unsafe { Base::from_script_gd(&for_object) },
323323
};
324324

325325
let data_ptr = Box::into_raw(Box::new(data));
@@ -454,7 +454,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> {
454454
/// }
455455
/// ```
456456
pub fn base(&self) -> ScriptBaseRef<'_, T> {
457-
ScriptBaseRef::new(self.base_ref.to_gd(), self.mut_ref)
457+
ScriptBaseRef::new(self.base_ref.to_script_gd(), self.mut_ref)
458458
}
459459

460460
/// Returns a mutable reference suitable for calling engine methods on this object.
@@ -518,7 +518,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> {
518518
pub fn base_mut(&mut self) -> ScriptBaseMut<'_, T> {
519519
let guard = self.cell.make_inaccessible(self.mut_ref).unwrap();
520520

521-
ScriptBaseMut::new(self.base_ref.to_gd(), guard)
521+
ScriptBaseMut::new(self.base_ref.to_script_gd(), guard)
522522
}
523523
}
524524

godot-core/src/obj/traits.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -355,9 +355,13 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
355355
///
356356
/// This is intended to be stored or passed to engine methods. You cannot call `bind()` or `bind_mut()` on it, while the method
357357
/// calling `to_gd()` is still running; that would lead to a double borrow panic.
358+
///
359+
/// # Panics
360+
/// If called during initialization (the `init()` function or `Gd::from_init_fn()`). Use [`Base::to_init_gd()`] instead.
358361
fn to_gd(&self) -> Gd<Self>;
359362

360363
/// Returns a reference to the `Base` stored by this object.
364+
#[doc(hidden)]
361365
fn base_field(&self) -> &Base<Self::Base>;
362366

363367
/// Returns a shared reference suitable for calling engine methods on this object.
@@ -419,7 +423,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
419423
///
420424
/// For this, use [`base_mut()`](WithBaseField::base_mut()) instead.
421425
fn base(&self) -> BaseRef<'_, Self> {
422-
let gd = self.base_field().to_gd();
426+
let gd = self.base_field().__constructed_gd();
423427

424428
BaseRef::new(gd, self)
425429
}
@@ -490,7 +494,7 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
490494
/// ```
491495
#[allow(clippy::let_unit_value)]
492496
fn base_mut(&mut self) -> BaseMut<'_, Self> {
493-
let base_gd = self.base_field().to_gd();
497+
let base_gd = self.base_field().__constructed_gd();
494498

495499
let gd = self.to_gd();
496500
// SAFETY:

godot-core/src/registry/callbacks.rs

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,6 @@ where
124124
F: FnOnce(Base<T::Base>) -> T,
125125
{
126126
let class_name = T::class_name();
127-
128127
//out!("create callback: {}", class_name.backing);
129128

130129
let base = unsafe { Base::from_sys(base_ptr) };
@@ -133,12 +132,15 @@ where
133132
let context = || format!("panic during {class_name}::init() constructor");
134133
let code = || make_user_instance(unsafe { Base::from_base(&base) });
135134
let user_instance = handle_panic(context, std::panic::AssertUnwindSafe(code))?;
135+
136136
// Print shouldn't be necessary as panic itself is printed. If this changes, re-enable in error case:
137137
// godot_error!("failed to create instance of {class_name}; Rust init() panicked");
138138

139+
let mut base_copy = unsafe { Base::from_base(&base) };
140+
139141
let instance = InstanceStorage::<T>::construct(user_instance, base);
140-
let instance_ptr = instance.into_raw();
141-
let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr;
142+
let instance_rust_ptr = instance.into_raw();
143+
let instance_ptr = instance_rust_ptr as sys::GDExtensionClassInstancePtr;
142144

143145
let binding_data_callbacks = crate::storage::nop_instance_callbacks();
144146
unsafe {
@@ -151,7 +153,14 @@ where
151153
);
152154
}
153155

154-
// std::mem::forget(class_name);
156+
// Mark initialization as complete, now that user constructor has finished.
157+
if base_copy.mark_initialized() {
158+
// If an extra RefCounted reference was handed out, notify storage about it, so it can unreference later.
159+
let instance_ref = unsafe { &*instance_rust_ptr };
160+
instance_ref.mark_surplus_ref();
161+
}
162+
163+
// No std::mem::forget(base_copy) here, since Base may stores other fields that need deallocation.
155164
Ok(instance_ptr)
156165
}
157166

godot-core/src/storage/instance_storage.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ pub unsafe trait Storage {
119119
where
120120
Self::Instance: Inherits<<Self::Instance as GodotClass>::Base>,
121121
{
122-
self.base().to_gd().cast()
122+
self.base().__constructed_gd().cast()
123123
}
124124

125125
/// Puts self onto the heap and returns a pointer to this new heap-allocation.
@@ -139,6 +139,13 @@ pub unsafe trait Storage {
139139
log_pre_drop(self);
140140
}
141141

142+
/// For ref-counted objects, marks as owning a surplus reference.
143+
///
144+
/// Needed when a `Base<T>` hands out extra `Gd<T>` pointers during `init()`, which then requires upgrading the `Base`
145+
/// weak pointer to a strong one. To compensate, this bool flag will skip the first `inc_ref()` call, which is typically
146+
/// object construction.
147+
fn mark_surplus_ref(&self);
148+
142149
/*#[inline(always)]
143150
fn destroyed_by_godot(&self) -> bool {
144151
out!(

0 commit comments

Comments
 (0)