Emit POSTINITIALIZE notification after init()

[beicause]

Changes:

• classdb_construct_object -> classdb_construct_object2
• string_new_with_utf8_chars_and_len -> string_new_with_utf8_chars_and_len2

After this PR, NOTIFICATION_POSTINITIALIZE will be sent after init() and can be received correctly in on_notification() in godot 4.4 or later. See also test_notifications in the itest.

Partially addresses #557

[Bromeon]

Thanks!

Changes:

• classdb_construct_object -> classdb_construct_object2
• string_new_with_utf8_chars_and_len -> string_new_with_utf8_chars_and_len2

There are many more changes than these two. Please edit your initial description to elaborate what exactly the PR does and its motivation.

Follow-up to #1208.

[GodotRust]

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

[beicause]

I'm relatively new to gdext. It seems that we can't modify the object itself (e.g.add_child) within the init function, so issue godotengine/godot#91023 does not occur in gdext ?

[Bromeon]

Part of that is what I'm looking into in #557.

This PR seems to integrate with Godot's own POST_INIT notification. Can you demonstrate what it enables with some code examples?

Please edit the initial post.

[beicause]

Unfortunately, I encountered a crash when using POSTINITIALIZE to create a ArrayMesh. The code:

#[derive(GodotClass)]
#[class(base=ArrayMesh,init,tool)]
pub struct EmptyTriangleMesh {
    base: Base<ArrayMesh>,
}

#[godot_api]
#[allow(unused_variables)]
impl IArrayMesh for EmptyTriangleMesh {
    fn on_notification(&mut self, what: notify::ObjectNotification) {
        if what == notify::ObjectNotification::POSTINITIALIZE {
            let mut arrays = VariantArray::new();
            arrays.resize(
                mesh::ArrayType::MAX.ord().try_into().unwrap(),
                &Variant::nil(),
            );
            let indices = PackedInt32Array::from([0, 1, 2]);
            arrays.set(
                mesh::ArrayType::INDEX.ord().try_into().unwrap(),
                &indices.to_variant(),
            );
            self.base_mut()
                .add_surface_from_arrays_ex(mesh::PrimitiveType::TRIANGLES, &arrays)
                .flags(mesh::ArrayFormat::FLAG_USES_EMPTY_VERTEX_ARRAY)
                .done();
        }
    }

    fn get_surface_count(&self) -> i32 {
        todo!()
    }

    fn surface_get_array_len(&self, index: i32) -> i32 {
        todo!()
    }

    fn surface_get_array_index_len(&self, index: i32) -> i32 {
        todo!()
    }

    fn surface_get_arrays(&self, index: i32) -> VariantArray {
        todo!()
    }

    fn surface_get_blend_shape_arrays(&self, index: i32) -> Array<VariantArray> {
        todo!()
    }

    fn surface_get_lods(&self, index: i32) -> Dictionary {
        todo!()
    }

    fn surface_get_format(&self, index: i32) -> u32 {
        todo!()
    }

    fn surface_get_primitive_type(&self, index: i32) -> u32 {
        todo!()
    }

    fn surface_set_material(&mut self, index: i32, material: Option<Gd<Material>>) {
        todo!()
    }

    fn surface_get_material(&self, index: i32) -> Option<Gd<Material>> {
        todo!()
    }

    fn get_blend_shape_count(&self) -> i32 {
        todo!()
    }

    fn get_blend_shape_name(&self, index: i32) -> StringName {
        todo!()
    }

    fn set_blend_shape_name(&mut self, index: i32, name: StringName) {
        todo!()
    }

    fn get_aabb(&self) -> Aabb {
        todo!()
    }
}

ERROR: Destroyed an object from Godot side, while a bind() or bind_mut() call was active.
  This is a bug in your code that may cause UB and logic errors. Make sure that objects are not
  destroyed while you still hold a Rust reference to them, or use Gd::free() which is safe.
  object: Base { id: -9223369839861485696, class: EmptyTriangleMesh }
   at: crash (./core/core_bind.cpp:344)

