Access to base pointer during initialization

[Bromeon]

Closes #557.
Closes #997.

Important

This may or may not be the final API. See below for trade-offs.

---

Feature

Allows accessing Gd<Base> during construction.

Current API:

let obj = Gd::from_init_fn(|base| {
    let mut gd: Gd<Node2D> = base.to_init_gd();
    gd.set_rotation(FRAC_PI_3);

    MyClass { base, i: 456 }
});

// Values are propagated to derived object.
assert_eq!(obj.get_rotation(), FRAC_PI_3);

Importantly, this works for RefCounted, without UB or memory leaks.

Implementation

For manually-managed objects, the implementation is simple. Just copy the 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.

Alternatives and trade-offs

Alternative 1: Initer type-state

Problem is that Base<T> needs to store an extra field to keep track of the strong-ref. The current implementation does this for all Ts, but it would be easy to limit it to RefCounted classes.

This means that most user-defined types will also need 8 extra bytes, if they declare a Base field. While this is not terrible, I don't like the thought of everyone paying for this, when access-during-init is a rare use case, and some people may not need it at all.

I was thus also thinking about an alternative API:

// Name TBD, could be Initer, PreBase, ...
pub struct Initer<B> { ... }
impl<B: GodotClass> Initer {
    fn to_gd(&self); 
    fn into_base(self);
}

Gd::from_init_fn and ITrait::init() would then take Initer instead of Base as a parameter.

// Here, init is of type Initer<Node2D>.
let obj = Gd::from_init_fn(|init| {
    let mut gd: Gd<Node2D> = init.to_gd();
    gd.set_rotation(FRAC_PI_3);

    // instead of MyClass { base, i: 456 }:
    MyClass {
        base: init.into_base(),
        i: 456,
    }
});

This has two main advantages:

• Base no longer needs to store the strong ref, which is anyway only useful during initialization.
• Since to_gd() is available on Initer and not on Base, it's statically impossible to call it after initialization.
  • Although I believe that to_init_gd() is clear enough to not be called later, so this is more of a theoretical nicety.

The drawbacks are:

• Breaking change for all init functions and closures.
  • This would affect almost all users, maybe mitigated by the fact that many use #[class(init)] instead of manual inits.
• Instead of simply passing through base, the user now needs the syntactically heavier base: init.into_base(), although there's no benefit in 99% of cases.
• Extra type state increases overall API complexity.

One option would also be to allow both APIs, taking Base or Initer depending on needs. This could be done backwards-compatibly, but would not only mean we need a Gd::from_init2_fn, but also ITrait::init2². But it would enlarge the API surface even further.

Alternative 2: global state

Another option is to store a global HashMap<InstanceId, Gd<RefCounted>> with the interim strong pointers. This could also simplify the Rc<RefCell<Option<Gd<T>>>> which is currently needed. It would effectively remove the field from all Base instances, but still keep the information around.

We would need to ensure thread safety, but I'm happy to accept synchronization overhead for this rather rare case. One thing we do need to make sure is is thread-safe deferred dec-ref. This is already the case for the current implementation.

The nice thing about this approach is that it is fully backwards-compatible with the current API, needs no new "initer" concept and still allows to reduce the size of the Base type. In other words, it's a good example of "pay what you use".

Alternative 3: Godot per-instance metadata

We could use Godot's Object::set_meta() to store per-object metadata. This is generally an interesting idea, however we would need to make sure that:

• This metadata is accessible throughout the lifecycle (i.e. also during construction of the extension instance).
• No one deletes/modifies it -- we can't prevent this.

A Rust-based storage mechanism would be safer, as we're fully in control.

---

So, I'm not yet sure. I'd like to get the current version merged soon-ish, so we have a base (:monocle_face:) for further iteration. This might also be a candidate to be postponed to v0.4, to give us some time to iterate.

Footnotes

1. It is tempting to just store a bool flag instead of another strong Gd, since we already have the weak Gd as a field. However, we need to have a physical Gd to uphold the strong guarantee. When that is dropped, the Base becomes dangling. So, an enum might be possible, but it's not taking up less space. ↩

2. Unless we do some dirty macro-tricks like f64/f32 in process/physics_process, but I don't think that helps ease-of-use 🤔 ↩

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1273

[beicause]

The side effect is that any RefCounted using to_init_gd can't be freed until the next frame, whereas those not using to_init_gd can be freed immediately. This is unexpected, especially when it implements custom Drop.

In godot-cpp I only need to use this pointer in constructor and postinit, I don't really hope godot-rust lack the quivalent functionality. By the way ndarray has unsafe things like RawArrayView without a lifetime - otherwise I couldn't store ArrayView fields in godot-rust.

[Bromeon]

The side effect is that any RefCounted using to_init_gd can't be freed until the next frame, whereas those not using to_init_gd can be freed immediately. This is unexpected, especially when it implements custom Drop.

This is clearly documented though.

But I'm happy to hear how we can safely implement immediate drop. I've only managed that for Rust-side construction, not MyClass.new() from GDScript.

In godot-cpp I only need to use this pointer in constructor and postinit, I don't really hope godot-rust lack the quivalent functionality.

This PR allows that. Why is deferred destruction a problem?

[Yarwin]

When I was experimenting with late init I've been doing pretty much the same stuff – just with atomics instead of Rc<...>. While Rc is objectively a better choice (and maybe the best) it is still not very good.

