Update for new NOTIFICATION_POSTINITIALIZE handling #1568
Conversation
dsnopek
added
the
enhancement
dsnopek
force-pushed
the
post-initialize-44
branch
from
5b14ef1
to
79d1be7
Compare
|
Test this commit with MRP in this issue and v4.4.dev.custom_build [61598c5c8], not crsh occur. |
dsnopek
force-pushed
the
post-initialize-44
branch
from
79d1be7
to
808d0f4
Compare
|
@Daylily-Zeleen I tested duplicating "MyNode" in your MRP, and it's working for me! We still need someone who is not me (as the author of this PR) to review and approve it, though :-) |
There was a problem hiding this comment.
I understand most of this PR is just copied over code from the GDExtension reference. I had a quick check, it looks correct to me. Since it was tested already, I don't see why we shouldn't be able to push for a merge.
I noticed that we have a POSTINITIALIZE notification example in the example.cpp. Perhaps that should be updated?
| @@ -292,7 +293,7 @@ typedef struct { | |||
| GDExtensionClassGetVirtual get_virtual_func; // Queries a virtual function by name and returns a callback to invoke the requested virtual function. | |||
| GDExtensionClassGetRID get_rid_func; | |||
| void *class_userdata; // Per-class user data, later accessible in instance bindings. | |||
| } GDExtensionClassCreationInfo; // Deprecated. Use GDExtensionClassCreationInfo3 instead. | |||
| } GDExtensionClassCreationInfo; // Deprecated. Use GDExtensionClassCreationInfo4 instead. | |||
There was a problem hiding this comment.
I doubt anyone will see a deprecation comment. Is there a reason we are avoiding [[deprecated("Use GDExtensionClassCreationInfo4 instead.")]]?
There was a problem hiding this comment.
This header is using a restricted subset of C: we want it to be possible to compile it with a C or C++ compiler, but it isn't meant to be "real code" because some bindings parse it, rather than compile it.
[[deprecated ...]] is a C++ thing, but even if it was C, I think we probably wouldn't use it anyway because it could create problems for bindings that manually parse the header.
FYI, I'd love to replace this with a JSON or XML definition that we generate the C code from, rather than the header being the definitive source.
| if constexpr (!std::is_abstract_v<T>) { | ||
| T *new_object = memnew(T); | ||
| Wrapped::_set_construct_info<T>(); |
There was a problem hiding this comment.
By replacing the memnew call with this, does the change imply that any Object created with memnew will not have postinitialize called? Should godot-cpp users be using ClassDB::instantiate (or just this function) instead henceforth? If so, we should update the docs accordingly.
There was a problem hiding this comment.
This PR shouldn't change anything about how developers use godot-cpp, this should be a purely internal change within godot-cpp.
If you look a couple lines down, it will call _postinitialize(), but only if p_notify_postinitialize is true. We're not using memnew() because that should always lead to NOTIFICATION_POSTINITIALIZE, but here we need to be able to control whether we're doing it or not.
There was a problem hiding this comment.
Ahh that makes sense. Thanks for explaining!
This adds support for the Godot changes from PR godotengine/godot#91018
With this change, we should finally be handling
NOTIFICATION_POSTINITIALIZEin a way that matches how it works in Godot itself, which should allow (among other things) truly internal child nodes.@Daylily-Zeleen If you have time, could you test that this works in your project?
Fixes #1567