So I must be missing something.

[Yarwin]

Hmm, interesting – it seems that our refcount is still unset before we pass it to godot (after "create" function will yield the result). base_mut() call increases&decreases the refcount, causing freeing the instance prematurely. Messing with refcount before we return created instance will cause leak or/and UB 🤔. I.e. it is extacly the same situation as the one in the init.

I checked what godot-cpp is doing, and it seems that their flow is the same? (create instance -> construct object 2 -> set instance -> set instance binding -> and finally, a postinitialize).

https://github.com/godotengine/godot-cpp/blob/6a870949a5d38c62638af23dc13210f77154aa2c/include/godot_cpp/core/class_db.hpp#L124
https://github.com/godotengine/godot-cpp/blob/6a870949a5d38c62638af23dc13210f77154aa2c/src/classes/wrapped.cpp#L69

[beicause]

Surprisingly, I tested the following code on godot-cpp-template:

#include "example_class.h"

void ExampleClass::_bind_methods() {
	godot::ClassDB::bind_method(D_METHOD("print_type", "variant"), &ExampleClass::print_type);
}

void ExampleClass::_notification(int p_what) {
	if (p_what == NOTIFICATION_POSTINITIALIZE) {
		Ref<ExampleClass> t = this;
		UtilityFunctions::print("ref count: ", t->get_reference_count());
	}
}

void ExampleClass::print_type(const Variant &p_variant) const {
	print_line(vformat("Type: %d", p_variant.get_type()));
}

extends Node


func _ready() -> void:
	var example := ExampleClass.new()
	example.print_type(example)

It prints a error that indicates example is null:

