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.

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

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented Apr 22, 2024

An alternative of #91018.

This pr reduce changes (no need to change GodotSharp), but a new pointer field “creation_without_postinitialization_func” needs to be introduced, it will increase memory usage (in my eos plugin, it will add hundreds of classes, 8 bytes per class look like expensive).


Fix #91023.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks for finding this issue!

Compared to PR #91018, I like this one better. However, I'm not yet sure if this is the right solution either.

It was PR godotengine/godot-cpp#1321 that has lead to godot-cpp sending NOTIFICATION_POSTINITIALIZE twice in some situations. I wonder if there's a better way to implement that which won't have this problem?

For example, notifications are called parent first, and we will have already sent it for the full hierarchy of the "owner class" - maybe we could send it only for the classes on the extension side?

core/object/class_db.cpp Outdated Show resolved Hide resolved
Comment on lines 1476 to 1485
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);
return (GDExtensionObjectPtr)ClassDB::instantiate_without_postinitialization(classname);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we can change classdb_construct_object for backwards compatibility reasons - I think we may need to create a classdb_construct_object2 (with a 2 appended) in gdextension_interface.h.

We only recently decided that it was the responsibility of the GDExtension binding to send NOTIFICATION_POSTINITIALIZE to instances of extension classes created from within the GDExtension binding (see godotengine/godot-cpp#1269 (comment)). I'm not sure that any other GDExtension binding is actually doing that yet, except for godot-cpp. So, if we changed classdb_construct_object to not send that notification, then it won't happen at all for bindings that haven't added their own mechanism to send it, which could break the "owner class".

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/create_extension_owner_ithout_postinitialization branch from 352c4a6 to b2c65e7 Compare May 1, 2024 04:55
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

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.

@dsnopek
Copy link
Contributor

dsnopek commented May 16, 2024

As mentioned over on PR #91018, I think I personally prefer #91018 at this point. But it would be nice to get some feedback from other GDExtension team members.

@dsnopek
Copy link
Contributor

dsnopek commented May 29, 2024

Closing this one in favor of #91018

@dsnopek dsnopek closed this May 29, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone May 29, 2024
@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/create_extension_owner_ithout_postinitialization branch June 4, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants