Fix Object::notification order

[Sauermann]

• resolve Object::notification() doesn't fully respect its p_reversed parameter #52325
• resolve Include extensions in the call ordering of notifications godot-proposals#5585
• CPP-bindings: Fix Object::notification order (cpp-bindings) godot-cpp#1151

Previously the p_reversed parameter didn't influence the order in a correct way.
Also script overridden _notification functions were not called in the correct order.

To fix this, some notification functions had to add a p_reversed parameter. This made it necessary to adjust mono and cpp bindings, since they use these functions.

MRP for testing: ReverseNotification.zip (based on notes in #52325)

• Investigate necessary changes for GDExtension (MRP: ReverseNotificationCPP.zip)
• Investigate necessary changes for mono (MRP: ReverseNotificationMono.zip)
• Ensure compatibility (see comment)
• Revert debugging print_line additions

Updated 2023-08-29: Fix merge conflict with #80394

[Maran23]

IMO we should consider writing tests for this as well (either in this PR or in a follow up).

[aaronfranke]

This PR is failing the CI checks, those need to be resolved.

[dsnopek]

The GDExtension compatibility changes are definitely headed in the right direction! I have just a couple of notes

[Sauermann]

@dsnopek Thank you for the detailed review. I believe, that I have addressed all your comments.

From other deprecation-uses, I'm aware of

#ifdef DISABLE_DEPRECATED
#endif // DISABLE_DEPRECATED

Should they be used here as well?

[dsnopek]

Thank you for the detailed review. I believe, that I have addressed all your comments.

Thanks, the changes look great!

From other deprecation-uses, I'm aware of

#ifdef DISABLE_DEPRECATED
#endif // DISABLE_DEPRECATED

Should they be used here as well?

Hm. I'm not exactly sure how we'd use that in this context.

gdextension_interface.h isn't a "normal" part of Godot's source code, it's meant to be used by other projects for interfacing with Godot, and may not even be parsed by a C/C++ compiler (as is the case for the godot-rust bindings for example), making it almost more like data than code. So, in the past we've made a point of using only the absolute minimum of preprocessor directives in gdextension_interface.h.

But maybe we could exclude the implementation of the old function in gdextension.cpp if DISABLE_DEPRECATED is defined? I think that's likely to break binary compatibility for plenty of GDExtensions, though.

[Sauermann]

Thanks for the explanation, which lets me think, that DISABLE_DEPRECATED shouldn't be used here.

[akien-mga]

Looks good to me.

[akien-mga]

Thanks!

[maiself]

This is taking the address of a temporary, which is causing crashes when the functions are called later.

[dsnopek]

Ack, right because unlike classdb_register_extension_class we're not copying the data into a new internal struct, we're directly referring to the struct provided by the GDExtension. We should probably not do that - it'd probably be better to copy that data into an instance of the struct on the Godot side...

[maiself]

Copying the data might be better in general. This is per instance tho, so either we duplicate the memory for each instance, or somehow create a mapping...

Another solution for this specific case is to add another class with a virtual override Godot side:

• ScriptInstance
• ScriptInstanceExtensionDeprecated (add this)
• ScriptInstanceExtension

Then have gdextension_script_instance_create[2] create the correct class. (edit: might require reinterpret_cast on the native_info member to make this work...)

[dsnopek]

Here's a proposed fix that copies the data: #81205

This is per instance tho, so either we duplicate the memory for each instance, or somehow create a mapping...

Hm, yeah, I suppose this does make each instance take up 23 pointers more memory... This is a trickier problem than I thought at first!

[Sauermann]

Sorry about that. Looks like I don't have a good intuition for these kind of problems.

[dsnopek]

Another solution for this specific case is to add another class with a virtual override Godot side

Yeah, I worry about the scalability of this approach: what happens as we make more changes to this struct? (We already have a change queued up for #81119)

I'd like to avoid the compatibility code continuing to require maintenance and growing in complexity over time. Copying is at least simple and straight forward. But copying as done in my current PR is always a performance and memory hit, even on the new code path. What if the hit was only on the compatibility path, maybe that's an OK trade-off?

Directly relates this comment on the PR: #81205 (comment)

[dsnopek]

Sorry about that. Looks like I don't have a good intuition for these kind of problems.

No worries - several other people (including me) read through your code without noticing it :-)

[Gallilus]

Hi running in to

So want to ask some questions.

Currently, I am using a GDExtensionScriptInstanceInfo2 without binding any methods

[Gallilus]

Why not GDExtensionScriptInstanceNotification?

[dsnopek]

Ack, yes, you're right, this should be GDExtensionScriptInstanceNotification. Luckly, the C types of the arguments are equivalent, so I don't think this breaks anything, but it should be fixed.

[Sauermann]

Is this just a straightforward rename from GDExtensionScriptInstanceInfo2 to GDExtensionScriptInstanceNotification? In that case I can create a PR. Not sure about other implications.

[dsnopek]

Yep, it should just be changing the type from GDExtensionClassNotification to GDExtensionScriptInstanceNotification

[Gallilus]

Is it possible that if not (p_info->notification_func) {script_instance_extension->deprecated_native_info.notification_func = nullptr} is needed?

[dsnopek]

No, I think this is fine. If p_info->notification_func is nullptr, then script_instance_extension->deprecated_native_info.notification_func will get set to nullptr as well.

[dsnopek]

@Gallilus I think we'll need more information about your crash to provide any helpful advice. Can you give the full stack trace? Can you share the code that's building the GDExtensionScriptInstanceInfo2 and calling the script_instance_create() function?

[Gallilus]

It tries to access a notification function when a .lr script gets attached. Only I have not defined a function jet so it should skip it. Not?

code here

void *godot::LinkerScript::_instance_create(Object *for_object) const {
    Ref<LinkerScript> top = Ref<LinkerScript>(this);
    while (top->base.is_valid()) {
        top = top->base;
        ERR_PRINT("LinkerScript::_instance_create() " + String(top->_save_path));
    }


    return create_instance(nullptr, 0, for_object, Object::cast_to<RefCounted>(for_object) != nullptr);
}


GDExtensionScriptInstancePtr godot::LinkerScript::create_instance(const Variant **p_args, int p_argcount, Object *p_owner, bool p_is_ref_counted) const {
    
    /* STEP 1, CREATE */
    LinkerInstance *instance = memnew(LinkerInstance);
    instance->_base_ref_counted = p_is_ref_counted;
    instance->_script = Ref<Script>(this);
    instance->_owner = p_owner;
    instance->_owner_id = p_owner->get_instance_id();

    /* STEP 2, INITIALIZE AND CONSTRUCT */
    instance->initialize();

    return internal::gdextension_interface_script_instance_create2 (
        &LinkerInstanceGlue::INSTANCE_INFO,
        instance
    );
}

[dsnopek]

@Gallilus:

How is LinkerInstanceGlue::INSTANCE_INFO initialized? All I could find in the code was the declaration here (where it's not initialized to any value) and this (which has all the assignments commented out).

In C++, memory isn't initialized to zero by default. So, if you aren't setting anything, LinkerInstanceGlue::INSTANCE_INFO.notification_func isn't nullptr, it's just some random value that happened to be in memory. :-)

[Gallilus]

Found the problem, I think.
I now initialised all INSTANCE_INFO functions to nullptr.
This did not help to prevent crashes on deprecated_native_info.notification_func so set that to nullptr and it seems to work.