ref count: 1
SCRIPT ERROR: Cannot call method 'print_type' on a null value.
          at: _ready (res://example.gd:6)
          GDScript backtrace (most recent call first):
              [0] _ready (res://example.gd:6)

But if I remove Ref<ExampleClass> t = this;, it will work properly:

ref count: 1
Type: 24

So could this be a issue with Godot?

Edit: Additionally, obtaining Ref<ExampleClass> in constructor causes the instance to be released, too.

[Yarwin]

Yep, it seems to be a problem with Godot itself – such behavior in init might be expected but it shouldn't happen in post-init (basically forces users to use gdscript to do anything while initializing the RefCounted) – and we have no reasonable, easy way to prevent it 🤔 .

[Bromeon]

@beicause you closed godotengine/godot#108395 with:

I do see postinit executes immediately after the instance is constructed, at which point the RefCounted::refcount_init has not been initialized. That is to say, the behavior of postinit is similar to constructor.
This seems unsolvable, I don't see how RefCounted can distinguish whether it is being referenced within postinit/constructor or externally

Given that, does it make sense to support POSTINITIALIZE notifications at all in godot-rust -- if it cannot be used safely?

If yes, what is the use case?
As mentioned, accessing base methods during init() should be solved by #557 (which I'm working on, but is hard due to the same issue that you mentioned -- ref-count not being initialized at the time).

In case we decide not to implement POSTINITIALIZE, we should also document this, maybe in on_notification 🤔

[beicause]

POSTINITIALIZE may be useful, as it allows base classes to execute logic after subclass initialization - for example Control updates its theme cache during POSTINITIALIZE. And godotengine/godot#91018 (comment) 🤔

If #557 gets resolved, could the same solution enable POSTINITIALIZE to access base class?

[Bromeon]

I see... Then we would need to emit it.

But how to handle it properly in on_notification, especially for RefCounted classes?

[Yarwin]

But how to handle it properly in on_notification, especially for RefCounted classes?

One silly idea I had was using deferred refcount with base() and base_mut() until post_notification has been properly emitted – decreasing the refcount would be then resolved by Callable with call_deferred at the end of a given frame.

Very silly hack, it adds some, potentially pretty serious, overhead, but hey, it works

[beicause]

I've used a hack to avoid releasing RefCounted in this PR:

let mut obj = Gd::<T>::from_obj_sys_weak(object_ptr).upcast_object();
obj.notify(crate::classes::notify::ObjectNotification::POSTINITIALIZE);
std::mem::forget(obj);

[Yarwin]

I've used a hack to avoid releasing RefCounted in this PR:

It is not a hack, it is totally sensible thing to do – don't increase the refcount while passing an object to the user internally.

What I've been thinking is something along lines of:

    if let Ok(object_ptr) = create_custom(T::__godot_user_init) {
        let mut obj = Gd::<T>::from_obj_sys_weak(object_ptr).upcast_object();
        if T::inherits::<RefCounted>() {
            <classes::Object as Bounds>::DynMemory::maybe_inc_ref(&mut obj.raw);
            obj.linked_callable("deferred refcount", move |_| {
                // Run RawGd<T> destructor.
                drop(Gd::<T>::from_obj_sys_weak(object_ptr));
                Ok(Variant::nil())
            }).call_deferred(&[]);
        }
        
        obj.notify(crate::classes::notify::ObjectNotification::POSTINITIALIZE);
        std::mem::forget(obj);
        object_ptr

It works but is very janky – instance is being destroyed at the end of a frame, not imminently, appending callable to every RefCounted has some inherent overhead etc – thus if we resort to such a hack, it should be explicitly called by user (by invoking base for the first time before ObjectNotification::POSTINITIALIZE is emitted).

Don't actually implement it – firstly we need to reason if it is sensible thing to do, check for potential side effects etc 😅

[beicause]

This seems fragile. There's no guarantee the object remains alive when the callable executes. I'd prefer having something like from_obj_sys_weak to obtain a weak ref and make it doesn't automatically trigger a drop.

[Yarwin]

This seems fragile.

I don't like it either. It is choose-yer-poison situation (on another hand nothing better comes to my mind).

There's no guarantee the object remains alive when the callable executes.

linked_callable is automatically invalidated by Godot when linked object goes out of the scope. The bigger problems are crashes in Drop – they will crash the whole godot runtime without much explanation which is… not ideal 😅 .

…and objects staying alive longer than they should are not perfect either.

I'd prefer having something like from_obj_sys_weak to obtain a weak ref and make it doesn't automatically trigger a drop.

Yeah, I would prefer it as well – unfortunately a lot of interactions with Godot require increasing the RefCount, thus making this way inherently unsound (but it would allow user to "route" calls via call_deferred so maybeee 🤔) .

I'll underline – I'm not (and don't strive to be) authoritative in this matter, it is just one possible solution 🤷

[Bromeon]

This seems fragile. There's no guarantee the object remains alive when the callable executes. I'd prefer having something like from_obj_sys_weak to obtain a weak ref and make it doesn't automatically trigger a drop.

Weak ref doesn't keep the object alive though -- that's the point.
If someone can rug-pull the instance below, then this is unsound 🤔

[beicause]

Weak ref doesn't keep the object alive though -- that's the point. If someone can rug-pull the instance below, then this is unsound 🤔

It's true. Even if it is unsound, it's still better than nothing. Why can't we ever provide unsafe APIs?

[Bromeon]

It's true. Even if it is unsound, it's still better than nothing. Why can't we ever provide unsafe APIs?

First, unsafe != unsound. The latter means there's a violation of Rust's memory safety principles. In other words, a bug that must be fixed.

Obviously we can provide unsafe APIs, and there are some in godot-rust. But object creation is such a central operation, where unsafety would introduce a disproportionate amount of mental overhead. I would only go that route if we have truly exhausted all other options, and even then only for specialized APIs, not general construction.

That said, I didn't test your code. It is possible it works well. Scenarios that we need to protect against:

• for manually-managed objects, someone calling self.free() in the post-init hook
• for reference-counted objects, someone indirectly triggering destruction of self (if possible?)
  • I don't count RefCounted::unreference() here. That method is explicitly removed from the API due to being dangerous, and we can at some point also introduce runtime checks for reflection calls.

And panic in these cases is good enough. But UB (undefined behavior) isn't.