Initer sounds like hell to maintain, I would strongly vote against it.

I haven't thought about global state before and it seems like the best approach – it can be "glued" on top of existing implementation to cover fairly niche use case and just works ™️. I don't see any inherent issues related to it as well 🤔, albeit I've never dig into this approach.

Using metadata feels ultra-hacky and unreliable.

IMO there should be another GDExtension callback for it similar to gdscript _init – which is called when script is being attached to the instance (there is no fixed place where it happens).

…and there is always a fifth option – gave up, document it, and advise to use gdscript-glue when necessary (It would make accessing post_init impossible though) 😅. Doesn't feel (and definitively isn't) like the best way to solve this problem.

---

This means that most user-defined types will also need 8 extra bytes, if they declare a Base field. While this is not terrible, I don't like the thought of everyone paying for this, when access-during-init is a rare use case, and some people may not need it at all.

It ain't that terrible since Godot itself tends to not optimize for memory usage (with an exception of cases when there is some inherent problem with it or where it matters a lot). It doesn't mean that we shouldn't care at all – it means that someone who cares a lot about memory usage would rather use other APIs (i.e. servers and Rids, rust structs attached to nodes instead of other nodes etc) 🤔 – but yeah, paying a toll on everything because of fairly niche usecase (required mostly for effect compositors and few others) feels silly.

[Bromeon]

I implemented alternative 2: global variable. Since we are currently limited to the main thread due to call_deferred(), I used thread_local. Technically we could use UnsafeCell & Co. here, but it's not a hot path we need to optimize for.

From my side, once I fixed CI, this would be ready for an initial implementation.

@beicause I'm not against future improvements for earlier destruction, but I'd like to proceed since this fixes a long-standing issue, even if not perfectly. I'm happy to hear concrete proposals about how immediate destruction (also for GDScript-side MyClass.new()) would be possible, but it might be that we'd need to take this upstream to Godot.

[beicause]

Deferred destruction may be fine in most cases, as it's already the case for script languages with GC. However, I don't really like having to use the hack and performance overheads since RefCounted is a very core type.

I don't know whether the upstream could have a solution for this. For example MyClass.new().do_something(), the instance is destroyed before it is referenced, how could it possibly implement a callback after initialized? Not sure when the RefCounted is initialized.

I have a naive idea that adding a weak gd that does not affect reference count. To make it safe, we can check whether the instance id is still valid before each method call in debug mode, while disabling the check in release mode.

[Bromeon]

I don't know whether the upstream could have a solution for this. For example MyClass.new().do_something(), the instance is destroyed before it is referenced, how could it possibly implement a callback after initialized?

Not sure I follow this example -- the do_something() would obtain a reference, just like in var a = MyClass.new(), the RefCounted isn't destroyed before assigned to a 🤔

---

But generally it is possible to immediately destruct. We have this FFI callback, which you know already from your other PR:

gdext/godot-core/src/registry/callbacks.rs

Lines 34 to 39 in 870c725

	pub unsafe extern "C" fn create<T: cap::GodotDefault>(
	_class_userdata: *mut std::ffi::c_void,
	_notify_postinitialize: sys::GDExtensionBool,
	) -> sys::GDExtensionObjectPtr {
	create_custom(T::__godot_user_init).unwrap_or(std::ptr::null_mut())
	}

And we call the create_custom function directly (not through FFI) for NewGd::new_gd() and Gd::from_init_fn(), e.g. here:

gdext/godot-core/src/obj/gd.rs

Lines 154 to 162 in 870c725

	pub fn from_init_fn<F>(init: F) -> Self
	where
	F: FnOnce(crate::obj::Base<T::Base>) -> T,
	{
	let object_ptr = callbacks::create_custom(init) // or propagate panic.
	.unwrap_or_else(|payload| PanicPayload::repanic(payload));
	unsafe { Gd::from_obj_sys(object_ptr) }
	}

Now, create_custom could be modified to inc-ref (keep a strong-ref), and Gd::from_sys() could do a dec-ref as soon as the reference is assigned somewhere else. This would be "immediate" regarding observable behavior: get_reference_count() is 1 as soon as the Gd<T> is constructed.

The problem is the FFI call here -- we are not in control what happens after the C function create() is called. But if Godot had a hook for "immediately after init" (the role of post-init, but done within Godot, not delegated to bindings), then we could also run such a dec-ref in that hook.

I was thinking if we should do immediate-destruction for Rust-constructed objects, and deferred for GDScript ones. But I'm not sure if the behavior then isn't even more confusing?

---

I have a naive idea that adding a weak gd that does not affect reference count. To make it safe, we can check whether the instance id is still valid before each method call in debug mode, while disabling the check in release mode.

There is no concept of "weak Gd" in godot-rust. Every public-facing Gd is always strong.

There are some places where we internally use weak Gds, like inside Base; but that's mostly an implementation detail because I can't be bothered having yet another pointer type just for that 😀

But if it's public-facing, it would likely need to be its own type due to very different semantics, just like Rc/Weak or Arc/Weak. And this would add a lot of API + implementation surface for what is now one use case, and in that only a performance optimization (and that because of a Godot quirk).

Maybe using Rust-side immediate destruction is better? For now I don't want to spend more time on implementation, but I'll gladly discuss this further and see what people think 🙂