Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create extension owner without postinitialization. #91019

Closed
wants to merge 1 commit into from
Closed

Create extension owner without postinitialization. #91019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions core/extension/gdextension_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,10 +1473,17 @@ static GDExtensionMethodBindPtr gdextension_classdb_get_method_bind(GDExtensionC
return (GDExtensionMethodBindPtr)mb;
}

#ifndef DISABLE_DEPRECATED
static GDExtensionObjectPtr gdextension_classdb_construct_object(GDExtensionConstStringNamePtr p_classname) {
const StringName classname = *reinterpret_cast<const StringName *>(p_classname);
return (GDExtensionObjectPtr)ClassDB::instantiate_no_placeholders(classname);
}
#endif

static GDExtensionObjectPtr gdextension_classdb_construct_object2(GDExtensionConstStringNamePtr p_classname) {
const StringName classname = *reinterpret_cast<const StringName *>(p_classname);
return (GDExtensionObjectPtr)ClassDB::instantiate_without_postinitialization(classname);
}

static void *gdextension_classdb_get_class_tag(GDExtensionConstStringNamePtr p_classname) {
const StringName classname = *reinterpret_cast<const StringName *>(p_classname);
Expand Down Expand Up @@ -1655,7 +1662,10 @@ void gdextension_setup_interface() {
#endif // DISABLE_DEPRECATED
REGISTER_INTERFACE_FUNC(callable_custom_create2);
REGISTER_INTERFACE_FUNC(callable_custom_get_userdata);
#ifndef DISABLE_DEPRECATED
REGISTER_INTERFACE_FUNC(classdb_construct_object);
#endif // DISABLE_DEPRECATED
REGISTER_INTERFACE_FUNC(classdb_construct_object2);
REGISTER_INTERFACE_FUNC(classdb_get_method_bind);
REGISTER_INTERFACE_FUNC(classdb_get_class_tag);
REGISTER_INTERFACE_FUNC(editor_add_plugin);
Expand Down
17 changes: 17 additions & 0 deletions core/extension/gdextension_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2592,6 +2592,7 @@ typedef void *(*GDExtensionInterfaceCallableCustomGetUserData)(GDExtensionConstT
/**
* @name classdb_construct_object
* @since 4.1
* @deprecated in Godot 4.3. Use `classdb_construct_object2` instead.
*
* Constructs an Object of the requested class.
*
Expand All @@ -2603,6 +2604,22 @@ typedef void *(*GDExtensionInterfaceCallableCustomGetUserData)(GDExtensionConstT
*/
typedef GDExtensionObjectPtr (*GDExtensionInterfaceClassdbConstructObject)(GDExtensionConstStringNamePtr p_classname);

/**
* @name classdb_construct_object2
* @since 4.3
*
* Constructs an Object of the requested class.
*
* The passed class must be a built-in godot class, or an already-registered extension class. In both cases, object_set_instance() should be called to fully initialize the object.
*
* "NOTIFICATION_POSTINITIALIZE" must be sent after construction.
*
* @param p_classname A pointer to a StringName with the class name.
*
* @return A pointer to the newly created Object.
*/
typedef GDExtensionObjectPtr (*GDExtensionInterfaceClassdbConstructObject2)(GDExtensionConstStringNamePtr p_classname);

/**
* @name classdb_get_method_bind
* @since 4.1
Expand Down
14 changes: 11 additions & 3 deletions core/object/class_db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ StringName ClassDB::get_compatibility_class(const StringName &p_class) {
return StringName();
}

Object *ClassDB::_instantiate_internal(const StringName &p_class, bool p_require_real_class) {
Object *ClassDB::_instantiate_internal(const StringName &p_class, bool p_require_real_class, bool p_skip_post_initialize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we print a warning in this function if someone tries to create an extension class while p_skip_post_initialize is true? Because with extension classes, it won't actually skip post initialize, only native classes.

This may actually be a weakness of this approach versus PR #91018 - that one could allow also creating extension classes that aren't postinitialized, whereas this one has the inconsistency that it can't do that.

Although, I'm not sure what the use case would be for creating extension classes without calling postinitialize? Maybe when we eventually support extension classes extending classes from other extensions we may need that? In that case, the class furthest down in the heirarchy may need to create an instance of its parent class, but want to defer postinitialize until after it's finished initializing. But I'm not 100% sure we'll need that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we print a warning in this function if someone tries to create an extension class while p_skip_post_initialize is true? Because with extension classes, it won't actually skip post initialize, only native classes.

I think this is redundant, ClassDB::_instantiate_internal is private function. it would not be used directly.

This may actually be a weakness of this approach versus PR #91018 - that one could allow also creating extension classes that aren't postinitialized, whereas this one has the inconsistency that it can't do that.

Both this and #91018 are can't create an extension class without postinitialization.

Although, I'm not sure what the use case would be for creating extension classes without calling postinitialize? Maybe when we eventually support extension classes extending classes from other extensions we may need that? In that case, the class furthest down in the heirarchy may need to create an instance of its parent class, but want to defer postinitialize until after it's finished initializing. But I'm not 100% sure we'll need that.

How to create an extension class is depend on extension->create_instance. If we need to create extension classes without postinitialization, we need a new GDExtensionClassCreationInfo4 to support this.

If it support extend/inherit an extension class from a GDExtension library to another library through ClassDB (expecially cross languages binding), I think create extension classes without postinitialization is required.

But for now, this goal seems too far away and the demand is not clear.
I think we should not handle it in this pr, in other words, we can add this feature in another pr if we ensure that we need it.

Copy link
Contributor

@dsnopek dsnopek May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is redundant, ClassDB::_instantiate_internal is private function. it would not be used directly.

Sure, but ClassDB::instantiate_without_postinitialization() provides a way to call it, and it's inconsistent that it won't actually skip NOTIFICATION_POSTINITIALIZE if the class is an extension class. Inside of ClassDB::_instantiate_internal() it's fairly easy to check if it's an extension class, so that'd be a good place to put a check and a warning message.

How to create an extension class is depend on extension->create_instance. If we need to create extension classes without postinitialization, we need a new GDExtensionClassCreationInfo4 to support this.

Yes. I'm suggesting that if we go with PR #91018, we should make a GDExtensionClassCreationInfo4 that changes create_instance_func to a new signature which accepts a bool, for example:

typedef GDExtensionObjectPtr (*GDExtensionClassCreateInstance2)(void *p_class_userdata, bool p_notify_postinitialize);

Then we could make ClassDB::instanstiate_without_postinitialization() consistent for both native and extension classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for now, this goal seems too far away and the demand is not clear.

Also, I think this goal isn't all that far away. With the current effort to port C# support to GDExtension, it seems likely that folks will want to have C# classes descend from classes provided by other GDExtensions. I suspect we'll be digging into this for Godot 4.4.

ClassInfo *ti;
{
OBJTYPE_RLOCK;
Expand Down Expand Up @@ -530,8 +530,12 @@ Object *ClassDB::_instantiate_internal(const StringName &p_class, bool p_require
}
}
#endif

return ti->creation_func();
if (p_skip_post_initialize) {
// If creating an extension owner, should not postinitialize here.
return ti->creation_without_postinitialization_func();
} else {
return ti->creation_func();
}
}
}

Expand All @@ -543,6 +547,10 @@ Object *ClassDB::instantiate_no_placeholders(const StringName &p_class) {
return _instantiate_internal(p_class, true);
}

Object *ClassDB::instantiate_without_postinitialization(const StringName &p_class) {
return _instantiate_internal(p_class, true, true);
}

#ifdef TOOLS_ENABLED
ObjectGDExtension *ClassDB::get_placeholder_extension(const StringName &p_class) {
ObjectGDExtension *placeholder_extension = placeholder_extensions.getptr(p_class);
Expand Down
16 changes: 15 additions & 1 deletion core/object/class_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ class ClassDB {
bool is_virtual = false;
bool is_runtime = false;
Object *(*creation_func)() = nullptr;
// This should be set if "creation_func" is set.
Object *(*creation_without_postinitialization_func)() = nullptr;

ClassInfo() {}
~ClassInfo() {}
Expand All @@ -145,6 +147,13 @@ class ClassDB {
return memnew(T);
}

template <typename T>
static Object *create_without_postinitialization() {
Object *ret = new ("") T;
ret->_initialize();
return ret;
}

static RWLock lock;
static HashMap<StringName, ClassInfo> classes;
static HashMap<StringName, StringName> resource_base_extensions;
Expand Down Expand Up @@ -183,7 +192,7 @@ class ClassDB {
static MethodBind *_bind_vararg_method(MethodBind *p_bind, const StringName &p_name, const Vector<Variant> &p_default_args, bool p_compatibility);
static void _bind_method_custom(const StringName &p_class, MethodBind *p_method, bool p_compatibility);

static Object *_instantiate_internal(const StringName &p_class, bool p_require_real_class = false);
static Object *_instantiate_internal(const StringName &p_class, bool p_require_real_class = false, bool p_skip_post_initialize = false);

public:
// DO NOT USE THIS!!!!!! NEEDS TO BE PUBLIC BUT DO NOT USE NO MATTER WHAT!!!
Expand All @@ -200,6 +209,7 @@ class ClassDB {
ClassInfo *t = classes.getptr(T::get_class_static());
ERR_FAIL_NULL(t);
t->creation_func = &creator<T>;
t->creation_without_postinitialization_func = &create_without_postinitialization<T>;
t->exposed = true;
t->is_virtual = p_virtual;
t->class_ptr = T::get_class_ptr_static();
Expand Down Expand Up @@ -228,6 +238,7 @@ class ClassDB {
ClassInfo *t = classes.getptr(T::get_class_static());
ERR_FAIL_NULL(t);
t->creation_func = &creator<T>;
t->creation_without_postinitialization_func = &create_without_postinitialization<T>;
t->exposed = false;
t->is_virtual = false;
t->class_ptr = T::get_class_ptr_static();
Expand All @@ -244,6 +255,7 @@ class ClassDB {
ERR_FAIL_NULL(t);
ERR_FAIL_COND_MSG(t->inherits_ptr && !t->inherits_ptr->creation_func, vformat("Cannot register runtime class '%s' that descends from an abstract parent class.", T::get_class_static()));
t->creation_func = &creator<T>;
t->creation_without_postinitialization_func = &create_without_postinitialization<T>;
t->exposed = true;
t->is_virtual = false;
t->is_runtime = true;
Expand All @@ -268,6 +280,7 @@ class ClassDB {
ClassInfo *t = classes.getptr(T::get_class_static());
ERR_FAIL_NULL(t);
t->creation_func = &_create_ptr_func<T>;
t->creation_without_postinitialization_func = &_create_ptr_func<T>;
t->exposed = true;
t->class_ptr = T::get_class_ptr_static();
t->api = current_api;
Expand All @@ -290,6 +303,7 @@ class ClassDB {
static bool is_virtual(const StringName &p_class);
static Object *instantiate(const StringName &p_class);
static Object *instantiate_no_placeholders(const StringName &p_class);
static Object *instantiate_without_postinitialization(const StringName &p_class);
static void set_object_extension_instance(Object *p_object, const StringName &p_class, GDExtensionClassInstancePtr p_instance);

static APIType get_api_type(const StringName &p_class);
Expand Down
6 changes: 5 additions & 1 deletion core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,13 @@ void Object::cancel_free() {
_predelete_ok = false;
}

void Object::_postinitialize() {
void Object::_initialize() {
_class_name_ptr = _get_class_namev(); // Set the direct pointer, which is much faster to obtain, but can only happen after postinitialize.
_initialize_classv();
_class_name_ptr = nullptr; // May have been called from a constructor.
}

void Object::_postinitialize() {
notification(NOTIFICATION_POSTINITIALIZE);
}

Expand Down Expand Up @@ -2155,6 +2158,7 @@ bool predelete_handler(Object *p_object) {
}

void postinitialize_handler(Object *p_object) {
p_object->_initialize();
p_object->_postinitialize();
}

Expand Down
1 change: 1 addition & 0 deletions core/object/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,7 @@ class Object {
int _predelete_ok = 0;
ObjectID _instance_id;
bool _predelete();
void _initialize();
void _postinitialize();
bool _can_translate = true;
bool _emitting = false;
Expand Down
Loading